Filter out events from analysis when constructing execution-phase values in Skyframe.

This avoids some unnecessary iteration over already-emitted events that can show up in profiles, and allows us to store execution-phase values a bit more compactly, since we don't need to carry around wrapper objects and nested sets everywhere.

This crucially depends on the fact that we can't build a target in the execution phase without first having analyzed it in a separate Skyframe call. Skyframe normally propagates all events/posts up the graph because it must be able to emit them if a user requests a node that only transitively depends on the node that emitted an event. However, because we do analysis in a separate Skyframe call, any warnings/posts associated with the analysis nodes will be emitted then, and we don't need to propagate them into execution.

PiperOrigin-RevId: 208767078

9 files changed, 159 insertions, 6 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 5e8f708444..028d4c8ee0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -19,6 +19,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Throwables;
 import com.google.common.cache.Cache;
@@ -88,6 +89,7 @@ import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.ErrorSensingEventHandler;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
@@ -146,8 +148,10 @@ import com.google.devtools.build.skyframe.Differencer.DiffWithDelta.Delta;
 import com.google.devtools.build.skyframe.ErrorInfo;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver;
 import com.google.devtools.build.skyframe.EvaluationResult;
+import com.google.devtools.build.skyframe.EventFilter;
 import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
 import com.google.devtools.build.skyframe.ImmutableDiff;
+import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.Injectable;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
 import com.google.devtools.build.skyframe.MemoizingEvaluator.EvaluatorSupplier;
@@ -670,11 +674,45 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
             evaluatorDiffer(),
             progressReceiver,
             graphInconsistencyReceiver,
+            DEFAULT_FILTER_WITH_ACTIONS,
             emittedEventState,
             tracksStateForIncrementality());
     buildDriver = getBuildDriver();
   }
 
+  /**
+   * Use the fact that analysis of a target must occur before execution of that target, and in a
+   * separate Skyframe evaluation, to avoid propagating events from configured target nodes (and
+   * more generally action lookup nodes) to action execution nodes. We take advantage of the fact
+   * that if a node depends on an action lookup node and is not itself an action lookup node, then
+   * it is an execution-phase node: the action lookup nodes are terminal in the analysis phase.
+   */
+  private static final EventFilter DEFAULT_FILTER_WITH_ACTIONS =
+      new EventFilter() {
+        @Override
+        public boolean storeEventsAndPosts() {
+          return true;
+        }
+
+        @Override
+        public boolean apply(Event input) {
+          // Use the filtering defined in the default filter: no info/progress messages.
+          return InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER.apply(input);
+        }
+
+        @Override
+        public Predicate<SkyKey> depEdgeFilterForEventsAndPosts(SkyKey primaryKey) {
+          if (primaryKey instanceof ActionLookupValue.ActionLookupKey) {
+            return Predicates.alwaysTrue();
+          } else {
+            return NO_ACTION_LOOKUP;
+          }
+        }
+      };
+
+  private static final Predicate<SkyKey> NO_ACTION_LOOKUP =
+      (key) -> !(key instanceof ActionLookupValue.ActionLookupKey);
+
   protected SkyframeProgressReceiver newSkyframeProgressReceiver() {
     return new SkyframeProgressReceiver();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 242a5ac66e..9fad67e516 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -342,6 +342,7 @@ public abstract class AbstractPackageLoader implements PackageLoader {
             preinjectedDifferencer,
             new EvaluationProgressReceiver.NullEvaluationProgressReceiver(),
             GraphInconsistencyReceiver.THROWING,
+            InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER,
             new MemoizingEvaluator.EmittedEventState(),
             /*keepEdges=*/ false));
   }
diff --git a/src/main/java/com/google/devtools/build/skyframe/EventFilter.java b/src/main/java/com/google/devtools/build/skyframe/EventFilter.java
index 0b859d24ff..3dc527ea34 100644
--- a/src/main/java/com/google/devtools/build/skyframe/EventFilter.java
+++ b/src/main/java/com/google/devtools/build/skyframe/EventFilter.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
 import com.google.devtools.build.lib.events.Event;
 
 /** Filters out events which should not be stored during evaluation in {@link ParallelEvaluator}. */
@@ -23,4 +24,8 @@ public interface EventFilter extends Predicate<Event> {
    * avoid doing unnecessary work when evaluating node entries.
    */
   boolean storeEventsAndPosts();
+
+  default Predicate<SkyKey> depEdgeFilterForEventsAndPosts(SkyKey primaryKey) {
+    return Predicates.alwaysTrue();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index e8f9a91648..42ab317df3 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -63,6 +63,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
   private final InvalidationState deleterState = new DeletingInvalidationState();
   private final Differencer differencer;
   private final GraphInconsistencyReceiver graphInconsistencyReceiver;
+  private final EventFilter eventFilter;
 
   // Keep edges in graph. Can be false to save memory, in which case incremental builds are
   // not possible.
@@ -90,6 +91,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
         differencer,
         progressReceiver,
         GraphInconsistencyReceiver.THROWING,
+        DEFAULT_STORED_EVENT_FILTER,
         new EmittedEventState(),
         true);
   }
@@ -99,12 +101,14 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
       Differencer differencer,
       @Nullable EvaluationProgressReceiver progressReceiver,
       GraphInconsistencyReceiver graphInconsistencyReceiver,
+      EventFilter eventFilter,
       EmittedEventState emittedEventState,
       boolean keepEdges) {
     this.skyFunctions = ImmutableMap.copyOf(skyFunctions);
     this.differencer = Preconditions.checkNotNull(differencer);
     this.progressReceiver = new DirtyTrackingProgressReceiver(progressReceiver);
     this.graphInconsistencyReceiver = Preconditions.checkNotNull(graphInconsistencyReceiver);
+    this.eventFilter = eventFilter;
     this.graph = new InMemoryGraphImpl(keepEdges);
     this.emittedEventState = emittedEventState;
     this.keepEdges = keepEdges;
@@ -187,7 +191,7 @@ public final class InMemoryMemoizingEvaluator implements MemoizingEvaluator {
               skyFunctions,
               eventHandler,
               emittedEventState,
-              DEFAULT_STORED_EVENT_FILTER,
+              eventFilter,
               ErrorInfoManager.UseChildErrorInfoIfNecessary.INSTANCE,
               keepGoing,
               numThreads,
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index cc0332af6f..73f39acc99 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -166,6 +166,7 @@ public interface MemoizingEvaluator {
         Differencer differencer,
         EvaluationProgressReceiver progressReceiver,
         GraphInconsistencyReceiver graphInconsistencyReceiver,
+        EventFilter eventFilter,
         EmittedEventState emittedEventState,
         boolean keepEdges);
   }
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 f879a18efd..fc808b024f 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java
@@ -266,7 +266,8 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
 
   NestedSet<TaggedEvents> buildAndReportEvents(NodeEntry entry, boolean expectDoneDeps)
       throws InterruptedException {
-    if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) {
+    EventFilter eventFilter = evaluatorContext.getStoredEventFilter();
+    if (!eventFilter.storeEventsAndPosts()) {
       return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     }
     NestedSetBuilder<TaggedEvents> eventBuilder = NestedSetBuilder.stableOrder();
@@ -278,7 +279,11 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     GroupedList<SkyKey> depKeys = entry.getTemporaryDirectDeps();
     Collection<SkyValue> deps =
         getDepValuesForDoneNodeFromErrorOrDepsOrGraph(
-            depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements());
+            Iterables.filter(
+                depKeys.getAllElementsAsIterable(),
+                eventFilter.depEdgeFilterForEventsAndPosts(skyKey)),
+            expectDoneDeps,
+            depKeys.numElements());
     for (SkyValue value : deps) {
       eventBuilder.addTransitive(ValueWithMetadata.getEvents(value));
     }
@@ -289,7 +294,8 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
 
   NestedSet<Postable> buildAndReportPostables(NodeEntry entry, boolean expectDoneDeps)
       throws InterruptedException {
-    if (!evaluatorContext.getStoredEventFilter().storeEventsAndPosts()) {
+    EventFilter eventFilter = evaluatorContext.getStoredEventFilter();
+    if (!eventFilter.storeEventsAndPosts()) {
       return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     }
     NestedSetBuilder<Postable> postBuilder = NestedSetBuilder.stableOrder();
@@ -298,7 +304,11 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment {
     GroupedList<SkyKey> depKeys = entry.getTemporaryDirectDeps();
     Collection<SkyValue> deps =
         getDepValuesForDoneNodeFromErrorOrDepsOrGraph(
-            depKeys.getAllElementsAsIterable(), expectDoneDeps, depKeys.numElements());
+            Iterables.filter(
+                depKeys.getAllElementsAsIterable(),
+                eventFilter.depEdgeFilterForEventsAndPosts(skyKey)),
+            expectDoneDeps,
+            depKeys.numElements());
     for (SkyValue value : deps) {
       postBuilder.addTransitive(ValueWithMetadata.getPosts(value));
     }
diff --git a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
index 8254f2eaf3..805029af93 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ValueWithMetadata.java
@@ -279,7 +279,8 @@ public abstract class ValueWithMetadata implements SkyValue {
     return null;
   }
 
-  static NestedSet<TaggedEvents> getEvents(SkyValue value) {
+  @VisibleForTesting
+  public static NestedSet<TaggedEvents> getEvents(SkyValue value) {
     if (value instanceof ValueWithMetadata) {
       return ((ValueWithMetadata) value).getTransitiveEvents();
     }
diff --git a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
index 3fc2e27ff7..a2fa72ce7f 100644
--- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java
@@ -26,6 +26,7 @@ import static org.junit.Assert.fail;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -114,12 +115,14 @@ public class MemoizingEvaluatorTest {
       Differencer differencer,
       EvaluationProgressReceiver progressReceiver,
       GraphInconsistencyReceiver graphInconsistencyReceiver,
+      EventFilter eventFilter,
       boolean keepEdges) {
     return new InMemoryMemoizingEvaluator(
         functions,
         differencer,
         progressReceiver,
         graphInconsistencyReceiver,
+        eventFilter,
         emittedEventState,
         true);
   }
@@ -4057,6 +4060,62 @@ public class MemoizingEvaluatorTest {
     assertThat(parentEvaluated.getCount()).isEqualTo(1);
   }
 
+  @Test
+  public void depEventPredicate() throws Exception {
+    if (!eventsStored()) {
+      return;
+    }
+    SkyKey parent = GraphTester.toSkyKey("parent");
+    SkyKey excludedDep = GraphTester.toSkyKey("excludedDep");
+    SkyKey includedDep = GraphTester.toSkyKey("includedDep");
+    tester.setEventFilter(
+        new EventFilter() {
+          @Override
+          public boolean storeEventsAndPosts() {
+            return true;
+          }
+
+          @Override
+          public boolean apply(Event input) {
+            return true;
+          }
+
+          @Override
+          public Predicate<SkyKey> depEdgeFilterForEventsAndPosts(SkyKey primaryKey) {
+            if (primaryKey.equals(parent)) {
+              return includedDep::equals;
+            } else {
+              return Predicates.alwaysTrue();
+            }
+          }
+        });
+    tester.initialize(/*keepEdges=*/ true);
+    tester
+        .getOrCreate(parent)
+        .addDependency(excludedDep)
+        .addDependency(includedDep)
+        .setComputedValue(CONCATENATE);
+    tester
+        .getOrCreate(excludedDep)
+        .setWarning("excludedDep warning")
+        .setConstantValue(new StringValue("excludedDep"));
+    tester
+        .getOrCreate(includedDep)
+        .setWarning("includedDep warning")
+        .setConstantValue(new StringValue("includedDep"));
+    tester.eval(/*keepGoing=*/ false, includedDep, excludedDep);
+    assertThatEvents(eventCollector).containsExactly("excludedDep warning", "includedDep warning");
+    eventCollector.clear();
+    emittedEventState.clear();
+    tester.eval(/*keepGoing=*/ true, parent);
+    assertThatEvents(eventCollector).containsExactly("includedDep warning");
+    assertThat(
+            ValueWithMetadata.getEvents(
+                tester.driver.getEntryForTesting(parent).getValueMaybeWithMetadata()))
+        .containsExactly(
+            new TaggedEvents(null, ImmutableList.of(Event.warn("includedDep warning"))));
+  }
+
   // Tests that we have a sane implementation of error transience.
   @Test
   public void errorTransienceBug() throws Exception {
@@ -5055,6 +5114,7 @@ public class MemoizingEvaluatorTest {
     private TrackingProgressReceiver progressReceiver = new TrackingProgressReceiver();
     private GraphInconsistencyReceiver graphInconsistencyReceiver =
         GraphInconsistencyReceiver.THROWING;
+    private EventFilter eventFilter = InMemoryMemoizingEvaluator.DEFAULT_STORED_EVENT_FILTER;
 
     public void initialize(boolean keepEdges) {
       this.differencer = getRecordingDifferencer();
@@ -5064,6 +5124,7 @@ public class MemoizingEvaluatorTest {
               differencer,
               progressReceiver,
               graphInconsistencyReceiver,
+              eventFilter,
               keepEdges);
       this.driver = getBuildDriver(evaluator);
     }
@@ -5077,6 +5138,14 @@ public class MemoizingEvaluatorTest {
     }
 
     /**
+     * Sets the {@link #eventFilter}. {@link #initialize} must be called after this to have any
+     * effect.
+     */
+    public void setEventFilter(EventFilter eventFilter) {
+      this.eventFilter = eventFilter;
+    }
+
+    /**
      * Sets the {@link #graphInconsistencyReceiver}. {@link #initialize} must be called after this
      * to have any effect.
      */
diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh
index 3f25c5df09..7266060b50 100755
--- a/src/test/shell/integration/execution_phase_tests.sh
+++ b/src/test/shell/integration/execution_phase_tests.sh
@@ -268,4 +268,28 @@ function test_jobs_default_auto() {
       "--jobs was not set to auto by default"
 }
 
+function test_analysis_warning_cached() {
+  mkdir -p "foo" "bar" || fail "Could not create directories"
+  cat > foo/BUILD <<'EOF' || fail "foo/BUILD"
+cc_library(
+    name = 'foo',
+    deprecation = 'foo warning',
+    srcs = ['foo.cc'],
+    visibility = ['//visibility:public']
+)
+EOF
+  cat > bar/BUILD <<'EOF' || fail "bar/BUILD"
+cc_library(name = 'bar', srcs = ['bar.cc'], deps = ['//foo:foo'])
+EOF
+  touch foo/foo.cc bar/bar.cc || fail "Couldn't touch"
+  bazel build --nobuild //bar:bar >& "$TEST_log" || fail "Expected success"
+  expect_log "WARNING: .*: foo warning"
+  bazel build //bar:bar >& "$TEST_log" || fail "Expected success"
+  expect_log "WARNING: .*: foo warning"
+  echo "// comment" >> bar/bar.cc || fail "Couldn't change contents"
+  bazel build //bar:bar >& "$TEST_log" || fail "Expected success"
+  expect_log "WARNING: .*: foo warning"
+}
+
+
 run_suite "Integration tests of ${PRODUCT_NAME} using the execution phase."