diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java index b70e4f23fe563e..b6a7fce159c11c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java @@ -34,7 +34,6 @@ import com.google.common.collect.MultimapBuilder; import com.google.common.flogger.GoogleLogger; import com.google.common.graph.EndpointPair; -import com.google.common.graph.GraphBuilder; import com.google.common.graph.ImmutableGraph; import com.google.common.graph.MutableGraph; import com.google.common.graph.Traverser; @@ -122,8 +121,7 @@ public RewindPlan getRewindPlan( // This graph tracks which Skyframe nodes must be restarted and the dependency relationships // between them. - MutableGraph rewindGraph = GraphBuilder.directed().allowsSelfLoops(false).build(); - rewindGraph.addNode(actionLookupData); + MutableGraph rewindGraph = Restart.newRewindGraphFor(actionLookupData); // SkyframeActionExecutor must re-execute the actions being restarted, so we must tell it to // evict its cached results for those actions. This collection tracks those actions (aside from @@ -178,8 +176,7 @@ public RewindPlan getRewindPlan( lostInputRecordsCount, lostInputRecordsThisAction.subList( 0, Math.min(lostInputRecordsCount, MAX_LOST_INPUTS_RECORDED)))); - return new RewindPlan( - Restart.selfAnd(ImmutableGraph.copyOf(rewindGraph)), additionalActionsToRestart.build()); + return new RewindPlan(Restart.of(rewindGraph), additionalActionsToRestart.build()); } /** 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 243c43dbaf393c..7346c2ac5c95c9 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.skyframe; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; @@ -862,18 +861,14 @@ protected void replay(ValueWithMetadata valueWithMetadata) { private void dirtyRewindGraphAndResetEntry(SkyKey key, NodeEntry entry, Restart restart) throws InterruptedException { ImmutableGraph rewindGraph = restart.rewindGraph(); - if (rewindGraph.nodes().isEmpty()) { - resetEntry(key, entry); - return; - } - checkArgument( + checkState( rewindGraph.nodes().contains(key), - "rewindGraph must contain the key for the failed evaluation if it's not empty. key: %s, " - + "rewindGraph: %s", + "Rewind graph missing evaluating key %s: %s", key, rewindGraph); - ImmutableList.Builder builder = ImmutableList.builder(); + ImmutableList.Builder builder = + ImmutableList.builderWithExpectedSize(rewindGraph.nodes().size() - 1); for (SkyKey k : Traverser.forGraph(rewindGraph).depthFirstPostOrder(key)) { if (!k.equals(key)) { builder.add(k); diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java index 2444fc1ea7860e..bd83350cab50b2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java @@ -13,11 +13,13 @@ // limitations under the License. package com.google.devtools.build.skyframe; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; -import com.google.common.base.Preconditions; +import com.google.common.graph.Graph; import com.google.common.graph.GraphBuilder; import com.google.common.graph.ImmutableGraph; +import com.google.common.graph.MutableGraph; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.events.ExtendedEventHandler; @@ -121,21 +123,36 @@ default Version getMaxTransitiveSourceVersionToInjectForNonHermeticFunction( * true. If restarting is not permitted, {@link #compute} should throw an appropriate {@link * SkyFunctionException}. */ - interface Restart extends SkyValue { - ImmutableGraph EMPTY_SKYKEY_GRAPH = - ImmutableGraph.copyOf(GraphBuilder.directed().allowsSelfLoops(false).build()); - - Restart SELF = () -> EMPTY_SKYKEY_GRAPH; - - static Restart selfAnd(ImmutableGraph rewindGraph) { - Preconditions.checkArgument( - rewindGraph.isDirected(), "rewindGraph undirected: %s", rewindGraph); - Preconditions.checkArgument( - !rewindGraph.allowsSelfLoops(), "rewindGraph allows self loops: %s", rewindGraph); - return () -> rewindGraph; + final class Restart implements SkyValue { + + /** + * Convenience method that creates a {@link MutableGraph} that fulfills the basic requirements + * of a {@link Restart}. + * + *

Additional edges may be added to the graph before passing to {@link #of}. + */ + public static MutableGraph newRewindGraphFor(SkyKey keyToRestart) { + MutableGraph rewindGraph = GraphBuilder.directed().allowsSelfLoops(false).build(); + rewindGraph.addNode(keyToRestart); + return rewindGraph; + } + + public static Restart of(Graph rewindGraph) { + checkArgument(rewindGraph.isDirected(), "Undirected: %s", rewindGraph); + checkArgument(!rewindGraph.allowsSelfLoops(), "Allows self loops: %s", rewindGraph); + checkArgument(!rewindGraph.nodes().isEmpty(), "Rewind graph must include key to restart"); + return new Restart(ImmutableGraph.copyOf(rewindGraph)); } - ImmutableGraph rewindGraph(); + private final ImmutableGraph rewindGraph; + + private Restart(ImmutableGraph rewindGraph) { + this.rewindGraph = rewindGraph; + } + + ImmutableGraph rewindGraph() { + return rewindGraph; + } } /**