Skip to content

Commit

Permalink
Restart at most once when prepopulating repository rule environment
Browse files Browse the repository at this point in the history
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

Closes #20434.

PiperOrigin-RevId: 588090528
Change-Id: I7917b137d6e60b6d6a73189cf396418a85b3ec28
  • Loading branch information
Silic0nS0ldier authored and bazel-io committed Dec 21, 2023
1 parent 8e2972b commit be9fb70
Showing 1 changed file with 20 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -516,43 +516,56 @@ public String toString() {
* potentially more expensive operations.
*/
// TODO(wyv): somehow migrate this to the base context too.
public void enforceLabelAttributes() throws EvalException, InterruptedException {
public void enforceLabelAttributes()
throws EvalException, InterruptedException, NeedsSkyframeRestartException {
// TODO: If a labels fails to resolve to an existing regular file, we do not add a dependency on
// that fact - if the file is created later or changes its type, it will not trigger a rerun of
// the repository function.
StructImpl attr = getAttr();
// Batch restarts as they are expensive
boolean needsRestart = false;
for (String name : attr.getFieldNames()) {
Object value = attr.getValue(name);
if (value instanceof Label) {
dependOnLabelIgnoringErrors((Label) value);
if (dependOnLabelIgnoringErrors((Label) value)) {
needsRestart = true;
}
}
if (value instanceof Sequence) {
for (Object entry : (Sequence) value) {
if (entry instanceof Label) {
dependOnLabelIgnoringErrors((Label) entry);
if (dependOnLabelIgnoringErrors((Label) entry)) {
needsRestart = true;
}
}
}
}
if (value instanceof Dict) {
for (Object entry : ((Dict) value).keySet()) {
if (entry instanceof Label) {
dependOnLabelIgnoringErrors((Label) entry);
if (dependOnLabelIgnoringErrors((Label) entry)) {
needsRestart = true;
}
}
}
}
}

if (needsRestart) {
throw new NeedsSkyframeRestartException();
}
}

private void dependOnLabelIgnoringErrors(Label label)
throws InterruptedException, NeedsSkyframeRestartException {
private boolean dependOnLabelIgnoringErrors(Label label) throws InterruptedException {
try {
getPathFromLabel(label);
} catch (NeedsSkyframeRestartException e) {
throw e;
return true;
} catch (EvalException e) {
// EvalExceptions indicate labels not referring to existing files. This is fine,
// as long as they are never resolved to files in the execution of the rule; we allow
// non-strict rules.
}
return false;
}
}

0 comments on commit be9fb70

Please sign in to comment.