diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java index e9c38fa1529eb9..7e6e7e6c403f4d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java @@ -163,8 +163,8 @@ public void dependOnFuture(ListenableFuture future) { } @Override - public boolean restartPermitted() { - return delegate.restartPermitted(); + public boolean resetPermitted() { + return delegate.resetPermitted(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 3944e9f9c8470b..55de154d4cea11 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -319,7 +319,7 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) directories.getRelativeOutputPath(), checkedInputs.actionInputMap, action.getOutputs(), - env.restartPermitted()); + env.resetPermitted()); } } @@ -426,10 +426,10 @@ private boolean declareDepsOnLostDiscoveredInputsIfAny(Environment env, Action a } /** - * Clean up state associated with the current action execution attempt and return a {@link - * Restart} value which rewinds the actions that generate the lost inputs. + * Cleans up state associated with the current action execution attempt and returns a {@link + * SkyFunction.Reset} value which rewinds the actions that generate the lost inputs. */ - private SkyFunction.Restart handleLostInputs( + private SkyFunction.Reset handleLostInputs( LostInputsActionExecutionException e, ActionLookupData actionLookupData, Action action, @@ -480,10 +480,11 @@ private SkyFunction.Restart handleLostInputs( try { rewindPlan = actionRewindStrategy.getRewindPlan( - action, actionLookupData, failedActionDeps, e, inputDepOwners, env); + actionLookupData, action, failedActionDeps, e, inputDepOwners, env); } catch (ActionExecutionException rewindingFailedException) { // This ensures coalesced shared actions aren't orphaned. - skyframeActionExecutor.resetRewindingAction(actionLookupData, action, lostDiscoveredInputs); + skyframeActionExecutor.prepareForRewinding( + actionLookupData, action, lostDiscoveredInputs, /* depsToRewind= */ ImmutableList.of()); throw new ActionExecutionFunctionException( new AlreadyReportedActionExecutionException( skyframeActionExecutor.processAndGetExceptionToThrow( @@ -499,11 +500,9 @@ private SkyFunction.Restart handleLostInputs( env.getListener() .post(new ActionRewoundEvent(actionStartTimeNanos, BlazeClock.nanoTime(), action)); } - skyframeActionExecutor.resetRewindingAction(actionLookupData, action, lostDiscoveredInputs); - for (Action actionToRestart : rewindPlan.getAdditionalActionsToRestart()) { - skyframeActionExecutor.resetPreviouslyCompletedAction(actionLookupData, actionToRestart); - } - return rewindPlan.getNodesToRestart(); + skyframeActionExecutor.prepareForRewinding( + actionLookupData, action, lostDiscoveredInputs, rewindPlan.getDepsToRewind()); + return rewindPlan.getReset(); } finally { if (rewindPlan == null && e.isActionStartedEventAlreadyEmitted()) { // Rewinding was unsuccessful. SkyframeActionExecutor's ActionRunner didn't emit an diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java index 7a1339d43c01eb..7844e282540aa1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java @@ -129,8 +129,8 @@ public void dependOnFuture(ListenableFuture future) { } @Override - public boolean restartPermitted() { - return delegate.restartPermitted(); + public boolean resetPermitted() { + return delegate.resetPermitted(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index adb505af7338f8..114f9a09c7f82f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -425,16 +425,43 @@ boolean shouldEmitProgressEvents(Action action) { new OwnerlessArtifactWrapper(action.getPrimaryOutput())); } - void resetPreviouslyCompletedAction(ActionLookupData actionLookupData, Action action) { + /** + * Called to prepare action execution states for rewinding after {@code failedAction} observed + * lost inputs. + */ + void prepareForRewinding( + ActionLookupData failedKey, + Action failedAction, + ImmutableList lostDiscoveredInputs, + ImmutableList depsToRewind) { + var ownerlessArtifactWrapper = new OwnerlessArtifactWrapper(failedAction.getPrimaryOutput()); + ActionExecutionState state = buildActionMap.get(ownerlessArtifactWrapper); + if (state != null) { + // If an action failed from lost inputs during input discovery then it won't have a state to + // obsolete. + state.obsolete(failedKey, buildActionMap, ownerlessArtifactWrapper); + } + if (!lostDiscoveredInputs.isEmpty()) { + lostDiscoveredInputsMap.put(ownerlessArtifactWrapper, lostDiscoveredInputs); + } + if (!actionFileSystemType().inMemoryFileSystem()) { + outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(failedAction.getOutputs()); + } + for (Action dep : depsToRewind) { + prepareDepForRewinding(failedKey, dep); + } + } + + private void prepareDepForRewinding(ActionLookupData failedKey, Action dep) { OwnerlessArtifactWrapper ownerlessArtifactWrapper = - new OwnerlessArtifactWrapper(action.getPrimaryOutput()); + new OwnerlessArtifactWrapper(dep.getPrimaryOutput()); ActionExecutionState actionExecutionState = buildActionMap.get(ownerlessArtifactWrapper); if (actionExecutionState != null) { - actionExecutionState.obsolete(actionLookupData, buildActionMap, ownerlessArtifactWrapper); + actionExecutionState.obsolete(failedKey, buildActionMap, ownerlessArtifactWrapper); } completedAndResetActions.add(ownerlessArtifactWrapper); if (!actionFileSystemType().inMemoryFileSystem()) { - outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(action.getOutputs()); + outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(dep.getOutputs()); } } @@ -443,26 +470,6 @@ ImmutableList getLostDiscoveredInputs(Action action) { return lostDiscoveredInputsMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())); } - void resetRewindingAction( - ActionLookupData actionLookupData, - Action action, - ImmutableList lostDiscoveredInputs) { - OwnerlessArtifactWrapper ownerlessArtifactWrapper = - new OwnerlessArtifactWrapper(action.getPrimaryOutput()); - ActionExecutionState state = buildActionMap.get(ownerlessArtifactWrapper); - if (state != null) { - // If an action failed from lost inputs during input discovery then it won't have a state to - // obsolete. - state.obsolete(actionLookupData, buildActionMap, ownerlessArtifactWrapper); - } - if (!lostDiscoveredInputs.isEmpty()) { - lostDiscoveredInputsMap.put(ownerlessArtifactWrapper, lostDiscoveredInputs); - } - if (!actionFileSystemType().inMemoryFileSystem()) { - outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(action.getOutputs()); - } - } - void noteActionEvaluationStarted(ActionLookupData actionLookupData, Action action) { if (completionReceiver != null) { completionReceiver.noteActionEvaluationStarted(actionLookupData, action); @@ -586,7 +593,7 @@ private ActionExecutionContext getContext( actionInputPrefetcher, actionKeyContext, outputMetadataStore, - env.restartPermitted(), + env.resetPermitted(), lostInputsCheck(actionFileSystem, action, outputService), fileOutErr, selectEventHandler(emitProgressEvents), @@ -824,7 +831,7 @@ NestedSet discoverInputs( actionInputPrefetcher, actionKeyContext, outputMetadataStore, - env.restartPermitted(), + env.resetPermitted(), lostInputsCheck(actionFileSystem, action, outputService), fileOutErr, eventHandler, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java index 38d77e486aca74..79e78fb8408fe0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java @@ -157,8 +157,8 @@ public void dependOnFuture(ListenableFuture future) { } @Override - public boolean restartPermitted() { - return delegate.restartPermitted(); + public boolean resetPermitted() { + return delegate.resetPermitted(); } @Override 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 b6a7fce159c11c..59a8214e7e105a 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 @@ -57,7 +57,7 @@ import com.google.devtools.build.lib.skyframe.proto.ActionRewind.ActionRewindEvent; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.skyframe.SkyFunction.Environment; -import com.google.devtools.build.skyframe.SkyFunction.Restart; +import com.google.devtools.build.skyframe.SkyFunction.Reset; import com.google.devtools.build.skyframe.SkyKey; import java.util.ArrayDeque; import java.util.ArrayList; @@ -71,14 +71,14 @@ /** * Given an action that failed to execute because of lost inputs which were generated by other - * actions, this finds the actions which generated them and the set of Skyframe nodes which must be - * restarted in order to recreate the lost inputs. + * actions, finds the actions which generated them and the set of Skyframe nodes which must be + * rewound in order to recreate the lost inputs. */ public final class ActionRewindStrategy { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - @VisibleForTesting public static final int MAX_REPEATED_LOST_INPUTS = 20; - @VisibleForTesting public static final int MAX_ACTION_REWIND_EVENTS = 5; + @VisibleForTesting static final int MAX_REPEATED_LOST_INPUTS = 20; + @VisibleForTesting static final int MAX_ACTION_REWIND_EVENTS = 5; private static final int MAX_LOST_INPUTS_RECORDED = 5; // Note that these references are mutated only outside of Skyframe evaluations, and accessed only @@ -91,7 +91,7 @@ public final class ActionRewindStrategy { * Returns a {@link RewindPlan} specifying: * *
    - *
  1. the Skyframe nodes to restart to recreate the lost inputs specified by {@code + *
  2. the Skyframe nodes to rewind to recreate the lost inputs specified by {@code * lostInputsException} *
  3. the actions whose execution state (in {@link * com.google.devtools.build.lib.skyframe.SkyframeActionExecutor}) must be reset (aside from @@ -99,34 +99,33 @@ public final class ActionRewindStrategy { *
* *

Note that all Skyframe nodes between the currently executing (failed) action's node and the - * nodes corresponding to the actions which create the lost inputs, inclusive, must be restarted. - * This ensures that reevaluating the current node will also reevaluate the nodes that will - * recreate the lost inputs. + * nodes corresponding to the actions which create the lost inputs, inclusive, must be included in + * reevaluate the nodes that will recreate the lost inputs. * * @throws ActionExecutionException if any lost inputs have been seen by this action as lost * before, or if any lost inputs are not the outputs of previously executed actions */ public RewindPlan getRewindPlan( + ActionLookupData failedKey, Action failedAction, - ActionLookupData actionLookupData, Set failedActionDeps, LostInputsActionExecutionException lostInputsException, ActionInputDepOwners inputDepOwners, Environment env) throws ActionExecutionException, InterruptedException { ImmutableList lostInputRecordsThisAction = - checkIfActionLostInputTooManyTimes(actionLookupData, failedAction, lostInputsException); + checkIfActionLostInputTooManyTimes(failedKey, failedAction, lostInputsException); ImmutableList lostInputs = lostInputsException.getLostInputs().values().asList(); - // This graph tracks which Skyframe nodes must be restarted and the dependency relationships + // This graph tracks which Skyframe nodes must be rewound and the dependency relationships // between them. - MutableGraph rewindGraph = Restart.newRewindGraphFor(actionLookupData); + MutableGraph rewindGraph = Reset.newRewindGraphFor(failedKey); - // 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 - // failedAction, which the caller of getRewindPlan already knows must be restarted). - ImmutableList.Builder additionalActionsToRestart = ImmutableList.builder(); + // SkyframeActionExecutor must re-execute the actions being rewound, so we must tell it to evict + // its cached results for those actions. This collection tracks those actions (aside from + // failedAction, which the caller of getRewindPlan already knows must be reset). + ImmutableList.Builder depsToRewind = ImmutableList.builder(); // With NSOS, not all input artifacts' keys are direct deps of the action. This maps input // artifacts to their containing direct dep ArtifactNestedSetKey(s). @@ -149,23 +148,22 @@ public RewindPlan getRewindPlan( Map actionMap = getActionsForLostArtifact(lostArtifact, env); if (actionMap == null) { // Some deps of the artifact are not done. Another rewind must be in-flight, and there is no - // need to restart the shared deps twice. + // need to rewind the shared deps twice. continue; } ImmutableList newlyVisitedActions = addArtifactDepsAndGetNewlyVisitedActions(rewindGraph, lostArtifact, actionMap); for (ArtifactNestedSetKey nestedSetKey : nestedSetKeys.get(lostArtifact)) { - addNestedSetToRewindGraph( - rewindGraph, actionLookupData, lostArtifact, artifactKey, nestedSetKey); + addNestedSetToRewindGraph(rewindGraph, failedKey, lostArtifact, artifactKey, nestedSetKey); } - // Note that artifactKey must be restarted. We do this after + // Note that artifactKey must be rewound. We do this after // addArtifactDepsAndGetNewlyVisitedActions so that it can track if actions are already known // to be in the graph. With NSOS, it is possible that artifactKey is not actually a direct dep // of the action, but this edge is benign since it's always a transitive dep. - rewindGraph.putEdge(actionLookupData, artifactKey); - additionalActionsToRestart.addAll(actions(newlyVisitedActions)); - checkActions(newlyVisitedActions, env, rewindGraph, additionalActionsToRestart); + rewindGraph.putEdge(failedKey, artifactKey); + depsToRewind.addAll(actions(newlyVisitedActions)); + checkActions(newlyVisitedActions, env, rewindGraph, depsToRewind); } int lostInputRecordsCount = lostInputRecordsThisAction.size(); @@ -176,7 +174,7 @@ public RewindPlan getRewindPlan( lostInputRecordsCount, lostInputRecordsThisAction.subList( 0, Math.min(lostInputRecordsCount, MAX_LOST_INPUTS_RECORDED)))); - return new RewindPlan(Restart.of(rewindGraph), additionalActionsToRestart.build()); + return new RewindPlan(Reset.of(rewindGraph), depsToRewind.build()); } /** @@ -201,7 +199,7 @@ MAX_ACTION_REWIND_EVENTS, comparing(RewindPlanStats::invalidatedNodesCount))) /** Returns all lost input records that will cause the failed action to rewind. */ private ImmutableList checkIfActionLostInputTooManyTimes( - ActionLookupData actionLookupData, + ActionLookupData failedKey, Action failedAction, LostInputsActionExecutionException lostInputsException) throws ActionExecutionException { @@ -223,7 +221,7 @@ private ImmutableList checkIfActionLostInputTooManyTimes( // the same input is repeatedly lost. String digest = entry.getKey(); LostInputRecord lostInputRecord = - LostInputRecord.create(actionLookupData, digest, entry.getValue().getExecPathString()); + LostInputRecord.create(failedKey, digest, entry.getValue().getExecPathString()); lostInputRecordsThisAction.add(lostInputRecord); int priorLosses = lostInputRecords.add(lostInputRecord, /*occurrences=*/ 1); if (MAX_REPEATED_LOST_INPUTS <= priorLosses) { @@ -343,13 +341,13 @@ private static void checkDerived( /** * Looks at each action in {@code actionsToCheck} and determines whether additional artifacts, * actions, and (in the case of {@link SkyframeAwareAction}s) other Skyframe nodes need to be - * restarted. If this finds more actions to restart, those actions are recursively checked too. + * rewound. If this finds more actions to rewind, those actions are recursively checked too. */ private static void checkActions( ImmutableList actionsToCheck, Environment env, MutableGraph rewindGraph, - ImmutableList.Builder additionalActionsToRestart) + ImmutableList.Builder depsToRewind) throws InterruptedException { ArrayDeque uncheckedActions = new ArrayDeque<>(actionsToCheck); @@ -361,7 +359,7 @@ private static void checkActions( ArrayList newlyDiscoveredActions = new ArrayList<>(); if (action instanceof SkyframeAwareAction) { - // This action depends on more than just its input artifact values. We need to also restart + // This action depends on more than just its input artifact values. We need to also rewind // the Skyframe subgraph it depends on, up to and including any artifacts, which may // aggregate multiple actions. addSkyframeAwareDepsAndGetNewlyVisitedArtifactsAndActions( @@ -373,7 +371,7 @@ private static void checkActions( } if (action.mayInsensitivelyPropagateInputs()) { - // Restarting this action won't recreate the missing input. We need to also restart this + // Rewinding this action won't recreate the missing input. We need to also rewind this // action's non-source inputs and the actions which created those inputs. addPropagatingActionDepsAndGetNewlyVisitedArtifactsAndActions( rewindGraph, actionKey, action, artifactsToCheck, newlyDiscoveredActions); @@ -383,7 +381,7 @@ private static void checkActions( Action additionalAction = checkNotNull( ActionUtils.getActionForLookupData(env, actionLookupData), actionLookupData); - additionalActionsToRestart.add(additionalAction); + depsToRewind.add(additionalAction); uncheckedActions.add(ActionAndLookupData.create(actionLookupData, additionalAction)); } for (DerivedArtifact artifact : artifactsToCheck) { @@ -393,7 +391,7 @@ private static void checkActions( } ImmutableList newlyVisitedActions = addArtifactDepsAndGetNewlyVisitedActions(rewindGraph, artifact, actionMap); - additionalActionsToRestart.addAll(actions(newlyVisitedActions)); + depsToRewind.addAll(actions(newlyVisitedActions)); uncheckedActions.addAll(newlyVisitedActions); } } @@ -448,9 +446,9 @@ private static void addPropagatingActionDepsAndGetNewlyVisitedArtifactsAndAction continue; } SkyKey artifactKey = Artifact.key(input); - // Restarting all derived inputs of propagating actions is overkill. Preferably, we'd want - // to only restart the inputs which correspond to the known lost outputs. The information - // to do this is probably present in the data available to #getRewindPlan. + // Rewinding all derived inputs of propagating actions is overkill. Preferably, we'd want to + // only rewind the inputs which correspond to the known lost outputs. The information to do + // this is probably present in the data available to #getRewindPlan. // // Rewinding is expected to be rare, so refining this may not be necessary. boolean newlyVisited = rewindGraph.addNode(artifactKey); @@ -617,24 +615,24 @@ private static ActionExecutionException createActionExecutionException( } /** - * Specifies the Skyframe nodes and actions that need to be restarted in order to recreate lost - * inputs. + * Wraps a {@link Reset} and a list of actions that need to be reported to {@link + * com.google.devtools.build.lib.skyframe.SkyframeActionExecutor} because they will be rewound. */ public static final class RewindPlan { - private final Restart nodesToRestart; - private final ImmutableList additionalActionsToRestart; + private final Reset reset; + private final ImmutableList depsToRewind; - private RewindPlan(Restart nodesToRestart, ImmutableList additionalActionsToRestart) { - this.nodesToRestart = nodesToRestart; - this.additionalActionsToRestart = additionalActionsToRestart; + private RewindPlan(Reset reset, ImmutableList depsToRewind) { + this.reset = reset; + this.depsToRewind = depsToRewind; } - public Restart getNodesToRestart() { - return nodesToRestart; + public Reset getReset() { + return reset; } - public ImmutableList getAdditionalActionsToRestart() { - return additionalActionsToRestart; + public ImmutableList getDepsToRewind() { + return depsToRewind; } } @@ -671,16 +669,16 @@ static RewindPlanStats create( @AutoValue abstract static class LostInputRecord { - abstract ActionLookupData failedActionLookupData(); + abstract ActionLookupData failedKey(); abstract String lostInputDigest(); abstract String lostInputPath(); static LostInputRecord create( - ActionLookupData failedActionLookupData, String lostInputDigest, String lostInputPath) { + ActionLookupData failedKey, String lostInputDigest, String lostInputPath) { return new AutoValue_ActionRewindStrategy_LostInputRecord( - failedActionLookupData, lostInputDigest, lostInputPath); + failedKey, lostInputDigest, lostInputPath); } } @@ -724,24 +722,24 @@ private static Multimap expandNestedSetKeys(Set< } /** - * Adds a skyframe dependency chain from {@code failedAction} to {@code lostArtifactKey} to the + * Adds a skyframe dependency chain from {@code failedKey} to {@code lostArtifactKey} to the * rewind graph. * - *

Each edge along the path from {@code failedAction} to {@code lostArtifactKey} is added to - * the rewind graph. This necessarily includes the initial edge {@code (failedAction, directDep)}. + *

Each edge along the path from {@code failedKey} to {@code lostArtifactKey} is added to the + * rewind graph. This necessarily includes the initial edge {@code (failedKey, directDep)}. * *

Although {@code lostArtifact} may be reachable via multiple distinct paths, it is only - * necessary to rewind one such path to ensure successful completion of {@code failedAction}. - * Other failing actions that depend on {@code lostArtifact} via a different path may initiate - * their own rewind strategy. + * necessary to rewind one such path to ensure successful completion of {@code failedKey}. Other + * failing actions that depend on {@code lostArtifact} via a different path may initiate their own + * rewind strategy. */ private static void addNestedSetToRewindGraph( MutableGraph rewindGraph, - ActionLookupData failedAction, + ActionLookupData failedKey, DerivedArtifact lostArtifact, SkyKey lostArtifactKey, ArtifactNestedSetKey directDep) { - SkyKey current = failedAction; + SkyKey current = failedKey; for (ArtifactNestedSetKey key : directDep.findPathToArtifact(lostArtifact)) { rewindGraph.putEdge(current, key); current = key; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java index 8a331551a6a355..f205299c31d255 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/RewindableGraphInconsistencyReceiver.java @@ -169,7 +169,7 @@ private boolean noteSelfInconsistency(Inconsistency inconsistency) { } @Override - public boolean restartPermitted() { + public boolean resetPermitted() { return true; } 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 a01fe9119878f4..2e73d679071b7c 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -44,7 +44,7 @@ import com.google.devtools.build.skyframe.NodeEntry.DirtyType; import com.google.devtools.build.skyframe.QueryableGraph.Reason; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; -import com.google.devtools.build.skyframe.SkyFunction.Restart; +import com.google.devtools.build.skyframe.SkyFunction.Reset; import com.google.devtools.build.skyframe.SkyFunctionEnvironment.UndonePreviouslyRequestedDeps; import com.google.devtools.build.skyframe.SkyFunctionException.ReifiedSkyFunctionException; import com.google.devtools.build.skyframe.proto.GraphInconsistency.Inconsistency; @@ -554,13 +554,13 @@ public void run() { env.doneBuilding(); } - if (value instanceof Restart) { + if (value instanceof Reset) { if (nodeEntry.hasUnsignaledDeps()) { // This is a partial reevaluation. It is not safe to reset the node because a dep may // be racing to signal it. return; } - dirtyRewindGraphAndResetEntry(skyKey, nodeEntry, (Restart) value); + dirtyRewindGraphAndResetEntry(skyKey, nodeEntry, (Reset) value); stateCache.invalidate(skyKey); cancelExternalDeps(env); evaluatorContext.getVisitor().enqueueEvaluation(skyKey, null); @@ -862,7 +862,7 @@ protected void replay(ValueWithMetadata valueWithMetadata) { // // When FN next evaluates, it requests R1, and because R1 is done, R2 is not scheduled for // evaluation, contrary to FN's expectations. - private void dirtyRewindGraphAndResetEntry(SkyKey key, NodeEntry entry, Restart restart) + private void dirtyRewindGraphAndResetEntry(SkyKey key, NodeEntry entry, Reset restart) throws InterruptedException { ImmutableGraph rewindGraph = restart.rewindGraph(); checkState( @@ -911,7 +911,7 @@ private void resetEntry(SkyKey key, NodeEntry entry) { evaluatorContext .getGraphInconsistencyReceiver() .noteInconsistencyAndMaybeThrow(key, /* otherKeys= */ null, Inconsistency.RESET_REQUESTED); - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); } void propagateEvaluatorContextCrashIfAny() { diff --git a/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java index 56eadbcc61e84c..bd5bda4b9e9bf6 100644 --- a/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/DelegatingNodeEntry.java @@ -139,8 +139,8 @@ public void removeUnfinishedDeps(Set unfinishedDeps) { } @Override - public void resetForRestartFromScratch() { - getDelegate().resetForRestartFromScratch(); + public void resetEvaluationFromScratch() { + getDelegate().resetEvaluationFromScratch(); } @Override 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 a8f9f23c22d9a4..92561e034d7145 100644 --- a/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java +++ b/src/main/java/com/google/devtools/build/skyframe/GraphInconsistencyReceiver.java @@ -38,7 +38,7 @@ void noteInconsistencyAndMaybeThrow( "Unexpected inconsistency: " + key + ", " + otherKey + ", " + inconsistency); }; - default boolean restartPermitted() { + default boolean resetPermitted() { return false; } diff --git a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java index d382316e5ee3ee..331ed79d822867 100644 --- a/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntry.java @@ -402,7 +402,7 @@ final synchronized int getNumTemporaryDirectDeps() { } @Override - public final synchronized void resetForRestartFromScratch() { + public final synchronized void resetEvaluationFromScratch() { checkState(!hasUnsignaledDeps(), this); ImmutableSet resetDeps = @@ -481,7 +481,7 @@ protected MoreObjects.ToStringHelper getStringHelper() { } /** - * Used to track already registered deps when there is a {@linkplain #resetForRestartFromScratch + * Used to track already registered deps when there is a {@linkplain #resetEvaluationFromScratch * reset} on a node's initial build. */ private static final class ResetInitialBuildingState extends InitialBuildingState { @@ -505,7 +505,7 @@ protected MoreObjects.ToStringHelper getStringHelper() { } /** - * Used to track already registered deps when there is a {@linkplain #resetForRestartFromScratch + * Used to track already registered deps when there is a {@linkplain #resetEvaluationFromScratch * reset} on a node's incremental build. */ private static final class ResetIncrementalBuildingState extends IncrementalBuildingState { diff --git a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java index 41e02bcc673b91..f8c197e95c18ea 100644 --- a/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/NodeEntry.java @@ -16,6 +16,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.skyframe.SkyFunction.Reset; import java.util.Collection; import java.util.List; import java.util.Set; @@ -522,7 +523,12 @@ default Version getMaxTransitiveSourceVersion() { void removeUnfinishedDeps(Set unfinishedDeps); /** - * Prepares this node for a restart of its evaluation to recover from an inconsistency. + * Prepares this node to reset its evaluation from scratch in order to recover from an + * inconsistency. + * + *

Temporary direct deps should be cleared by this call, as they will be added again when + * requested during the restarted evaluation of this node. If the graph keeps dependency edges, + * however, the temporary direct deps must be accounted for in {@link #getResetDirectDeps}. * *

Called on a {@link DirtyState#REBUILDING} node when one of the following scenarios is * observed: @@ -531,9 +537,9 @@ default Version getMaxTransitiveSourceVersion() { *

  • One or more already requested dependencies are not done. This may happen when a * dependency's node was dropped from the graph to save memory, or if a dependency was * {@linkplain DirtyType#REWIND rewound} by another node. - *
  • The corresponding {@link SkyFunction} for this node returned {@link SkyFunction.Restart} - * to indicate that one or more dependencies were done but are in need of {@linkplain - * DirtyType#REWIND rewinding} to regenerate their values. + *
  • The corresponding {@link SkyFunction} for this node returned {@link Reset} to indicate + * that one or more dependencies were done but are in need of {@linkplain DirtyType#REWIND + * rewinding} to regenerate their values. * * *

    This method is similar to calling {@link #markDirty} with {@link DirtyType#REWIND} with an @@ -541,19 +547,15 @@ default Version getMaxTransitiveSourceVersion() { * its value, while this method is called on a building node because of an issue * with a dependency. The dependency will be rewound if we are in scenario 2 above. * - *

    Temporary direct deps should be cleared by this call, as they will be added again when - * requested during the restarted evaluation of this node. If the graph keeps dependency edges, - * however, the temporary direct deps must be accounted for in {@link #getResetDirectDeps}. - * *

    Reverse deps on the other hand should be preserved - parents waiting on this node are * unaware that it is being restarted and will not register themselves again, yet they still need * to be signaled when this node is done. */ @ThreadSafe - void resetForRestartFromScratch(); + void resetEvaluationFromScratch(); /** - * If the graph keeps dependency edges and {@link #resetForRestartFromScratch} has been called on + * If the graph keeps dependency edges and {@link #resetEvaluationFromScratch} has been called on * this node since it was last done, returns the set of temporary direct deps that were registered * prior to the restart. Otherwise, returns an empty set. * diff --git a/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java index 21538256272e97..e9839244547161 100644 --- a/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/NonIncrementalInMemoryNodeEntry.java @@ -120,7 +120,7 @@ public final synchronized GroupedDeps getTemporaryDirectDeps() { } @Override - public final synchronized void resetForRestartFromScratch() { + public final synchronized void resetEvaluationFromScratch() { checkState(!hasUnsignaledDeps(), this); var newBuildingState = new NonIncrementalBuildingState(); newBuildingState.reverseDeps = dirtyBuildingState.reverseDeps; diff --git a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java index 85c67491e95298..6bb65be076ff88 100644 --- a/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java +++ b/src/main/java/com/google/devtools/build/skyframe/ParallelEvaluatorContext.java @@ -178,8 +178,8 @@ ErrorInfoManager getErrorInfoManager() { return errorInfoManager; } - boolean restartPermitted() { - return graphInconsistencyReceiver.restartPermitted(); + boolean resetPermitted() { + return graphInconsistencyReceiver.resetPermitted(); } boolean mergingSkyframeAnalysisExecutionPhases() { diff --git a/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java b/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java index d5e9c17093305c..964099b4d661df 100644 --- a/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java +++ b/src/main/java/com/google/devtools/build/skyframe/PartialReevaluationMailbox.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.skyframe.SkyFunction.Environment.ClassToInstanceMapSkyKeyComputeState; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; +import com.google.devtools.build.skyframe.SkyFunction.Reset; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; @@ -126,16 +127,16 @@ static Mail ofCauses(Causes causes) { * *

    Skyframe may enqueue a node for evaluation for several other reasons, such as when the node * declared an external dependency (via {@link SkyFunction.Environment#dependOnFuture}) that - * completes, or when the node's {@link SkyFunction#compute} method returns a {@link - * SkyFunction.Restart} value and the node is restarted. In some of these cases (e.g. returning a - * {@link SkyFunction.Restart} value), the node's {@link SkyKeyComputeState} will be invalidated, - * which also drops its mailbox, and the next time that mailbox is read it will return a "freshly - * initialized" state. But in others (e.g. an external dependency completes), the node's {@link - * SkyKeyComputeState} is retained. In any of these cases in which a node is enqueued for - * evaluation and its mailbox is retained, a flag will be set in the node's mailbox to indicate - * that the node's {@link SkyFunction} should try its best to make progress, by, e.g., checking - * whether its external dep futures have completed, checking whether its previously requested deps - * are done, or reevaluating from scratch. ({@link Causes#other}) returns the value of that flag. + * completes, or when the node's {@link SkyFunction#compute} method returns a {@link Reset} value + * and the node is restarted. In some of these cases (e.g. returning a {@link Reset} value), the + * node's {@link SkyKeyComputeState} will be invalidated, which also drops its mailbox, and the + * next time that mailbox is read it will return a "freshly initialized" state. But in others + * (e.g. an external dependency completes), the node's {@link SkyKeyComputeState} is retained. In + * any of these cases in which a node is enqueued for evaluation and its mailbox is retained, a + * flag will be set in the node's mailbox to indicate that the node's {@link SkyFunction} should + * try its best to make progress, by, e.g., checking whether its external dep futures have + * completed, checking whether its previously requested deps are done, or reevaluating from + * scratch. ({@link Causes#other}) returns the value of that flag. */ @AutoValue public abstract static class Causes { diff --git a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java index a56cada008e571..498d092102fe8a 100644 --- a/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/RecordingSkyFunctionEnvironment.java @@ -177,8 +177,8 @@ public void dependOnFuture(ListenableFuture future) { } @Override - public boolean restartPermitted() { - return delegate.restartPermitted(); + public boolean resetPermitted() { + return delegate.resetPermitted(); } @Override 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 bd83350cab50b2..03efeac201a99d 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java @@ -48,7 +48,9 @@ public interface SkyFunction { * *

    This method should return a non-{@code null} value, or {@code null} if any dependencies were * missing ({@link Environment#valuesMissing} was true before returning). In that case the missing - * dependencies will be computed and the {@code compute} method called again. + * dependencies will be computed and the {@code compute} method called again. A subsequent + * invocation of this method after missing dependencies are done is commonly referred to as a + * Skyframe restart (not to be confused with {@link Reset}). * *

    This method should throw if it fails, or if one of its dependencies fails with an exception * and this method cannot recover. If one of its dependencies fails and this method can enrich the @@ -63,7 +65,7 @@ public interface SkyFunction { * possible) exception enrichment logic simple enough to be insensitive to the evaluating thread's * interrupt state. * - *

    This method may return {@link Restart} in rare circumstances. See its docs. Do not return + *

    This method may return {@link Reset} in rare circumstances. See its docs. Do not return * values of this type unless you know exactly what you are doing. * *

    If version information is discovered for the given {@code skyKey}, {@link @@ -104,9 +106,17 @@ default Version getMaxTransitiveSourceVersionToInjectForNonHermeticFunction( /** * Sentinel {@link SkyValue} type for {@link #compute} to return, indicating that something went - * wrong, and that the evaluation returning this value must be restarted, and the nodes associated + * wrong, and that the evaluation should be started over (including calling {@link + * NodeEntry#resetEvaluationFromScratch}). + * + *

    Returning a {@link Reset} from {@link #compute} differs from returning {@code null}. A + * {@code null} return is expected under normal circumstances when a dependency is requested but + * is not yet done, causing Skyframe to restart the function when all requested dependencies are + * done. A {@link Reset} signals a more severe issue that requires clearing the associated node's + * temporary direct deps and {@linkplain NodeEntry.DirtyType#REWIND rewinding} nodes associated * with other keys in {@link #rewindGraph()} (whose directed edges should correspond to the nodes' - * direct dependencies) must also be restarted. + * direct dependencies). Rewound dependency nodes are re-evaluated before the function is + * attempted again. * *

    An intended cause for returning this is external data loss; e.g., if a dependency's * "done-ness" is intended to mean that certain data is available in an external system, but @@ -114,39 +124,39 @@ default Version getMaxTransitiveSourceVersionToInjectForNonHermeticFunction( * reevaluation of the dependency is expected to repair the discrepancy. * *

    Values of this type will never be returned by {@link Environment}'s getValue - * methods or from {@link NodeEntry#getValue()}. + * methods or from {@link NodeEntry#getValue}. * *

    All {@link ListenableFuture}s used in calls to {@link Environment#dependOnFuture} which were * not already complete will be cancelled. * - *

    This may only be returned by {@link #compute} if {@link Environment#restartPermitted} is - * true. If restarting is not permitted, {@link #compute} should throw an appropriate {@link + *

    This may only be returned by {@link #compute} if {@link Environment#resetPermitted} is true. + * If resetting is not permitted, {@link #compute} should throw an appropriate {@link * SkyFunctionException}. */ - final class Restart implements SkyValue { + final class Reset implements SkyValue { /** * Convenience method that creates a {@link MutableGraph} that fulfills the basic requirements - * of a {@link Restart}. + * of a {@link Reset}. * *

    Additional edges may be added to the graph before passing to {@link #of}. */ - public static MutableGraph newRewindGraphFor(SkyKey keyToRestart) { + public static MutableGraph newRewindGraphFor(SkyKey keyToReset) { MutableGraph rewindGraph = GraphBuilder.directed().allowsSelfLoops(false).build(); - rewindGraph.addNode(keyToRestart); + rewindGraph.addNode(keyToReset); return rewindGraph; } - public static Restart of(Graph rewindGraph) { + public static Reset 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)); + checkArgument(!rewindGraph.nodes().isEmpty(), "Rewind graph must include key to reset"); + return new Reset(ImmutableGraph.copyOf(rewindGraph)); } private final ImmutableGraph rewindGraph; - private Restart(ImmutableGraph rewindGraph) { + private Reset(ImmutableGraph rewindGraph) { this.rewindGraph = rewindGraph; } @@ -392,10 +402,10 @@ default void injectVersionForNonHermeticFunction(Version version) {} void dependOnFuture(ListenableFuture future); /** - * A {@link SkyFunction#compute} call may return {@link Restart} only if this returns {@code + * A {@link SkyFunction#compute} call may return {@link Reset} only if this returns {@code * true}. */ - boolean restartPermitted(); + boolean resetPermitted(); /** * Container for data stored in between calls to {@link #compute} for the same {@link SkyKey}. @@ -503,9 +513,9 @@ public T getInstance( *

    Note that {@link SkyKeyComputeState#close()} allows us to hold on to other kinds of * external resources and clean them up when necessary, but see the Javadoc there for caveats. * - *

    A notable example of the above note is that if {@link #compute} returns a {@link Restart} + *

    A notable example of the above note is that if {@link #compute} returns a {@link Reset} * then a call to {@link #getState} on the subsequent call to {@link #compute} will definitely - * use the {@code stateSupplier}. It's important that Skyframe do this because {@link Restart} + * use the {@code stateSupplier}. It's important that Skyframe do this because {@link Reset} * indicates that work should be redone, and so it'd be wrong to reuse work from the previous * {@link #compute} call. */ diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java index 2e3e75478ff61c..2448279c2c4aa2 100644 --- a/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunctionEnvironment.java @@ -376,7 +376,7 @@ private void addTransitiveEventsFromDepValuesForDoneNode( newlyRequestedDepsValues.put(key, valueOrNullMarker); if (valueOrNullMarker == NULL_MARKER) { // TODO(mschaller): handle registered deps that transitioned from done to dirty during eval - // But how? Restarting the current node may not help, because this dep was *registered*, not + // But how? Resetting the current node may not help, because this dep was *registered*, not // requested. For now, no node that gets registered as a dep is eligible for // intra-evaluation dirtying, so let it crash. checkState(!assertDone, "%s had not done: %s", skyKey, key); @@ -1019,8 +1019,8 @@ public String toString() { } @Override - public boolean restartPermitted() { - return evaluatorContext.restartPermitted(); + public boolean resetPermitted() { + return evaluatorContext.resetPermitted(); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 9a02ec95753862..c521306e39d928 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -452,7 +452,7 @@ public boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors() { } @Override - public boolean restartPermitted() { + public boolean resetPermitted() { return false; } diff --git a/src/test/java/com/google/devtools/build/lib/includescanning/CppIncludeScanningContextImplTest.java b/src/test/java/com/google/devtools/build/lib/includescanning/CppIncludeScanningContextImplTest.java index 4030c7ddd002f8..9f017a183b233d 100644 --- a/src/test/java/com/google/devtools/build/lib/includescanning/CppIncludeScanningContextImplTest.java +++ b/src/test/java/com/google/devtools/build/lib/includescanning/CppIncludeScanningContextImplTest.java @@ -196,7 +196,7 @@ private ActionExecutionContext createActionExecutionContext(Environment environm new ActionKeyContext(), new FileOutErr(), scratch.resolve("/execroot"), - /*metadataHandler=*/ null, + /* outputMetadataStore= */ null, environment, DiscoveredModulesPruner.DEFAULT); } @@ -237,7 +237,7 @@ protected ValueOrUntypedException getSingleValueOrUntypedException(SkyKey depKey @Override protected ImmutableMap getValueOrUntypedExceptions( Iterable depKeys) { - return StreamSupport.stream(depKeys.spliterator(), /*parallel=*/ false) + return StreamSupport.stream(depKeys.spliterator(), /* parallel= */ false) .collect( toImmutableMap( Function.identity(), @@ -271,7 +271,7 @@ public boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors() { } @Override - public boolean restartPermitted() { + public boolean resetPermitted() { return false; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index 5cf1072298b276..4326b001e4078a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -2671,9 +2671,9 @@ public void usesCorrectGraphInconsistencyReceiver( SkyFunctionName.FOR_TESTING, (key, env) -> { if (!trackIncrementalState && !useActionCache && rewindLostInputs) { - assertThat(env.restartPermitted()).isTrue(); + assertThat(env.resetPermitted()).isTrue(); } else { - assertThat(env.restartPermitted()).isFalse(); + assertThat(env.resetPermitted()).isFalse(); } return new SkyValue() {}; }); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java index 1bd28732972949..a47b7a72fabc8a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java @@ -101,7 +101,7 @@ public void dependOnFuture(ListenableFuture future) { } @Override - public boolean restartPermitted() { + public boolean resetPermitted() { return false; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java index 4fb0f35f209a9f..80eebe562de170 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java @@ -1160,9 +1160,9 @@ public final void runParallelTrackSharedActionsRewound() throws Exception { // in which 1A and 2A are shared, 1B and 2B are shared, and xB depends on an output of xA, // when 1B rewinds because the 1A output it depends on is lost, and 2B ran simultaneously with // the first, failed, evaluation of 1B and registers itself as depending on 1B's completion - // future, then 2B gets restarted when 1B clears its ActionExecutionState. Re-evaluations of - // dep actions may proceed non-deterministically, but this test makes 2A win the "rewound A" - // race, and then 1B win the "rewound B" race. + // future, then 2B gets reset when 1B clears its ActionExecutionState. Re-evaluations of dep + // actions may proceed non-deterministically, but this test makes 2A win the "rewound A" race, + // and then 1B win the "rewound B" race. setUpParallelTrackSharedActionPackage(); @@ -1211,8 +1211,8 @@ public final void runParallelTrackSharedActionsRewound() throws Exception { // (*) Count: // 1. before shared_2A is first evaluated // 2. after shared_2A is first evaluated - // 3. restarted by shared_1B's state clearing - // 4. restarted by its own rewinding, before shared_2A is again evaluated + // 3. reset by shared_1B's state clearing + // 4. reset by its own rewinding, before shared_2A is again evaluated // 5. after shared_2A is again evaluated CountDownLatch shared2BReadyForFifthTime = new CountDownLatch(1); @@ -1382,8 +1382,8 @@ final void runTreeFileArtifactRewound(SpawnShim shim) throws Exception { // // The compilation action "Compiling tree/make_cc_dir.cc/file1.cc" fails, saying that // "make_cc_dir.cc/file1.cc", one of the output files in the tree outputted by the "make_cc" - // rule, is lost. The action that generated that tree, "Action tree/make_cc_dir.cc", is - // restarted along with the failed compilation action. + // rule, is lost. The action that generated that tree, "Action tree/make_cc_dir.cc", is rewound + // along with the failed compilation action. // // This test also confirms that rewinding is compatible with critical-path tracking when a // non-shared action (like this test's compiling actions) fails and is run a second time. @@ -1434,8 +1434,8 @@ public final void runTreeArtifactRewound_allFilesLost_spawnFailed() throws Excep // tree, not the file contained in the tree. // // The linking action "Linking tree/libconsumes_tree.so" fails, saying that the "*.pic.o" files - // produced by the compilation actions are lost. The linking action which failed is restarted - // along with those compilation actions. + // produced by the compilation actions are lost. The linking action which failed is reset along + // with those compilation actions. // // This test also confirms that rewinding is compatible with critical-path tracking when a // previously completed non-shared action (like this test's compiling actions) is rerun. @@ -1454,12 +1454,12 @@ public final void runTreeArtifactRewound_oneFileLost_spawnFailed() throws Except // of the files in the tree that "Linking tree/libconsumes_tree.so" depends on. By doing so it // exercises the case when only a subset of a tree's files are lost. // - // The linking action which failed is restarted, and *all* the compilation actions whose outputs - // are included by the tree are also restarted. + // The linking action which failed is reset, and *all* the compilation actions whose outputs + // are included by the tree are rewound. // - // It would be better if only the compilation action responsible for the lost file was - // restarted, but rewinding is expected to be uncommon, so the overkill effort shouldn't be a - // problem in practice. + // It would be better if only the compilation action responsible for the lost file was rewound, + // but rewinding is expected to be uncommon, so the overkill effort shouldn't be a problem in + // practice. ImmutableList lostTreeFileArtifactNames = ImmutableList.of("make_cc_dir/file1.pic.o"); @@ -1562,8 +1562,8 @@ public final void runGeneratedRunfilesRewound_oneFileLost_spawnFailed() throws E // one of the two generated runfiles that "Executing genrule //middle:tool_user" depends on. // // Like with runTreeArtifactRewound_oneFileLost_spawnFailed, it would be better if only the one - // action responsible for the lost input was restarted, but rewinding is expected to be - // uncommon, so the overkill effort isn't expected to be a problem in practice. + // action responsible for the lost input was rewound, but rewinding is expected to be uncommon, + // so the overkill effort isn't expected to be a problem in practice. ImmutableList lostRunfiles = ImmutableList.of("gen1.dat"); diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java index 9051f81f285713..61addf46db6ccb 100644 --- a/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/InMemoryNodeEntryTest.java @@ -362,7 +362,7 @@ public void resetLifecycle() throws Exception { assertThat(entry.getResetDirectDeps()).isEmpty(); // Reset clears temporary direct deps. - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); assertThat(entry.getDirtyState()).isEqualTo(DirtyState.REBUILDING); assertThat(entry.getTemporaryDirectDeps()).isEmpty(); assertThat(entry.getTemporaryDirectDeps() instanceof GroupedDeps.WithHashSet) @@ -417,7 +417,7 @@ public void resetTwice_moreDepsRequestedBeforeFirstReset() throws Exception { assertThat(entry.signalDep(initialVersion, dep2)).isTrue(); // First reset. - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); assertThat(entry.getTemporaryDirectDeps()).isEmpty(); // Add back only one dep. @@ -426,7 +426,7 @@ public void resetTwice_moreDepsRequestedBeforeFirstReset() throws Exception { assertThat(entry.getTemporaryDirectDeps()).containsExactly(ImmutableList.of(dep1)); // Second reset. - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); assertThat(entry.getTemporaryDirectDeps()).isEmpty(); // Both deps added back. @@ -437,8 +437,8 @@ public void resetTwice_moreDepsRequestedBeforeFirstReset() throws Exception { assertThat(entry.getTemporaryDirectDeps()) .containsExactly(ImmutableList.of(dep1), ImmutableList.of(dep2)); - // If tracking of reset deps is required, make sure both deps are reported for even though only - // dep1 was registered during the most recent restarted evaluation. + // If tracking of reset deps is required, make sure both deps are reported even though only dep1 + // was registered during the most recent evaluation attempt. if (entry.keepsEdges()) { assertThat(entry.getResetDirectDeps()).containsExactly(dep1, dep2); } diff --git a/src/test/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntryTest.java b/src/test/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntryTest.java index 307e097dec5663..d253bc1cc9102f 100644 --- a/src/test/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntryTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/IncrementalInMemoryNodeEntryTest.java @@ -696,7 +696,7 @@ public void getAllDirectDepsForIncompleteNode_afterReset() throws Exception { SkyKey dep = key("dep"); entry.addSingletonTemporaryDirectDep(dep); entry.signalDep(initialVersion, dep); - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); assertThat(entry.getAllDirectDepsForIncompleteNode()).containsExactly(dep); } @@ -746,7 +746,7 @@ public void resetOnDirtyNode(@TestParameter boolean valueChanges) throws Excepti assertThat(entry.getResetDirectDeps()).isEmpty(); // Reset clears temporary direct deps. - entry.resetForRestartFromScratch(); + entry.resetEvaluationFromScratch(); assertThat(entry.getDirtyState()).isEqualTo(DirtyState.REBUILDING); assertThat(entry.getTemporaryDirectDeps()).isEmpty(); assertThat(entry.getTemporaryDirectDeps() instanceof GroupedDeps.WithHashSet) 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 6da34a78da09ec..84d16d5d9c5fce 100644 --- a/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/skyframe/MemoizingEvaluatorTest.java @@ -55,6 +55,7 @@ import com.google.devtools.build.skyframe.NotifyingHelper.EventType; import com.google.devtools.build.skyframe.NotifyingHelper.Listener; import com.google.devtools.build.skyframe.NotifyingHelper.Order; +import com.google.devtools.build.skyframe.SkyFunction.Reset; import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.proto.GraphInconsistency.Inconsistency; import com.google.errorprone.annotations.ForOverride; @@ -144,7 +145,7 @@ protected boolean cyclesDetected() { } @ForOverride - protected boolean restartSupported() { + protected boolean resetSupported() { return true; } @@ -2358,20 +2359,19 @@ public void dirtyThenDeleted() throws Exception { } /** - * Basic test for a {@link SkyFunction.Restart} with no rewinding of dependencies. + * Basic test for a {@link SkyFunction.Reset} with no rewinding of dependencies. * *

    Ensures that {@link NodeEntry#getResetDirectDeps} is used correctly by Skyframe to avoid - * registering duplicate rdep edges when {@code dep} is requested both before and after a restart. + * registering duplicate rdep edges when {@code dep} is requested both before and after a reset. * - *

    This test covers the case where {@code dep} is newly requested post-restart during a {@link + *

    This test covers the case where {@code dep} is newly requested post-reset during a {@link * SkyFunction#compute} invocation that returns a {@link SkyValue}, which exercises a different * {@link AbstractParallelEvaluator} code path than the scenario covered by {@link - * #restartSelfOnly_extraDepMissingAfterRestart}. + * #resetSelfOnly_extraDepMissingAfterReset}. */ - // TODO(b/228090759): Similarly test a restart on an incremental build. @Test - public void restartSelfOnly_singleDep() throws Exception { - assume().that(restartSupported()).isTrue(); + public void resetSelfOnly_singleDep() throws Exception { + assume().that(resetSupported()).isTrue(); var inconsistencyReceiver = new RecordingInconsistencyReceiver(); tester.setGraphInconsistencyReceiver(inconsistencyReceiver); @@ -2384,7 +2384,7 @@ public void restartSelfOnly_singleDep() throws Exception { .getOrCreate(top) .setBuilder( new SkyFunction() { - private boolean restarted = false; + private boolean alreadyReset = false; @Nullable @Override @@ -2393,9 +2393,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept if (depValue == null) { return null; } - if (!restarted) { - restarted = true; - return Restart.of(Restart.newRewindGraphFor(top)); + if (!alreadyReset) { + alreadyReset = true; + return Reset.of(Reset.newRewindGraphFor(top)); } return new StringValue("topVal"); } @@ -2416,20 +2416,20 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept } /** - * Test for a {@link SkyFunction.Restart} with no rewinding of dependencies, with a missing - * dependency requested post-restart. + * Test for a {@link SkyFunction.Reset} with no rewinding of dependencies, with a missing + * dependency requested post-reset. * *

    Ensures that {@link NodeEntry#getResetDirectDeps} is used correctly by Skyframe to avoid - * registering duplicate rdep edges when {@code dep} is requested both before and after a restart. + * registering duplicate rdep edges when {@code dep} is requested both before and after a reset. * - *

    This test covers the case where {@code dep} is newly requested post-restart in a {@link + *

    This test covers the case where {@code dep} is newly requested post-reset in a {@link * SkyFunction#compute} invocation that returns {@code null} (because {@code extraDep} is * missing), which exercises a different {@link AbstractParallelEvaluator} code path than the - * scenario covered by {@link #restartSelfOnly_singleDep}. + * scenario covered by {@link #resetSelfOnly_singleDep}. */ @Test - public void restartSelfOnly_extraDepMissingAfterRestart() throws Exception { - assume().that(restartSupported()).isTrue(); + public void resetSelfOnly_extraDepMissingAfterReset() throws Exception { + assume().that(resetSupported()).isTrue(); var inconsistencyReceiver = new RecordingInconsistencyReceiver(); tester.setGraphInconsistencyReceiver(inconsistencyReceiver); @@ -2443,7 +2443,7 @@ public void restartSelfOnly_extraDepMissingAfterRestart() throws Exception { .getOrCreate(top) .setBuilder( new SkyFunction() { - private boolean restarted = false; + private boolean alreadyReset = false; @Nullable @Override @@ -2452,9 +2452,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept if (depValue == null) { return null; } - if (!restarted) { - restarted = true; - return Restart.of(Restart.newRewindGraphFor(top)); + if (!alreadyReset) { + alreadyReset = true; + return Reset.of(Reset.newRewindGraphFor(top)); } var extraDepValue = env.getValue(extraDep); if (extraDepValue == null) { @@ -2484,15 +2484,15 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept } /** - * Tests that if a dependency is requested prior to a {@link SkyFunction.Restart} but not after, + * Tests that if a dependency is requested prior to a {@link SkyFunction.Reset} but not after, * then the corresponding reverse dep edge is removed. * *

    This happens in practice with input-discovering actions, which use mutable state to track * input discovery, resulting in unstable dependencies. */ @Test - public void restartSelfOnly_depNotRequestedAgainAfterRestart() throws Exception { - assume().that(restartSupported()).isTrue(); + public void resetSelfOnly_depNotRequestedAgainAfterReset() throws Exception { + assume().that(resetSupported()).isTrue(); var inconsistencyReceiver = new RecordingInconsistencyReceiver(); tester.setGraphInconsistencyReceiver(inconsistencyReceiver); @@ -2506,21 +2506,21 @@ public void restartSelfOnly_depNotRequestedAgainAfterRestart() throws Exception .getOrCreate(top) .setBuilder( new SkyFunction() { - private boolean restarted = false; + private boolean alreadyReset = false; @Nullable @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { env.getValuesAndExceptions( - restarted + alreadyReset ? ImmutableList.of(stableDep) : ImmutableList.of(stableDep, flakyDep)); if (env.valuesMissing()) { return null; } - if (!restarted) { - restarted = true; - return Restart.of(Restart.newRewindGraphFor(top)); + if (!alreadyReset) { + alreadyReset = true; + return Reset.of(Reset.newRewindGraphFor(top)); } return new StringValue("topVal"); } @@ -2554,8 +2554,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept * --nokeep_going} build or if the user hits ctrl+c. */ @Test - public void restartSelfOnly_evaluationAborted() throws Exception { - assume().that(restartSupported()).isTrue(); + public void resetSelfOnly_evaluationAborted() throws Exception { + assume().that(resetSupported()).isTrue(); assume().that(incrementalitySupported()).isTrue(); var inconsistencyReceiver = new RecordingInconsistencyReceiver(); @@ -2569,20 +2569,20 @@ public void restartSelfOnly_evaluationAborted() throws Exception { .getOrCreate(top) .setBuilder( new SkyFunction() { - private boolean restarted = false; + private boolean alreadyReset = false; @Nullable @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { - if (restarted) { + if (alreadyReset) { throw new InterruptedException("Evaluation aborted"); } var depValue = env.getValue(dep); if (depValue == null) { return null; } - restarted = true; - return Restart.of(Restart.newRewindGraphFor(top)); + alreadyReset = true; + return Reset.of(Reset.newRewindGraphFor(top)); } }); tester.getOrCreate(dep).setConstantValue(new StringValue("depVal")); @@ -2610,7 +2610,7 @@ public synchronized void noteInconsistencyAndMaybeThrow( } @Override - public boolean restartPermitted() { + public boolean resetPermitted() { return true; } diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingHelper.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingHelper.java index 776219187438ef..fe4a313f7bfec3 100644 --- a/src/test/java/com/google/devtools/build/skyframe/NotifyingHelper.java +++ b/src/test/java/com/google/devtools/build/skyframe/NotifyingHelper.java @@ -348,8 +348,8 @@ public ImmutableSet getAllDirectDepsForIncompleteNode() throws Interrupt } @Override - public void resetForRestartFromScratch() { - delegate.resetForRestartFromScratch(); + public void resetEvaluationFromScratch() { + delegate.resetEvaluationFromScratch(); graphListener.accept(myKey, EventType.RESET_FOR_RESTART_FROM_SCRATCH, Order.AFTER, this); }