Restart at most once when prepopulating repository rule environment

[Silic0nS0ldier]

When a repository rule is fetch attributes are iterated over in enforceLabelAttributes to prepopulate the environment, restarting the fetch each time a new dependency is discovered.

This is faster than calling repository_ctx.path(...) early in the repository rule implementation function but still has considerable overhead.

This PR defers throwing NeedsSkyframeRestartException till after every attribute has been processed, greatly reducing the number of restarts and iterations.

In an internal project these optimisations are particularly noticeable.
Before: 2min 8s, 2min 7s
After: 1min 35s, 1min 27s

[fmeum]

This also allows downloads of label{_list} dependencies to be performed in parallel, which is nice.

The ideal solution would probably be to turn StarlarkBaseExternalContext#getPathFromLabel into a function that accepts a list of Labels and uses env.getValuesAndExceptions throughout for "batched" Skyframe requests, but that would require a larger refactoring. This looks like a much simpler fix that should give most of the same benefits.

CC @Wyverald

[Wyverald]

nice, thank you!

[fmeum]

@bazel-io flag

[iancha1992]

@bazel-io fork 7.1.0

[iancha1992]

@fmeum @Wyverald I will put this for 7.1.0 for now since it does not look like a regression
cc: @bazelbuild/triage

[Silic0nS0ldier]

since it does not look like a regression

To confirm, this is not a fix for a regression (at least not a recent one since Bazel 6.4.0 also benefits).

---

The ideal solution would probably be to turn StarlarkBaseExternalContext#getPathFromLabel into a function that accepts a list of Labels and uses env.getValuesAndExceptions throughout for "batched" Skyframe requests, but that would require a larger refactoring.

Would be more efficient and likely require less code. I think it would make sense to batch that work with resolving // TODO(wyv): somehow migrate this to the base context too..

Looking at src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java#fetchInternal there are plenty of places where null is returned (indicating a restart is required). There may yet be more restarts that can be eliminated.

---

I see that this has been tagged for Bazel 7.1. Any chance this can be included in the next v6 release?

[iancha1992]

cc: @Wyverald @meteorcloudy

[meteorcloudy]

Any chance this can be included in the next v6 release?

We don't have 6.5 release planned. By our release policy, we'll no longer backport features to the previous LTS when a new one is out, only critical bug fixes. We might reconsider if there are actually enough interests for a new 6.5 release if that helps with the migration to 7.

[aignas]

@meteorcloudy, since 6.5 has been decided to happen, is this PR something that you would be willing to include in the release? Having these performance improvements could be helpful in the context of rules authors needing to support non-latest bazel version and it may allow to not worry about the restarts as much.

[fmeum]

@bazel-io fork 6.5.0

[iancha1992]

The changes in this PR have been included in Bazel 6.5.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

[Silic0nS0ldier]

Tested 6.5.0rc1 locally (macOS) and on CI (Linux), no issues.

Change is working as advertised too. Local test case finished about a minute faster.

[iancha1992]

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!