From 47602124250790055b3fb8e35046491d0deca3e6 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 21 Dec 2023 10:07:51 -0800 Subject: [PATCH] Remove `lostDiscoveredInputsMap`. Tracking lost discovered inputs for purposes of requesting them after a restart is no longer necessary because they are guaranteed to be contained in the scheduling dependencies. Further simplifications are possible - see the `TODO`. PiperOrigin-RevId: 592891912 Change-Id: I4723ef2a37c2c8209a340c87ebda69cd6f5a6e9d --- .../lib/buildtool/BuildRequestOptions.java | 10 --- .../lib/skyframe/ActionExecutionFunction.java | 65 ++++--------------- .../lib/skyframe/SkyframeActionExecutor.java | 21 ------ 3 files changed, 12 insertions(+), 84 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index bb07db755ec95c..2bfa4214ddadeb 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -346,16 +346,6 @@ public String getSymlinkPrefix(String productName) { help = "Whether to use action rewinding to recover from lost inputs.") public boolean rewindLostInputs; - @Option( - name = "track_lost_discovered_inputs", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.EXECUTION}, - help = - "Whether to track lost discovered inputs and proactively request them after rewinding." - + " This option is a rollout mechanism for removing this behavior, see b/315059768.") - public boolean trackLostDiscoveredInputs; - @Option( name = "incompatible_skip_genfiles_symlink", defaultValue = "true", 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 e06d89f95f1698..7c0f9e9fdcb6b3 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 @@ -16,7 +16,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; -import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.concurrent.TimeUnit.MINUTES; import com.google.common.base.Joiner; @@ -24,6 +23,7 @@ import com.google.common.base.Predicates; import com.google.common.base.Suppliers; import com.google.common.base.Verify; +import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -59,7 +59,6 @@ import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.LostInputsActionExecutionException; -import com.google.devtools.build.lib.actions.MissingInputFileException; import com.google.devtools.build.lib.actions.PackageRootResolver; import com.google.devtools.build.lib.actions.SpawnMetrics; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; @@ -258,14 +257,6 @@ private SkyValue computeInternal(ActionLookupData actionLookupData, Environment env = new ProgressEventSuppressingEnvironment(env); } - if (action.discoversInputs()) { - // If this action previously failed due to a lost input found during input discovery, ensure - // that the input is regenerated before attempting discovery again. - if (declareDepsOnLostDiscoveredInputsIfAny(env, action)) { - return null; - } - } - InputDiscoveryState state; if (action.discoversInputs()) { state = env.getState(InputDiscoveryState::new); @@ -494,39 +485,6 @@ private static ImmutableSet getInputDepKeys( return result.build(); } - private boolean declareDepsOnLostDiscoveredInputsIfAny(Environment env, Action action) - throws InterruptedException, ActionExecutionFunctionException { - ImmutableList previouslyLostDiscoveredInputs = - skyframeActionExecutor.getLostDiscoveredInputs(action); - if (previouslyLostDiscoveredInputs != null) { - SkyframeLookupResult lostInputValues = - env.getValuesAndExceptions(previouslyLostDiscoveredInputs); - if (env.valuesMissing()) { - return true; - } - for (SkyKey lostInputKey : previouslyLostDiscoveredInputs) { - try { - SkyValue value = - lostInputValues.getOrThrow( - lostInputKey, MissingInputFileException.class, ActionExecutionException.class); - if (value == null) { - return true; - } - } catch (MissingInputFileException e) { - // MissingInputFileException comes from problems with source artifact construction. - // Rewinding never invalidates source artifacts. - throw new IllegalStateException( - "MissingInputFileException unexpected from rewound generated discovered input. key=" - + lostInputKey, - e); - } catch (ActionExecutionException e) { - throw new ActionExecutionFunctionException(e); - } - } - } - return false; - } - /** * 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. @@ -549,7 +507,7 @@ private SkyFunction.Reset handleLostInputs( // Collect the set of direct deps of this action which may be responsible for the lost inputs, // some of which may be discovered. - ImmutableList lostDiscoveredInputs = ImmutableList.of(); + // TODO: b/315059768 - We can likely just use inputDepKeys. Confirm this and simplify. ImmutableSet failedActionDeps; if (e.isFromInputDiscovery()) { // Lost inputs found during input discovery are necessarily ordinary derived artifacts. Their @@ -558,17 +516,18 @@ private SkyFunction.Reset handleLostInputs( // lostDiscoveredInputsMap. Also, lost inputs from input discovery may come from nested sets, // which may be directly represented in skyframe. To ensure that applicable nested set nodes // are rewound, this action's deps are also considered when computing the rewind plan. - lostDiscoveredInputs = - e.getLostInputs().values().stream() - .map(input -> Artifact.key((Artifact) input)) - .collect(toImmutableList()); failedActionDeps = - ImmutableSet.builder().addAll(inputDepKeys).addAll(lostDiscoveredInputs).build(); + ImmutableSet.builder() + .addAll(inputDepKeys) + .addAll( + Collections2.transform( + e.getLostInputs().values(), input -> Artifact.key((Artifact) input))) + .build(); } else if (state.discoveredInputs != null) { failedActionDeps = ImmutableSet.builder() .addAll(inputDepKeys) - .addAll(Lists.transform(state.discoveredInputs.toList(), Artifact::key)) + .addAll(Artifact.keys(state.discoveredInputs.toList())) .build(); } else { failedActionDeps = inputDepKeys; @@ -584,7 +543,7 @@ private SkyFunction.Reset handleLostInputs( } catch (ActionExecutionException rewindingFailedException) { // This ensures coalesced shared actions aren't orphaned. skyframeActionExecutor.prepareForRewinding( - actionLookupData, action, lostDiscoveredInputs, /* depsToRewind= */ ImmutableList.of()); + actionLookupData, action, /* depsToRewind= */ ImmutableList.of()); throw new ActionExecutionFunctionException( new AlreadyReportedActionExecutionException( skyframeActionExecutor.processAndGetExceptionToThrow( @@ -598,7 +557,7 @@ private SkyFunction.Reset handleLostInputs( // Obsolete the ActionExecutionState so that after rewinding, we will check inputs again and // discover the propagated exception. skyframeActionExecutor.prepareForRewinding( - actionLookupData, action, lostDiscoveredInputs, /* depsToRewind= */ ImmutableList.of()); + actionLookupData, action, /* depsToRewind= */ ImmutableList.of()); throw undoneInputsException; } finally { if (e.isActionStartedEventAlreadyEmitted()) { @@ -615,7 +574,7 @@ private SkyFunction.Reset handleLostInputs( } skyframeActionExecutor.prepareForRewinding( - actionLookupData, action, lostDiscoveredInputs, rewindPlan.getDepsToRewind()); + actionLookupData, action, rewindPlan.getDepsToRewind()); return rewindPlan.getReset(); } 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 55f7c32a4432cb..df6b92fdb22a6d 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 @@ -198,14 +198,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { // receiving more than one of those events per action. private Set rewoundActions; - // We also keep track of actions that failed due to lost discovered inputs. In some circumstances - // the input discovery process will use a discovered input before requesting it as a dep. If that - // input was generated but is lost, and its generating action is rewound, then the lost input's - // generating action must be rerun before the failed action tries input discovery again. A - // previously failed action satisfies that requirement by requesting the deps in this map at the - // start of its next attempt, - private ConcurrentMap> lostDiscoveredInputsMap; - private ActionOutputDirectoryHelper outputDirectoryHelper; private OptionsProvider options; @@ -222,7 +214,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { private OutputService outputService; private boolean finalizeActions; private boolean rewindingEnabled; - private boolean trackLostDiscoveredInputs; private final Supplier> sourceRootSupplier; private DiscoveredModulesPruner discoveredModulesPruner; @@ -297,7 +288,6 @@ void prepareForExecution( // Start with a new map each build so there's no issue with internal resizing. this.buildActionMap = Maps.newConcurrentMap(); this.rewoundActions = Sets.newConcurrentHashSet(); - this.lostDiscoveredInputsMap = Maps.newConcurrentMap(); this.hadExecutionError = false; this.actionCacheChecker = checkNotNull(actionCacheChecker); // Don't cache possibly stale data from the last build. @@ -305,7 +295,6 @@ void prepareForExecution( // Cache some option values for performance, since we consult them on every action. this.finalizeActions = buildRequestOptions.finalizeActions; this.rewindingEnabled = buildRequestOptions.rewindLostInputs; - this.trackLostDiscoveredInputs = buildRequestOptions.trackLostDiscoveredInputs; this.outputService = outputService; this.outputDirectoryHelper = outputDirectoryHelper; @@ -411,7 +400,6 @@ void executionOver() { this.outputService = null; this.buildActionMap = null; this.rewoundActions = null; - this.lostDiscoveredInputsMap = null; this.actionCacheChecker = null; this.outputDirectoryHelper = null; } @@ -448,7 +436,6 @@ boolean shouldEmitProgressEvents(Action action) { void prepareForRewinding( ActionLookupData failedKey, Action failedAction, - ImmutableList lostDiscoveredInputs, ImmutableList depsToRewind) { var ownerlessArtifactWrapper = new OwnerlessArtifactWrapper(failedAction.getPrimaryOutput()); ActionExecutionState state = buildActionMap.get(ownerlessArtifactWrapper); @@ -457,9 +444,6 @@ void prepareForRewinding( // obsolete. state.obsolete(failedKey, buildActionMap, ownerlessArtifactWrapper); } - if (trackLostDiscoveredInputs && !lostDiscoveredInputs.isEmpty()) { - lostDiscoveredInputsMap.put(ownerlessArtifactWrapper, lostDiscoveredInputs); - } if (!actionFileSystemType().inMemoryFileSystem()) { outputDirectoryHelper.invalidateTreeArtifactDirectoryCreation(failedAction.getOutputs()); } @@ -485,11 +469,6 @@ private void prepareDepForRewinding(ActionLookupData failedKey, Action dep) { } } - @Nullable - ImmutableList getLostDiscoveredInputs(Action action) { - return lostDiscoveredInputsMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())); - } - void noteActionEvaluationStarted(ActionLookupData actionLookupData, Action action) { if (completionReceiver != null) { completionReceiver.noteActionEvaluationStarted(actionLookupData, action);