Stop re-requesting ActionExecutionFunction deps after input discovery causes a Skyframe restart: we no longer have the invariant that a SkyFunction requests the same deps on each evaluation, and iterating over all the inputs of an action isn't trivial. PiperOrigin-RevId: 420830112
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 d8bdedc..c53a6b7 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
@@ -28,8 +28,8 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimap; import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; @@ -248,11 +248,6 @@ // Missing deps. return null; } - } else if (state.allInputs.packageLookupsRequested != null) { - // Preserve the invariant that we ask for the same deps each build. Because we know these deps - // were already retrieved successfully, we don't request them with exceptions. - env.getValues(state.allInputs.packageLookupsRequested); - Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state); } CheckInputResults checkedInputs = null; @@ -262,22 +257,22 @@ .actionFileSystemType() .equals(ActionFileSystemType.STAGE_REMOTE_FILES)); - Iterable<SkyKey> depKeys = getInputDepKeys(allInputs, state); - // We do this unconditionally to maintain our invariant of asking for the same deps each build. - List< - ValueOrException3< - SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>> - inputDeps = - env.getOrderedValuesOrThrow( - depKeys, - SourceArtifactException.class, - ActionExecutionException.class, - ArtifactNestedSetEvalException.class); - try { if (previousExecution == null && !state.hasArtifactData()) { // Do we actually need to find our metadata? - checkedInputs = checkInputs(env, action, inputDeps, allInputs, depKeys, actionLookupData); + Iterable<SkyKey> depKeys = getInputDepKeys(allInputs, state); + checkedInputs = + checkInputs( + env, + action, + env.getOrderedValuesOrThrow( + depKeys, + SourceArtifactException.class, + ActionExecutionException.class, + ArtifactNestedSetEvalException.class), + allInputs, + depKeys, + actionLookupData); } } catch (ActionExecutionException e) { throw new ActionExecutionFunctionException(e); @@ -339,7 +334,14 @@ actionStartTime); } catch (LostInputsActionExecutionException e) { return handleLostInputs( - e, actionLookupData, action, actionStartTime, env, inputDeps, allInputs, depKeys, state); + e, + actionLookupData, + action, + actionStartTime, + env, + allInputs, + getInputDepKeys(allInputs, state), + state); } catch (ActionExecutionException e) { // In this case we do not report the error to the action reporter because we have already // done it in SkyframeActionExecutor.reportErrorIfNotAbortingMode() method. That method @@ -418,12 +420,6 @@ Action action, long actionStartTime, Environment env, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet<Artifact> allInputs, Iterable<SkyKey> depKeys, InputDiscoveryState state) @@ -436,8 +432,7 @@ RewindPlan rewindPlan = null; try { ActionInputDepOwners inputDepOwners = - createAugmentedInputDepOwners( - e, action, env, inputDeps, allInputs, depKeys, actionLookupData); + createAugmentedInputDepOwners(e, action, env, allInputs, actionLookupData); // Collect the set of direct deps of this action which may be responsible for the lost inputs, // some of which may be discovered. @@ -516,14 +511,7 @@ LostInputsActionExecutionException e, Action action, Environment env, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet<Artifact> allInputs, - Iterable<SkyKey> requestedSkyKeys, ActionLookupData actionLookupDataForError) throws InterruptedException { @@ -540,9 +528,7 @@ getInputDepOwners( env, action, - inputDeps, allInputs, - requestedSkyKeys, lostInputsAndOwnersSoFar, actionLookupDataForError); } catch (ActionExecutionException unexpected) { @@ -1099,14 +1085,7 @@ private ActionInputDepOwnerMap getInputDepOwners( Environment env, Action action, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet<Artifact> allInputs, - Iterable<SkyKey> requestedSkyKeys, Collection<ActionInput> lostInputs, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { @@ -1117,9 +1096,9 @@ return accumulateInputs( env, action, - inputDeps, + /*inputDeps=*/ null, allInputs, - requestedSkyKeys, + /*requestedSkyKeys=*/ null, ignoredInputDepsSize -> new ActionInputDepOwnerMap(lostInputs), (actionInputMapSink, expandedArtifacts, @@ -1156,48 +1135,56 @@ /** * May return {@code null} if {@code allowValuesMissingEarlyReturn} and {@code * env.valuesMissing()} are true and no inputs result in {@link ActionExecutionException}s. + * + * <p>If {@code inputDeps} (and therefore {@code requestedSkyKeys}) is null, assumes that deps + * have already been checked for exceptions and inserted into the {@link + * ArtifactNestedSetFunction} cache, so those steps are skipped. (This codepath currently only + * used for action rewinding.) */ @Nullable private <S extends ActionInputMapSink, R> R accumulateInputs( Environment env, Action action, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, + @Nullable + List< + ValueOrException3< + SourceArtifactException, + ActionExecutionException, + ArtifactNestedSetEvalException>> + inputDeps, NestedSet<Artifact> allInputs, - Iterable<SkyKey> requestedSkyKeys, + @Nullable Iterable<SkyKey> requestedSkyKeys, IntFunction<S> actionInputMapSinkFactory, AccumulateInputResultsFactory<S, R> accumulateInputResultsFactory, boolean allowValuesMissingEarlyReturn, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { Predicate<Artifact> isMandatoryInput = makeMandatoryInputPredicate(action); - ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = - new ActionExecutionFunctionExceptionHandler( - Suppliers.memoize( - () -> { - ImmutableList<Artifact> allInputsList = allInputs.toList(); - Multimap<SkyKey, Artifact> skyKeyToArtifactSet = - MultimapBuilder.hashKeys().hashSetValues().build(); - allInputsList.forEach( - input -> { - SkyKey key = Artifact.key(input); - if (key != input) { - skyKeyToArtifactSet.put(key, input); - } - }); - return skyKeyToArtifactSet; - }), - inputDeps, - action, - isMandatoryInput, - requestedSkyKeys, - env.valuesMissing()); - - actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); + ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = null; + if (inputDeps != null) { + actionExecutionFunctionExceptionHandler = + new ActionExecutionFunctionExceptionHandler( + Suppliers.memoize( + () -> { + ImmutableList<Artifact> allInputsList = allInputs.toList(); + SetMultimap<SkyKey, Artifact> skyKeyToArtifactSet = + MultimapBuilder.hashKeys().hashSetValues().build(); + allInputsList.forEach( + input -> { + SkyKey key = Artifact.key(input); + if (key != input) { + skyKeyToArtifactSet.put(key, input); + } + }); + return skyKeyToArtifactSet; + }), + inputDeps, + action, + isMandatoryInput, + requestedSkyKeys, + env.valuesMissing()); + actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); + } if (env.valuesMissing() && allowValuesMissingEarlyReturn) { return null; @@ -1264,8 +1251,13 @@ } if (value instanceof MissingArtifactValue) { if (isMandatoryInput.test(input)) { - actionExecutionFunctionExceptionHandler.accumulateMissingFileArtifactValue( - input, (MissingArtifactValue) value); + checkNotNull( + actionExecutionFunctionExceptionHandler, + "Missing artifact should have been caught already %s %s %s", + input, + value, + action) + .accumulateMissingFileArtifactValue(input, (MissingArtifactValue) value); continue; } else { value = FileArtifactValue.MISSING_FILE_MARKER; @@ -1281,9 +1273,12 @@ value, env); } - // After accumulating the inputs, we might find some mandatory artifact with - // SourceFileInErrorArtifactValue. - actionExecutionFunctionExceptionHandler.maybeThrowException(); + + if (actionExecutionFunctionExceptionHandler != null) { + // After accumulating the inputs, we might find some mandatory artifact with + // SourceFileInErrorArtifactValue. + actionExecutionFunctionExceptionHandler.maybeThrowException(); + } return accumulateInputResultsFactory.create( inputArtifactData, @@ -1473,7 +1468,7 @@ /** Helper subclass for the error-handling logic for ActionExecutionFunction#accumulateInputs. */ private final class ActionExecutionFunctionExceptionHandler { - private final Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions; + private final Supplier<SetMultimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions; private final List< ValueOrException3< SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>> @@ -1487,7 +1482,7 @@ private ActionExecutionException firstActionExecutionException; ActionExecutionFunctionExceptionHandler( - Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions, + Supplier<SetMultimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions, List< ValueOrException3< SourceArtifactException,