diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java index c96c178d7fc86a..b58e57c1dd0815 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java @@ -258,13 +258,14 @@ public void enqueueing(SkyKey skyKey) { @Override public void evaluated(SkyKey skyKey, SkyValue node, EvaluationState state) { SkyFunctionName type = skyKey.functionName(); - if (type == SkyFunctions.TARGET_COMPLETION) { + if (type == SkyFunctions.TARGET_COMPLETION && node != null) { TargetCompletionValue val = (TargetCompletionValue) node; ConfiguredTarget target = val.getConfiguredTarget(); builtTargets.add(target); eventBus.post(TargetCompleteEvent.createSuccessful(target)); } else if (type == SkyFunctions.ACTION_EXECUTION) { - // Remember all completed actions, regardless of having been cached or really executed. + // Remember all completed actions, even those in error, regardless of having been cached or + // really executed. actionCompleted((Action) skyKey.argument()); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index 857c23187fa94f..1221f0bd92a463 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -495,7 +495,7 @@ public void enqueueing(SkyKey skyKey) {} @Override public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) { - if (skyKey.functionName() == SkyFunctions.CONFIGURED_TARGET) { + if (skyKey.functionName() == SkyFunctions.CONFIGURED_TARGET && value != null) { if (state == EvaluationState.BUILT) { evaluatedConfiguredTargets.add(skyKey); // During multithreaded operation, this is only set to true, so no concurrency issues. diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java index 7928878e987a0a..aeef2e4404fff4 100644 --- a/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java +++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationProgressReceiver.java @@ -15,6 +15,8 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety; +import javax.annotation.Nullable; + /** * Receiver to inform callers which values have been invalidated. Values may be invalidated and then * re-validated if they have been found not to be changed. @@ -66,12 +68,12 @@ enum InvalidationState { void enqueueing(SkyKey skyKey); /** - * Notifies that {@code value} has been evaluated. + * Notifies that the node for {@code skyKey} has been evaluated. * - *

{@code state} indicates the new state of the value. + *

{@code state} indicates the new state of the node. * - *

This method is not called if the value builder threw an error when building this value. + *

If the value builder threw an error when building this node, then {@code value} is null. */ @ThreadSafety.ThreadSafe - void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state); + void evaluated(SkyKey skyKey, @Nullable SkyValue value, EvaluationState state); } diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java index a99105847b7e36..55c1ab59c02be2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java @@ -555,43 +555,38 @@ void commit(boolean enqueueParents) { // during a --keep_going build. NestedSet events = buildEvents(/*missingChildren=*/false); + Version valueVersion; + SkyValue valueWithMetadata; if (value == null) { Preconditions.checkNotNull(errorInfo, "%s %s", skyKey, primaryEntry); - // We could consider using max(childVersions) here instead of graphVersion. When full - // versioning is implemented, this would allow evaluation at a version between - // max(childVersions) and graphVersion to re-use this result. - Set reverseDeps = primaryEntry.setValue( - ValueWithMetadata.error(errorInfo, events), graphVersion); - signalValuesAndEnqueueIfReady(enqueueParents ? visitor : null, reverseDeps, graphVersion); + valueWithMetadata = ValueWithMetadata.error(errorInfo, events); } else { // We must be enqueueing parents if we have a value. Preconditions.checkState(enqueueParents, "%s %s", skyKey, primaryEntry); - Set reverseDeps; - Version valueVersion; - // If this entry is dirty, setValue may not actually change it, if it determines that - // the data being written now is the same as the data already present in the entry. - // We could consider using max(childVersions) here instead of graphVersion. When full - // versioning is implemented, this would allow evaluation at a version between - // max(childVersions) and graphVersion to re-use this result. - reverseDeps = primaryEntry.setValue( - ValueWithMetadata.normal(value, errorInfo, events), graphVersion); - // Note that if this update didn't actually change the value entry, this version may not - // be the graph version. - valueVersion = primaryEntry.getVersion(); - Preconditions.checkState(valueVersion.atMost(graphVersion), - "%s should be at most %s in the version partial ordering", - valueVersion, graphVersion); - if (progressReceiver != null) { - // Tell the receiver that this value was built. If valueVersion.equals(graphVersion), it - // was evaluated this run, and so was changed. Otherwise, it is less than graphVersion, - // by the Preconditions check above, and was not actually changed this run -- when it was - // written above, its version stayed below this update's version, so its value remains the - // same as before. - progressReceiver.evaluated(skyKey, value, - valueVersion.equals(graphVersion) ? EvaluationState.BUILT : EvaluationState.CLEAN); - } - signalValuesAndEnqueueIfReady(visitor, reverseDeps, valueVersion); - } + valueWithMetadata = ValueWithMetadata.normal(value, errorInfo, events); + } + // If this entry is dirty, setValue may not actually change it, if it determines that + // the data being written now is the same as the data already present in the entry. + // We could consider using max(childVersions) here instead of graphVersion. When full + // versioning is implemented, this would allow evaluation at a version between + // max(childVersions) and graphVersion to re-use this result. + Set reverseDeps = primaryEntry.setValue(valueWithMetadata, graphVersion); + // Note that if this update didn't actually change the value entry, this version may not + // be the graph version. + valueVersion = primaryEntry.getVersion(); + Preconditions.checkState(valueVersion.atMost(graphVersion), + "%s should be at most %s in the version partial ordering", + valueVersion, graphVersion); + if (progressReceiver != null) { + // Tell the receiver that this value was built. If valueVersion.equals(graphVersion), it + // was evaluated this run, and so was changed. Otherwise, it is less than graphVersion, + // by the Preconditions check above, and was not actually changed this run -- when it was + // written above, its version stayed below this update's version, so its value remains the + // same as before. + progressReceiver.evaluated(skyKey, value, + valueVersion.equals(graphVersion) ? EvaluationState.BUILT : EvaluationState.CLEAN); + } + signalValuesAndEnqueueIfReady(enqueueParents ? visitor : null, reverseDeps, valueVersion); visitor.notifyDone(skyKey); replayingNestedSetEventVisitor.visit(events); @@ -815,7 +810,7 @@ public void run() { visitor.notifyDone(skyKey); Set reverseDeps = state.markClean(); SkyValue value = state.getValue(); - if (progressReceiver != null && value != null) { + if (progressReceiver != null) { // Tell the receiver that the value was not actually changed this run. progressReceiver.evaluated(skyKey, value, EvaluationState.CLEAN); } @@ -1054,19 +1049,16 @@ private void informProgressReceiverThatValueIsDone(SkyKey key) { NodeEntry entry = graph.get(key); Preconditions.checkState(entry.isDone(), entry); SkyValue value = entry.getValue(); - if (value != null) { - Version valueVersion = entry.getVersion(); - Preconditions.checkState(valueVersion.atMost(graphVersion), - "%s should be at most %s in the version partial ordering", valueVersion, graphVersion); - // Nodes with errors will have no value. Don't inform the receiver in that case. - // For most nodes we do not inform the progress receiver if they were already done - // when we retrieve them, but top-level nodes are presumably of more interest. - // If valueVersion is not equal to graphVersion, it must be less than it (by the - // Preconditions check above), and so the node is clean. - progressReceiver.evaluated(key, value, valueVersion.equals(graphVersion) - ? EvaluationState.BUILT - : EvaluationState.CLEAN); - } + Version valueVersion = entry.getVersion(); + Preconditions.checkState(valueVersion.atMost(graphVersion), + "%s should be at most %s in the version partial ordering", valueVersion, graphVersion); + // For most nodes we do not inform the progress receiver if they were already done when we + // retrieve them, but top-level nodes are presumably of more interest. + // If valueVersion is not equal to graphVersion, it must be less than it (by the + // Preconditions check above), and so the node is clean. + progressReceiver.evaluated(key, value, valueVersion.equals(graphVersion) + ? EvaluationState.BUILT + : EvaluationState.CLEAN); } } 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 00df824492a6e4..67a22ccfe4c648 100644 --- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java @@ -486,11 +486,11 @@ private void setupDiamondDependency() { .setComputedValue(COPY); } - // Regression test: ParallelEvaluator notifies ValueProgressReceiver of already-built top-level - // values in error: we built "top" and "mid" as top-level targets; "mid" contains an error. We - // make sure "mid" is built as a dependency of "top" before enqueuing mid as a top-level target - // (by using a latch), so that the top-level enqueuing finds that mid has already been built. The - // progress receiver should not be notified of any value having been evaluated. + // ParallelEvaluator notifies ValueProgressReceiver of already-built top-level values in error: we + // built "top" and "mid" as top-level targets; "mid" contains an error. We make sure "mid" is + // built as a dependency of "top" before enqueuing mid as a top-level target (by using a latch), + // so that the top-level enqueuing finds that mid has already been built. The progress receiver + // should be notified that mid has been built. @Test public void alreadyAnalyzedBadTarget() throws Exception { final SkyKey mid = GraphTester.toSkyKey("mid"); @@ -523,11 +523,11 @@ public void accept(SkyKey key, EventType type, Order order, Object context) { tester.eval(/*keepGoing=*/false, top, mid); assertEquals(0L, valueSet.getCount()); trackingAwaiter.assertNoErrors(); - assertThat(tester.invalidationReceiver.evaluated).isEmpty(); + assertThat(tester.invalidationReceiver.evaluated).containsExactly(mid); } @Test - public void receiverNotToldOfVerifiedValueDependingOnCycle() throws Exception { + public void receiverToldOfVerifiedValueDependingOnCycle() throws Exception { SkyKey leaf = GraphTester.toSkyKey("leaf"); SkyKey cycle = GraphTester.toSkyKey("cycle"); SkyKey top = GraphTester.toSkyKey("top"); @@ -535,12 +535,12 @@ public void receiverNotToldOfVerifiedValueDependingOnCycle() throws Exception { tester.getOrCreate(cycle).addDependency(cycle); tester.getOrCreate(top).addDependency(leaf).addDependency(cycle); tester.eval(/*keepGoing=*/true, top); - assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf).inOrder(); + assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf, top, cycle); tester.invalidationReceiver.clear(); tester.getOrCreate(leaf, /*markAsModified=*/true); tester.invalidate(); tester.eval(/*keepGoing=*/true, top); - assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf).inOrder(); + assertThat(tester.invalidationReceiver.evaluated).containsExactly(leaf, top); } @Test diff --git a/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java b/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java index e93098d702b66a..465f32c373782c 100644 --- a/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java +++ b/src/test/java/com/google/devtools/build/skyframe/TrackingInvalidationReceiver.java @@ -51,11 +51,9 @@ public void enqueueing(SkyKey skyKey) { @Override public void evaluated(SkyKey skyKey, SkyValue value, EvaluationState state) { evaluated.add(skyKey); - switch (state) { - default: - dirty.remove(value); - deleted.remove(value); - break; + if (value != null) { + dirty.remove(value); + deleted.remove(value); } }