Skip to content

Commit

Permalink
Remove lostDiscoveredInputsMap.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Dec 21, 2023
1 parent 3127825 commit 4760212
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 84 deletions.
10 changes: 0 additions & 10 deletions src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
65 changes: 12 additions & 53 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@
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;
import com.google.common.base.MoreObjects;
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -494,39 +485,6 @@ private static ImmutableSet<SkyKey> getInputDepKeys(
return result.build();
}

private boolean declareDepsOnLostDiscoveredInputsIfAny(Environment env, Action action)
throws InterruptedException, ActionExecutionFunctionException {
ImmutableList<SkyKey> 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.
Expand All @@ -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<SkyKey> lostDiscoveredInputs = ImmutableList.of();
// TODO: b/315059768 - We can likely just use inputDepKeys. Confirm this and simplify.
ImmutableSet<SkyKey> failedActionDeps;
if (e.isFromInputDiscovery()) {
// Lost inputs found during input discovery are necessarily ordinary derived artifacts. Their
Expand All @@ -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.<SkyKey>builder().addAll(inputDepKeys).addAll(lostDiscoveredInputs).build();
ImmutableSet.<SkyKey>builder()
.addAll(inputDepKeys)
.addAll(
Collections2.transform(
e.getLostInputs().values(), input -> Artifact.key((Artifact) input)))
.build();
} else if (state.discoveredInputs != null) {
failedActionDeps =
ImmutableSet.<SkyKey>builder()
.addAll(inputDepKeys)
.addAll(Lists.transform(state.discoveredInputs.toList(), Artifact::key))
.addAll(Artifact.keys(state.discoveredInputs.toList()))
.build();
} else {
failedActionDeps = inputDepKeys;
Expand All @@ -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(
Expand All @@ -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()) {
Expand All @@ -615,7 +574,7 @@ private SkyFunction.Reset handleLostInputs(
}

skyframeActionExecutor.prepareForRewinding(
actionLookupData, action, lostDiscoveredInputs, rewindPlan.getDepsToRewind());
actionLookupData, action, rewindPlan.getDepsToRewind());
return rewindPlan.getReset();
}

Expand Down
21 changes: 0 additions & 21 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,6 @@ public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
// receiving more than one of those events per action.
private Set<OwnerlessArtifactWrapper> 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<OwnerlessArtifactWrapper, ImmutableList<SkyKey>> lostDiscoveredInputsMap;

private ActionOutputDirectoryHelper outputDirectoryHelper;

private OptionsProvider options;
Expand All @@ -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<ImmutableList<Root>> sourceRootSupplier;

private DiscoveredModulesPruner discoveredModulesPruner;
Expand Down Expand Up @@ -297,15 +288,13 @@ 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.
this.options = options;
// 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;

Expand Down Expand Up @@ -411,7 +400,6 @@ void executionOver() {
this.outputService = null;
this.buildActionMap = null;
this.rewoundActions = null;
this.lostDiscoveredInputsMap = null;
this.actionCacheChecker = null;
this.outputDirectoryHelper = null;
}
Expand Down Expand Up @@ -448,7 +436,6 @@ boolean shouldEmitProgressEvents(Action action) {
void prepareForRewinding(
ActionLookupData failedKey,
Action failedAction,
ImmutableList<SkyKey> lostDiscoveredInputs,
ImmutableList<Action> depsToRewind) {
var ownerlessArtifactWrapper = new OwnerlessArtifactWrapper(failedAction.getPrimaryOutput());
ActionExecutionState state = buildActionMap.get(ownerlessArtifactWrapper);
Expand All @@ -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());
}
Expand All @@ -485,11 +469,6 @@ private void prepareDepForRewinding(ActionLookupData failedKey, Action dep) {
}
}

@Nullable
ImmutableList<SkyKey> getLostDiscoveredInputs(Action action) {
return lostDiscoveredInputsMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput()));
}

void noteActionEvaluationStarted(ActionLookupData actionLookupData, Action action) {
if (completionReceiver != null) {
completionReceiver.noteActionEvaluationStarted(actionLookupData, action);
Expand Down

0 comments on commit 4760212

Please sign in to comment.