diff options
Diffstat (limited to 'src')
5 files changed, 113 insertions, 29 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 0b371780ca..cf49cc96c8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -76,6 +76,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.BuildDriver; import com.google.devtools.build.skyframe.Differencer; +import com.google.devtools.build.skyframe.GraphInconsistencyReceiver; import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator; import com.google.devtools.build.skyframe.Injectable; import com.google.devtools.build.skyframe.MemoizingEvaluator.EvaluatorSupplier; @@ -167,6 +168,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { buildFilesByPriority, actionOnIOExceptionReadingBuildFile, /*shouldUnblockCpuWorkWhenFetchingDeps=*/ false, + GraphInconsistencyReceiver.THROWING, defaultBuildOptions, new PackageProgressReceiver(), mutableArtifactFactorySupplier, 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 d21b239197..c7c38478bc 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 @@ -201,6 +201,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { private final FileSystem fileSystem; private final BlazeDirectories directories; protected final ExternalFilesHelper externalFilesHelper; + private final GraphInconsistencyReceiver graphInconsistencyReceiver; @Nullable protected OutputService outputService; // TODO(bazel-team): Figure out how to handle value builders that block internally. Blocking @@ -339,6 +340,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { List<BuildFileName> buildFilesByPriority, ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile, boolean shouldUnblockCpuWorkWhenFetchingDeps, + GraphInconsistencyReceiver graphInconsistencyReceiver, BuildOptions defaultBuildOptions, @Nullable PackageProgressReceiver packageProgress, MutableArtifactFactorySupplier artifactResolverSupplier, @@ -348,6 +350,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { this.evaluatorSupplier = evaluatorSupplier; this.pkgFactory = pkgFactory; this.shouldUnblockCpuWorkWhenFetchingDeps = shouldUnblockCpuWorkWhenFetchingDeps; + this.graphInconsistencyReceiver = graphInconsistencyReceiver; this.pkgFactory.setSyscalls(syscalls); this.workspaceStatusActionFactory = workspaceStatusActionFactory; this.packageManager = new SkyframePackageManager( @@ -655,7 +658,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { skyFunctions, evaluatorDiffer(), progressReceiver, - GraphInconsistencyReceiver.THROWING, + graphInconsistencyReceiver, emittedEventState, tracksStateForIncrementality()); buildDriver = getBuildDriver(); diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java index bc6e01f346..5cb810f699 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.util.GroupedList.GroupedListHelper; import com.google.devtools.build.skyframe.EvaluationProgressReceiver.EvaluationState; import com.google.devtools.build.skyframe.EvaluationProgressReceiver.NodeState; +import com.google.devtools.build.skyframe.GraphInconsistencyReceiver.Inconsistency; import com.google.devtools.build.skyframe.MemoizingEvaluator.EmittedEventState; import com.google.devtools.build.skyframe.NodeEntry.DependencyState; import com.google.devtools.build.skyframe.NodeEntry.DirtyState; @@ -274,22 +275,7 @@ public abstract class AbstractParallelEvaluator { unknownStatusDeps); continue; } - Map<SkyKey, ? extends NodeEntry> oldChildren = - graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, unknownStatusDeps); - Preconditions.checkState( - oldChildren.size() == unknownStatusDeps.size(), - "Not all old children were present: %s %s %s %s", - skyKey, - state, - unknownStatusDeps, - oldChildren); - for (Map.Entry<SkyKey, ? extends NodeEntry> e : oldChildren.entrySet()) { - SkyKey directDep = e.getKey(); - NodeEntry directDepEntry = e.getValue(); - // TODO(bazel-team): If this signals the current node, consider falling through to the - // VERIFIED_CLEAN case below directly, without scheduling a new Evaluate(). - enqueueChild(skyKey, state, directDep, directDepEntry, /*depAlreadyExists=*/ true); - } + handleKnownChildrenForDirtyNode(unknownStatusDeps, state); return DirtyOutcome.ALREADY_PROCESSED; } switch (state.getDirtyState()) { @@ -320,6 +306,39 @@ public abstract class AbstractParallelEvaluator { } } + private void handleKnownChildrenForDirtyNode(Collection<SkyKey> knownChildren, NodeEntry state) + throws InterruptedException { + Map<SkyKey, ? extends NodeEntry> oldChildren = + graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, knownChildren); + if (oldChildren.size() != knownChildren.size()) { + GraphInconsistencyReceiver inconsistencyReceiver = + evaluatorContext.getGraphInconsistencyReceiver(); + Set<SkyKey> missingChildren = + Sets.difference(ImmutableSet.copyOf(knownChildren), oldChildren.keySet()); + for (SkyKey missingChild : missingChildren) { + inconsistencyReceiver.noteInconsistencyAndMaybeThrow( + skyKey, missingChild, Inconsistency.CHILD_MISSING_FOR_DIRTY_NODE); + } + Map<SkyKey, ? extends NodeEntry> recreatedEntries = + graph.createIfAbsentBatch(skyKey, Reason.ENQUEUING_CHILD, missingChildren); + for (Map.Entry<SkyKey, ? extends NodeEntry> recreatedEntry : recreatedEntries.entrySet()) { + enqueueChild( + skyKey, + state, + recreatedEntry.getKey(), + recreatedEntry.getValue(), + /*depAlreadyExists=*/ false); + } + } + for (Map.Entry<SkyKey, ? extends NodeEntry> e : oldChildren.entrySet()) { + SkyKey directDep = e.getKey(); + NodeEntry directDepEntry = e.getValue(); + // TODO(bazel-team): If this signals the current node and makes it ready, consider + // evaluating it in this thread instead of scheduling a new evaluation. + enqueueChild(skyKey, state, directDep, directDepEntry, /*depAlreadyExists=*/ true); + } + } + @Override public void run() { try { @@ -561,15 +580,7 @@ public abstract class AbstractParallelEvaluator { newDepsThatWerentInTheLastEvaluationNodes = graph.createIfAbsentBatchAsync( skyKey, Reason.RDEP_ADDITION, newDepsThatWerentInTheLastEvaluation); - - for (Map.Entry<SkyKey, ? extends NodeEntry> e : - graph - .getBatch(skyKey, Reason.ENQUEUING_CHILD, newDepsThatWereInTheLastEvaluation) - .entrySet()) { - SkyKey newDirectDep = e.getKey(); - NodeEntry newDirectDepEntry = e.getValue(); - enqueueChild(skyKey, state, newDirectDep, newDirectDepEntry, /*depAlreadyExists=*/ true); - } + handleKnownChildrenForDirtyNode(newDepsThatWereInTheLastEvaluation, state); for (Map.Entry<SkyKey, ? extends NodeEntry> e : newDepsThatWerentInTheLastEvaluationNodes.get().entrySet()) { @@ -616,8 +627,7 @@ public abstract class AbstractParallelEvaluator { } evaluatorContext .getGraphInconsistencyReceiver() - .noteInconsistencyAndMaybeThrow( - key, /*otherKey=*/ null, GraphInconsistencyReceiver.Inconsistency.RESET_REQUESTED); + .noteInconsistencyAndMaybeThrow(key, /*otherKey=*/ null, Inconsistency.RESET_REQUESTED); entry.resetForRestartFromScratch(); // TODO(mschaller): rdeps of children have to be handled here. If the graph does not keep edges, // nothing has to be done, since there are no reverse deps to keep consistent. If the graph diff --git a/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java index bba9264dac..6aa161ab40 100644 --- a/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java +++ b/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java @@ -29,7 +29,8 @@ public interface GraphInconsistencyReceiver { /** The type of inconsistency detected. */ enum Inconsistency { - RESET_REQUESTED + RESET_REQUESTED, + CHILD_MISSING_FOR_DIRTY_NODE } /** A {@link GraphInconsistencyReceiver} that crashes on any inconsistency. */ 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 807563a1eb..ef7ada37b2 100644 --- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java @@ -2424,6 +2424,74 @@ public class MemoizingEvaluatorTest { RunResetNodeOnRequestWithDepsMode.NO_KEEP_EDGES_SO_NO_REEVALUATION); } + private void missingDirtyChild(boolean sameGroup) throws Exception { + SkyKey topKey = GraphTester.skyKey("top"); + SkyKey missingChild = GraphTester.skyKey("missing"); + AtomicInteger numInconsistencyCalls = new AtomicInteger(0); + tester.setGraphInconsistencyReceiver( + (key, otherKey, inconsistency) -> { + Preconditions.checkState(missingChild.equals(otherKey), otherKey); + Preconditions.checkState( + inconsistency + == GraphInconsistencyReceiver.Inconsistency.CHILD_MISSING_FOR_DIRTY_NODE, + inconsistency); + Preconditions.checkState(topKey.equals(key), key); + numInconsistencyCalls.incrementAndGet(); + }); + tester.initialize(/*keepEdges=*/ true); + tester.getOrCreate(missingChild).setConstantValue(new StringValue("will go missing")); + SkyKey presentChild = GraphTester.skyKey("present"); + tester.getOrCreate(presentChild).setConstantValue(new StringValue("present")); + StringValue topValue = new StringValue("top"); + tester + .getOrCreate(topKey) + .setBuilder( + new SkyFunction() { + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { + if (sameGroup) { + env.getValues(ImmutableSet.of(presentChild, missingChild)); + } else { + env.getValue(presentChild); + if (env.valuesMissing()) { + return null; + } + env.getValue(missingChild); + } + return env.valuesMissing() ? null : topValue; + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } + }); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, topKey)).isEqualTo(topValue); + deleteKeyFromGraph(missingChild); + tester + .getOrCreate(presentChild, /*markAsModified=*/ true) + .setConstantValue(new StringValue("changed")); + tester.invalidate(); + assertThat(tester.evalAndGet(/*keepGoing=*/ false, topKey)).isEqualTo(topValue); + assertThat(numInconsistencyCalls.get()).isEqualTo(1); + } + + protected void deleteKeyFromGraph(SkyKey key) { + ((InMemoryMemoizingEvaluator) tester.evaluator).getGraphForTesting().remove(key); + } + + @Test + public void missingDirtyChild_SameGroup() throws Exception { + missingDirtyChild(/*sameGroup=*/ true); + } + + @Test + public void missingDirtyChild_DifferentGroup() throws Exception { + missingDirtyChild(/*sameGroup=*/ false); + } + /** * The same dep is requested in two groups, but its value determines what the other dep in the * second group is. When it changes, the other dep in the second group should not be requested. |