diff options
Diffstat (limited to 'src')
5 files changed, 113 insertions, 15 deletions
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 e5e8e6c322..ff885d8dbe 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluator.java @@ -139,6 +139,7 @@ public final class ParallelEvaluator implements Evaluator { private final EventHandler reporter; private final NestedSetVisitor<TaggedEvents> replayingNestedSetEventVisitor; private final boolean keepGoing; + private final boolean storeErrorsAlongsideValues; private final int threadCount; @Nullable private final ForkJoinPool forkJoinPool; @Nullable private final EvaluationProgressReceiver progressReceiver; @@ -164,6 +165,7 @@ public final class ParallelEvaluator implements Evaluator { this.inflightKeysReceiver = inflightKeysReceiver; this.reporter = Preconditions.checkNotNull(reporter); this.keepGoing = keepGoing; + this.storeErrorsAlongsideValues = true; this.threadCount = threadCount; this.progressReceiver = progressReceiver; this.dirtyKeyTracker = Preconditions.checkNotNull(dirtyKeyTracker); @@ -181,6 +183,7 @@ public final class ParallelEvaluator implements Evaluator { EmittedEventState emittedEventState, EventFilter storedEventFilter, boolean keepGoing, + boolean storeErrorsAlongsideValues, @Nullable EvaluationProgressReceiver progressReceiver, DirtyKeyTracker dirtyKeyTracker, Receiver<Collection<SkyKey>> inflightKeysReceiver, @@ -191,6 +194,8 @@ public final class ParallelEvaluator implements Evaluator { this.inflightKeysReceiver = inflightKeysReceiver; this.reporter = Preconditions.checkNotNull(reporter); this.keepGoing = keepGoing; + this.storeErrorsAlongsideValues = storeErrorsAlongsideValues; + Preconditions.checkState(storeErrorsAlongsideValues || keepGoing); this.threadCount = 0; this.progressReceiver = progressReceiver; this.dirtyKeyTracker = Preconditions.checkNotNull(dirtyKeyTracker); @@ -332,10 +337,16 @@ public final class ParallelEvaluator implements Evaluator { * <p>Child errors are remembered, if there are any and yet the parent recovered without * error, so that subsequent noKeepGoing evaluations can stop as soon as they encounter a * node whose (transitive) children had experienced an error, even if that (transitive) - * parent node had been able to recover from it during a keepGoing build. + * parent node had been able to recover from it during a keepGoing build. This behavior can be + * suppressed by setting {@link #storeErrorsAlongsideValues} to false, which will cause nodes + * with values to have no stored error info. This may be useful if this graph will only ever be + * used for keepGoing builds, since in that case storing errors from recovered nodes is + * pointless. */ private void finalizeErrorInfo() { - if (errorInfo == null && !childErrorInfos.isEmpty()) { + if (errorInfo == null + && (storeErrorsAlongsideValues || value == null) + && !childErrorInfos.isEmpty()) { errorInfo = ErrorInfo.fromChildErrors(skyKey, childErrorInfos); } } diff --git a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java index a370cdc6a2..fd2f44b362 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java +++ b/src/test/java/com/google/devtools/build/skyframe/ChainedFunction.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.skyframe; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.skyframe.GraphTester.ValueComputer; import com.google.devtools.build.skyframe.ParallelEvaluator.SkyFunctionEnvironment; @@ -26,7 +27,7 @@ import javax.annotation.Nullable; * {@link ValueComputer} that can be chained together with others of its type to synchronize the * order in which builders finish. */ -final class ChainedFunction implements SkyFunction { +public final class ChainedFunction implements SkyFunction { @Nullable private final SkyValue value; @Nullable private final CountDownLatch notifyStart; @Nullable private final CountDownLatch waitToFinish; @@ -34,9 +35,14 @@ final class ChainedFunction implements SkyFunction { private final boolean waitForException; private final Iterable<SkyKey> deps; - ChainedFunction(@Nullable CountDownLatch notifyStart, @Nullable CountDownLatch waitToFinish, - @Nullable CountDownLatch notifyFinish, boolean waitForException, - @Nullable SkyValue value, Iterable<SkyKey> deps) { + /** Do not use! Use {@link Builder} instead. */ + ChainedFunction( + @Nullable CountDownLatch notifyStart, + @Nullable CountDownLatch waitToFinish, + @Nullable CountDownLatch notifyFinish, + boolean waitForException, + @Nullable SkyValue value, + Iterable<SkyKey> deps) { this.notifyStart = notifyStart; this.waitToFinish = waitToFinish; this.notifyFinish = notifyFinish; @@ -80,6 +86,51 @@ final class ChainedFunction implements SkyFunction { } } + /** Builder for {@link ChainedFunction} objects. */ + public static class Builder { + @Nullable private SkyValue value; + @Nullable private CountDownLatch notifyStart; + @Nullable private CountDownLatch waitToFinish; + @Nullable private CountDownLatch notifyFinish; + private boolean waitForException; + private Iterable<SkyKey> deps = ImmutableList.of(); + + public Builder setValue(SkyValue value) { + this.value = value; + return this; + } + + public Builder setNotifyStart(CountDownLatch notifyStart) { + this.notifyStart = notifyStart; + return this; + } + + public Builder setWaitToFinish(CountDownLatch waitToFinish) { + this.waitToFinish = waitToFinish; + return this; + } + + public Builder setNotifyFinish(CountDownLatch notifyFinish) { + this.notifyFinish = notifyFinish; + return this; + } + + public Builder setWaitForException(boolean waitForException) { + this.waitForException = waitForException; + return this; + } + + public Builder setDeps(Iterable<SkyKey> deps) { + this.deps = Preconditions.checkNotNull(deps); + return this; + } + + public SkyFunction build() { + return new ChainedFunction( + notifyStart, waitToFinish, notifyFinish, waitForException, value, deps); + } + } + @Override public String extractTag(SkyKey skyKey) { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java index 0f659bfc40..7a5f1d1100 100644 --- a/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java +++ b/src/test/java/com/google/devtools/build/skyframe/ErrorInfoSubject.java @@ -43,4 +43,16 @@ public class ErrorInfoSubject extends Subject<ErrorInfoSubject, ErrorInfo> { fail("has root cause of exception " + key); } } + + public void isTransient() { + if (!getSubject().isTransient()) { + fail("is transient"); + } + } + + public void isNotTransient() { + if (getSubject().isTransient()) { + fail("is not transient"); + } + } } diff --git a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java index c0a5629aa8..7980ef5e95 100644 --- a/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java +++ b/src/test/java/com/google/devtools/build/skyframe/EvaluationResultSubject.java @@ -13,8 +13,10 @@ // limitations under the License. package com.google.devtools.build.skyframe; +import com.google.common.collect.ImmutableList; import com.google.common.truth.DefaultSubject; import com.google.common.truth.FailureStrategy; +import com.google.common.truth.IterableSubject; import com.google.common.truth.Subject; import com.google.common.truth.Truth; @@ -49,4 +51,9 @@ public class EvaluationResultSubject extends Subject<EvaluationResultSubject, Ev .named("Error entry for " + getDisplaySubject()); } + public IterableSubject hasDirectDepsInGraphThat(SkyKey parent) { + return Truth.assertThat( + getSubject().getWalkableGraph().getDirectDeps(ImmutableList.of(parent)).get(parent)) + .named("Direct deps for " + parent + " in " + getDisplaySubject()); + } } 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 108662aa97..94c9b819c3 100644 --- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java @@ -18,6 +18,8 @@ import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertContainsEvent; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount; import static com.google.devtools.build.lib.testutil.MoreAsserts.assertNoEvents; +import static com.google.devtools.build.skyframe.ErrorInfoSubjectFactory.assertThatErrorInfo; +import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult; import static com.google.devtools.build.skyframe.GraphTester.CONCATENATE; import static com.google.devtools.build.skyframe.GraphTester.COPY; import static com.google.devtools.build.skyframe.GraphTester.NODE_TYPE; @@ -1066,8 +1068,12 @@ public class MemoizingEvaluatorTest { assertThat(cycleInfo.getPathToCycle()).isEmpty(); } - @Test - public void parentOfCycleAndTransient() throws Exception { + /** + * {@link ParallelEvaluator} can be configured to not store errors alongside recovered values. + * In that case, transient errors that are recovered from do not make the parent transient. + */ + protected void parentOfCycleAndTransientInternal(boolean errorsStoredAlongsideValues) + throws Exception { initializeTester(); SkyKey cycleKey1 = GraphTester.toSkyKey("cycleKey1"); SkyKey cycleKey2 = GraphTester.toSkyKey("cycleKey2"); @@ -1083,23 +1089,34 @@ public class MemoizingEvaluatorTest { tester.getOrCreate(top).setBuilder(new ChainedFunction(topEvaluated, null, null, false, new StringValue("unused"), ImmutableList.of(mid, cycleKey1))); EvaluationResult<StringValue> evalResult = tester.eval(true, top); - assertTrue(evalResult.hasError()); + assertThatEvaluationResult(evalResult).hasError(); ErrorInfo errorInfo = evalResult.getError(top); assertThat(topEvaluated.getCount()).isEqualTo(1); - // The parent should be transitively transient, since it transitively depends on a transient - // error. - assertThat(errorInfo.isTransient()).isTrue(); + if (errorsStoredAlongsideValues) { + // The parent should be transitively transient, since it transitively depends on a transient + // error. + assertThat(errorInfo.isTransient()).isTrue(); + assertThat(errorInfo.getException()).hasMessage(NODE_TYPE.getName() + ":errorKey"); + assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey); + } else { + assertThatErrorInfo(errorInfo).isNotTransient(); + assertThatErrorInfo(errorInfo).hasExceptionThat().isNull(); + } assertWithMessage(errorInfo.toString()) .that(errorInfo.getCycleInfo()) .containsExactly( new CycleInfo(ImmutableList.of(top), ImmutableList.of(cycleKey1, cycleKey2))); - assertThat(errorInfo.getException()).hasMessage(NODE_TYPE.getName() + ":errorKey"); - assertThat(errorInfo.getRootCauseOfException()).isEqualTo(errorKey); // But the parent itself shouldn't have a direct dep on the special error transience node. - assertThat(evalResult.getWalkableGraph().getDirectDeps(ImmutableList.of(top)).get(top)) + assertThatEvaluationResult(evalResult) + .hasDirectDepsInGraphThat(top) .doesNotContain(ErrorTransienceValue.KEY); } + @Test + public void parentOfCycleAndTransient() throws Exception { + parentOfCycleAndTransientInternal(/*errorsStoredAlongsideValues=*/ true); + } + /** * Regression test: IllegalStateException in BuildingState.isReady(). The ParallelEvaluator used * to assume during cycle-checking that all values had been built as fully as possible -- that |