From 7574d84e61086b5835a2b2cc003ca72fcfcf4fe4 Mon Sep 17 00:00:00 2001 From: janakr Date: Tue, 14 Aug 2018 21:38:51 -0700 Subject: 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 --- .../build/lib/skyframe/SkyframeExecutor.java | 38 ++++++++++++ .../skyframe/packages/AbstractPackageLoader.java | 1 + .../devtools/build/skyframe/EventFilter.java | 5 ++ .../build/skyframe/InMemoryMemoizingEvaluator.java | 6 +- .../build/skyframe/MemoizingEvaluator.java | 1 + .../build/skyframe/SkyFunctionEnvironment.java | 18 ++++-- .../devtools/build/skyframe/ValueWithMetadata.java | 3 +- .../build/skyframe/MemoizingEvaluatorTest.java | 69 ++++++++++++++++++++++ .../shell/integration/execution_phase_tests.sh | 24 ++++++++ 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 depEdgeFilterForEventsAndPosts(SkyKey primaryKey) { + if (primaryKey instanceof ActionLookupValue.ActionLookupKey) { + return Predicates.alwaysTrue(); + } else { + return NO_ACTION_LOOKUP; + } + } + }; + + private static final Predicate 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 { * avoid doing unnecessary work when evaluating node entries. */ boolean storeEventsAndPosts(); + + default Predicate 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 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 eventBuilder = NestedSetBuilder.stableOrder(); @@ -278,7 +279,11 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { GroupedList depKeys = entry.getTemporaryDirectDeps(); Collection 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 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 postBuilder = NestedSetBuilder.stableOrder(); @@ -298,7 +304,11 @@ class SkyFunctionEnvironment extends AbstractSkyFunctionEnvironment { GroupedList depKeys = entry.getTemporaryDirectDeps(); Collection 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 getEvents(SkyValue value) { + @VisibleForTesting + public static NestedSet 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 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); } @@ -5076,6 +5137,14 @@ public class MemoizingEvaluatorTest { this.progressReceiver = progressReceiver; } + /** + * 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." -- cgit v1.2.3