From 468f177334d4375f762e1dcdb13c51feeed11dfb Mon Sep 17 00:00:00 2001 From: janakr Date: Fri, 10 Aug 2018 23:36:52 -0700 Subject: Refactoring of SkyFunctionEnvironment to iterate over events/postables only when they're actually being put into a committed value. The previous behavior submitted deps' events twice, when the dep was added and when the node finished building. The intention is to build on this refactoring to cut off events/postables across the analysis-execution boundary, so that actions are not carrying around nested sets of warnings coming from their configured targets. This will be safe because to execute an action, we must already have analyzed its configured target, so the warning would have been emitted there. As can be seen from the changed test, this is not a pure behavior no-op. We will now emit cached events slightly later, on value committal, rather than on first dep declaration. This should not be an issue: since the events are cached, the user must have already seen them on a prior build, so the delay should not be important. Inversely, we now report events slightly more quickly during bubbling up, since we report them at each stage, as opposed to just at ParallelEvaluator evaluation completion. PiperOrigin-RevId: 208316502 --- .../AbstractExceptionalParallelEvaluator.java | 8 ++--- .../build/skyframe/SkyFunctionEnvironment.java | 35 ++++++++++------------ 2 files changed, 20 insertions(+), 23 deletions(-) (limited to 'src/main') diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java index 315aa8458c..f8f21f254e 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractExceptionalParallelEvaluator.java @@ -513,8 +513,8 @@ public abstract class AbstractExceptionalParallelEvaluator errorKey, ValueWithMetadata.error( ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)), - env.buildEvents(parentEntry, /*expectDoneDeps=*/ false), - env.buildPosts(parentEntry, /*expectDoneDeps=*/ false))); + env.buildAndReportEvents(parentEntry, /*expectDoneDeps=*/ false), + env.buildAndReportPostables(parentEntry, /*expectDoneDeps=*/ false))); continue; } } finally { @@ -526,8 +526,8 @@ public abstract class AbstractExceptionalParallelEvaluator errorKey, ValueWithMetadata.error( ErrorInfo.fromChildErrors(errorKey, ImmutableSet.of(error)), - env.buildEvents(parentEntry, /*expectDoneDeps=*/ false), - env.buildPosts(parentEntry, /*expectDoneDeps=*/ false))); + env.buildAndReportEvents(parentEntry, /*expectDoneDeps=*/ false), + env.buildAndReportPostables(parentEntry, /*expectDoneDeps=*/ false))); } // Reset the interrupt bit if there was an interrupt from outside this evaluator interrupt. diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index b5fb94c751..f879a18efd 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java @@ -264,7 +264,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { Preconditions.checkState(building, skyKey); } - NestedSet buildEvents(NodeEntry entry, boolean expectDoneDeps) + NestedSet buildAndReportEvents(NodeEntry entry, boolean expectDoneDeps) throws InterruptedException { if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); @@ -277,14 +277,17 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { GroupedList depKeys = entry.getTemporaryDirectDeps(); Collection deps = - getDepValuesForDoneNodeFromErrorOrDepsOrGraph(depKeys, expectDoneDeps); + getDepValuesForDoneNodeFromErrorOrDepsOrGraph( + depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements()); for (SkyValue value : deps) { eventBuilder.addTransitive(ValueWithMetadata.getEvents(value)); } - return eventBuilder.build(); + NestedSet result = eventBuilder.build(); + evaluatorContext.getReplayingNestedSetEventVisitor().visit(result); + return result; } - NestedSet buildPosts(NodeEntry entry, boolean expectDoneDeps) + NestedSet buildAndReportPostables(NodeEntry entry, boolean expectDoneDeps) throws InterruptedException { if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) { return NestedSetBuilder.emptySet(Order.STABLE_ORDER); @@ -294,11 +297,14 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { GroupedList depKeys = entry.getTemporaryDirectDeps(); Collection deps = - getDepValuesForDoneNodeFromErrorOrDepsOrGraph(depKeys, expectDoneDeps); + getDepValuesForDoneNodeFromErrorOrDepsOrGraph( + depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements()); for (SkyValue value : deps) { postBuilder.addTransitive(ValueWithMetadata.getPosts(value)); } - return postBuilder.build(); + NestedSet result = postBuilder.build(); + evaluatorContext.getReplayingNestedSetPostableVisitor().visit(result); + return result; } void setValue(SkyValue newValue) { @@ -415,8 +421,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { *

If {@code assertDone}, this asserts that all deps in {@code depKeys} are done. */ private Collection getDepValuesForDoneNodeFromErrorOrDepsOrGraph( - GroupedList depKeys, boolean assertDone) throws InterruptedException { - int keySize = depKeys.numElements(); + Iterable depKeys, boolean assertDone, int keySize) throws InterruptedException { List result = new ArrayList<>(keySize); // depKeys may contain keys in newlyRegisteredDeps whose values have not yet been retrieved from // the graph during this environment's lifetime. @@ -424,7 +429,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { ArrayList missingKeys = expectedMissingKeys > 0 ? new ArrayList<>(expectedMissingKeys) : null; - for (SkyKey key : depKeys.getAllElementsAsIterable()) { + for (SkyKey key : depKeys) { SkyValue value = maybeGetValueFromErrorOrDeps(key); if (value == null) { if (key == ErrorTransienceValue.KEY) { @@ -556,12 +561,6 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { if (!previouslyRequestedDepsValues.containsKey(depKey)) { newlyRequestedDeps.add(depKey); - evaluatorContext - .getReplayingNestedSetPostableVisitor() - .visit(ValueWithMetadata.getPosts(depValue)); - evaluatorContext - .getReplayingNestedSetEventVisitor() - .visit(ValueWithMetadata.getEvents(depValue)); } } newlyRequestedDeps.endGroup(); @@ -708,8 +707,8 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { // (2) value == null && enqueueParents happens for values that are found to have errors // during a --keep_going build. - NestedSet posts = buildPosts(primaryEntry, /*expectDoneDeps=*/ true); - NestedSet events = buildEvents(primaryEntry, /*expectDoneDeps=*/ true); + NestedSet posts = buildAndReportPostables(primaryEntry, /*expectDoneDeps=*/ true); + NestedSet events = buildAndReportEvents(primaryEntry, /*expectDoneDeps=*/ true); SkyValue valueWithMetadata; if (value == null) { @@ -780,8 +779,6 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { evaluatorContext.signalValuesAndEnqueueIfReady( skyKey, reverseDeps, currentVersion, enqueueParents); - evaluatorContext.getReplayingNestedSetPostableVisitor().visit(posts); - evaluatorContext.getReplayingNestedSetEventVisitor().visit(events); return enqueueParents == EnqueueParentBehavior.ENQUEUE ? null : reverseDeps; } -- cgit v1.2.3