Allow ParallelEvaluator to not store errors alongside values if nodes recover from errors. In the case of a single keep_going build, with no subsequent nokeep_going builds, storing the errors is unnecessary.
--
MOS_MIGRATED_REVID=114355846
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 a370cdc..fd2f44b 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 @@
* {@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 @@
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 @@
}
}
+ /** 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 0f659bf..7a5f1d1 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 @@
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 c0a5629..7980ef5 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 @@
.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 108662a..94c9b81 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.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 @@
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 @@
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
