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

2 files changed, 20 insertions, 23 deletions

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<E extends Exception>
               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<E extends Exception>
           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<TaggedEvents> buildEvents(NodeEntry entry, boolean expectDoneDeps)
+  NestedSet<TaggedEvents> 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<SkyKey> depKeys = entry.getTemporaryDirectDeps();
     Collection<SkyValue> deps =
-        getDepValuesForDoneNodeFromErrorOrDepsOrGraph(depKeys, expectDoneDeps);
+        getDepValuesForDoneNodeFromErrorOrDepsOrGraph(
+            depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements());
     for (SkyValue value : deps) {
       eventBuilder.addTransitive(ValueWithMetadata.getEvents(value));
     }
-    return eventBuilder.build();
+    NestedSet<TaggedEvents> result = eventBuilder.build();
+    evaluatorContext.getReplayingNestedSetEventVisitor().visit(result);
+    return result;
   }
 
-  NestedSet<Postable> buildPosts(NodeEntry entry, boolean expectDoneDeps)
+  NestedSet<Postable> 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<SkyKey> depKeys = entry.getTemporaryDirectDeps();
     Collection<SkyValue> deps =
-        getDepValuesForDoneNodeFromErrorOrDepsOrGraph(depKeys, expectDoneDeps);
+        getDepValuesForDoneNodeFromErrorOrDepsOrGraph(
+            depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements());
     for (SkyValue value : deps) {
       postBuilder.addTransitive(ValueWithMetadata.getPosts(value));
     }
-    return postBuilder.build();
+    NestedSet<Postable> result = postBuilder.build();
+    evaluatorContext.getReplayingNestedSetPostableVisitor().visit(result);
+    return result;
   }
 
   void setValue(SkyValue newValue) {
@@ -415,8 +421,7 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
    * <p>If {@code assertDone}, this asserts that all deps in {@code depKeys} are done.
    */
   private Collection<SkyValue> getDepValuesForDoneNodeFromErrorOrDepsOrGraph(
-      GroupedList<SkyKey> depKeys, boolean assertDone) throws InterruptedException {
-    int keySize = depKeys.numElements();
+      Iterable<SkyKey> depKeys, boolean assertDone, int keySize) throws InterruptedException {
     List<SkyValue> 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<SkyKey> 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<Postable> posts = buildPosts(primaryEntry, /*expectDoneDeps=*/ true);
-    NestedSet<TaggedEvents> events = buildEvents(primaryEntry, /*expectDoneDeps=*/ true);
+    NestedSet<Postable> posts = buildAndReportPostables(primaryEntry, /*expectDoneDeps=*/ true);
+    NestedSet<TaggedEvents> 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;
   }