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
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 5e8f708..028d4c8 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.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.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.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 @@
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 242a5ac..9fad67e 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 @@
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 0b859d2..3dc527e 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 @@
* 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 e8f9a91..42ab317 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 @@
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 @@
differencer,
progressReceiver,
GraphInconsistencyReceiver.THROWING,
+ DEFAULT_STORED_EVENT_FILTER,
new EmittedEventState(),
true);
}
@@ -99,12 +101,14 @@
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 @@
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 cc0332a..73f39ac 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 @@
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 f879a18..fc808b0 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 @@
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 @@
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 @@
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 @@
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 8254f2e..805029a 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 @@
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 3fc2e27..a2fa72c 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 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 @@
Differencer differencer,
EvaluationProgressReceiver progressReceiver,
GraphInconsistencyReceiver graphInconsistencyReceiver,
+ EventFilter eventFilter,
boolean keepEdges) {
return new InMemoryMemoizingEvaluator(
functions,
differencer,
progressReceiver,
graphInconsistencyReceiver,
+ eventFilter,
emittedEventState,
true);
}
@@ -4057,6 +4060,62 @@
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 @@
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 @@
differencer,
progressReceiver,
graphInconsistencyReceiver,
+ eventFilter,
keepEdges);
this.driver = getBuildDriver(evaluator);
}
@@ -5077,6 +5138,14 @@
}
/**
+ * 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 3f25c5d..7266060 100755
--- a/src/test/shell/integration/execution_phase_tests.sh
+++ b/src/test/shell/integration/execution_phase_tests.sh
@@ -268,4 +268,28 @@
"--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."
