Skip to content

Commit

Permalink
Automated rollback of commit 37bd5f6.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Rollforward with fix: Don't use Immutable map builder to agggregate Filesets. We may reach the same one via direct and indirect runfiles, so we need to allow for duplicate entries.

*** Original change description ***

Automated rollback of commit 3bace1b.

*** Reason for rollback ***

b/112583337
RELNOTES: None

*** Original change description ***

Track Fileset in artifact expansion.

PiperOrigin-RevId: 208748163
  • Loading branch information
@ericfelly
ericfelly authored and Copybara-Service committed Aug 15, 2018
1 parent 2734ceb commit 8dece49
Show file tree
Hide file tree
Showing 7 changed files with 132 additions and 46 deletions.
17 changes: 8 additions & 9 deletions src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ private ShowSubcommands(boolean shouldShowSubcommands, boolean prettyPrintArgs)
private final MetadataHandler metadataHandler;
private final FileOutErr fileOutErr;
private final ImmutableMap<String, String> clientEnv;
private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>>
inputFilesetMappings;
private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets;
@Nullable private final ArtifactExpander artifactExpander;
@Nullable private final Environment env;

Expand All @@ -83,7 +82,7 @@ private ActionExecutionContext(
MetadataHandler metadataHandler,
FileOutErr fileOutErr,
Map<String, String> clientEnv,
ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
@Nullable ArtifactExpander artifactExpander,
@Nullable SkyFunction.Environment env,
@Nullable FileSystem actionFileSystem,
Expand All @@ -94,7 +93,7 @@ private ActionExecutionContext(
this.metadataHandler = metadataHandler;
this.fileOutErr = fileOutErr;
this.clientEnv = ImmutableMap.copyOf(clientEnv);
this.inputFilesetMappings = inputFilesetMappings;
this.topLevelFilesets = topLevelFilesets;
this.executor = executor;
this.artifactExpander = artifactExpander;
this.env = env;
Expand All @@ -113,7 +112,7 @@ public ActionExecutionContext(
MetadataHandler metadataHandler,
FileOutErr fileOutErr,
Map<String, String> clientEnv,
ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
ArtifactExpander artifactExpander,
@Nullable FileSystem actionFileSystem,
@Nullable Object skyframeDepsResult) {
Expand All @@ -125,7 +124,7 @@ public ActionExecutionContext(
metadataHandler,
fileOutErr,
clientEnv,
inputFilesetMappings,
topLevelFilesets,
artifactExpander,
/*env=*/ null,
actionFileSystem,
Expand Down Expand Up @@ -229,8 +228,8 @@ public ExtendedEventHandler getEventHandler() {
return executor.getEventHandler();
}

public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getInputFilesetMappings() {
return inputFilesetMappings;
public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getTopLevelFilesets() {
return topLevelFilesets;
}

@Nullable
Expand Down Expand Up @@ -326,7 +325,7 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) {
metadataHandler,
fileOutErr,
clientEnv,
inputFilesetMappings,
topLevelFilesets,
artifactExpander,
env,
actionFileSystem,
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,15 @@ public interface ArtifactExpander {
* Only aggregating middlemen and tree artifacts are expanded.
*/
void expand(Artifact artifact, Collection<? super Artifact> output);

/**
* Retrieve the expansion of Filesets for the given artifact.
*
* @param artifact {@code artifact.isFileset()} must be true.
*/
default ImmutableList<FilesetOutputSymlink> getFileset(Artifact artifact) {
throw new UnsupportedOperationException();
}
}

public static final ImmutableList<Artifact> NO_ARTIFACTS = ImmutableList.of();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ public Spawn getSpawn(ActionExecutionContext actionExecutionContext)
return getSpawn(
actionExecutionContext.getArtifactExpander(),
actionExecutionContext.getClientEnv(),
actionExecutionContext.getInputFilesetMappings());
actionExecutionContext.getTopLevelFilesets());
}

Spawn getSpawn(
Expand Down
80 changes: 52 additions & 28 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 @@ -28,7 +28,6 @@
import com.google.devtools.build.lib.actions.ActionInputMap;
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
Expand All @@ -48,7 +47,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.rules.cpp.IncludeScannable;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -157,7 +155,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
env.getValues(state.allInputs.keysRequested);
Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state);
}
Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkedInputs = null;
CheckInputResults checkedInputs = null;
try {
// Declare deps on known inputs to action. We do this unconditionally to maintain our
// invariant of asking for the same deps each build.
Expand Down Expand Up @@ -199,13 +197,14 @@ public SkyValue compute(SkyKey skyKey, Environment env)

if (checkedInputs != null) {
Preconditions.checkState(!state.hasArtifactData(), "%s %s", state, action);
state.inputArtifactData = checkedInputs.first;
state.expandedArtifacts = checkedInputs.second;
state.inputArtifactData = checkedInputs.actionInputMap;
state.expandedArtifacts = checkedInputs.expandedArtifacts;
state.expandedFilesets = checkedInputs.expandedFilesets;
if (skyframeActionExecutor.usesActionFileSystem()) {
state.actionFileSystem =
skyframeActionExecutor.createActionFileSystem(
directories.getRelativeOutputPath(),
checkedInputs.first,
checkedInputs.actionInputMap,
action.getOutputs());
}
}
Expand Down Expand Up @@ -462,33 +461,32 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
metadataHandler.discardOutputMetadata();
}

ImmutableMap.Builder<PathFragment, ImmutableList<FilesetOutputSymlink>> filesetMappings =
ImmutableMap.builder();
// Make sure this is a regular HashMap rather than ImmutableMapBuilder so that we are safe
// in case of collisions.
Map<PathFragment, ImmutableList<FilesetOutputSymlink>> filesetMappings = new HashMap<>();
for (Artifact actionInput : action.getInputs()) {
if (!actionInput.isFileset()) {
continue;
}

ActionLookupKey filesetActionLookupKey = (ActionLookupKey) actionInput.getArtifactOwner();
// Index 0 for the Fileset ConfiguredTarget indicates the SkyframeFilesetManifestAction where
// we compute the fileset's outputSymlinks.
SkyKey filesetActionKey = ActionExecutionValue.key(filesetActionLookupKey, 0);
ActionExecutionValue filesetValue = (ActionExecutionValue) env.getValue(filesetActionKey);
if (filesetValue == null) {
// At this point skyframe does not guarantee that the filesetValue will be ready, since
// the current action does not directly depend on the outputs of the
// SkyframeFilesetManifestAction whose ActionExecutionValue (filesetValue) is needed here.
// TODO(kush): Get rid of this hack by making the outputSymlinks available in the Fileset
// artifact, which this action depends on, so its value will be guaranteed to be present.
ImmutableList<FilesetOutputSymlink> mapping =
ActionInputMapHelper.getFilesets(env, actionInput);
if (mapping == null) {
return null;
}
filesetMappings.put(actionInput.getExecPath(), filesetValue.getOutputSymlinks());
filesetMappings.put(actionInput.getExecPath(), mapping);
}

ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> filesets =
filesetMappings.build();
ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets =
ImmutableMap.copyOf(filesetMappings);

// Aggregate top-level Filesets with Filesets nested in Runfiles. Both should be used to update
// the FileSystem context.
state.expandedFilesets
.forEach((artifact, links) -> filesetMappings.put(artifact.getExecPath(), links));
try {
state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler, filesets);
state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler,
ImmutableMap.copyOf(filesetMappings));
} catch (IOException e) {
throw new ActionExecutionException(
"Failed to update filesystem context: ", e, action, /*catastrophe=*/ false);
Expand All @@ -498,7 +496,8 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
perActionFileCache,
metadataHandler,
Collections.unmodifiableMap(state.expandedArtifacts),
filesets,
Collections.unmodifiableMap(state.expandedFilesets),
topLevelFilesets,
state.actionFileSystem,
skyframeDepsResult)) {
if (!state.hasExecutedAction()) {
Expand Down Expand Up @@ -649,15 +648,32 @@ public SkyKey apply(Artifact artifact) {
}
}

private static class CheckInputResults {
/** Metadata about Artifacts consumed by this Action. */
private final ActionInputMap actionInputMap;
/** Artifact expansion mapping for Runfiles tree and tree artifacts. */
private final Map<Artifact, Collection<Artifact>> expandedArtifacts;
/** Artifact expansion mapping for Filesets embedded in Runfiles. */
private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets;

public CheckInputResults(ActionInputMap actionInputMap,
Map<Artifact, Collection<Artifact>> expandedArtifacts,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) {
this.actionInputMap = actionInputMap;
this.expandedArtifacts = expandedArtifacts;
this.expandedFilesets = expandedFilesets;
}
}

/**
* Declare dependency on all known inputs of action. Throws exception if any are known to be
* missing. Some inputs may not yet be in the graph, in which case the builder should abort.
*/
private Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs(
private CheckInputResults checkInputs(
Environment env,
Action action,
Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps)
throws ActionExecutionException {
throws ActionExecutionException, InterruptedException {
int missingCount = 0;
int actionFailures = 0;
// Only populate input data if we have the input values, otherwise they'll just go unused.
Expand All @@ -669,6 +685,7 @@ private Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs(
ActionInputMap inputArtifactData = new ActionInputMap(populateInputData ? inputDeps.size() : 0);
Map<Artifact, Collection<Artifact>> expandedArtifacts =
new HashMap<>(populateInputData ? 128 : 0);
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = new HashMap<>();

ActionExecutionException firstActionExecutionException = null;
for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>>
Expand All @@ -677,7 +694,13 @@ private Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs(
try {
SkyValue value = depsEntry.getValue().get();
if (populateInputData) {
ActionInputMapHelper.addToMap(inputArtifactData, expandedArtifacts, input, value);
ActionInputMapHelper.addToMap(
inputArtifactData,
expandedArtifacts,
expandedFilesets,
input,
value,
env);
}
} catch (MissingInputFileException e) {
missingCount++;
Expand Down Expand Up @@ -726,7 +749,7 @@ private Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs(
rootCauses.build(),
/*catastrophe=*/ false);
}
return Pair.of(inputArtifactData, expandedArtifacts);
return new CheckInputResults(inputArtifactData, expandedArtifacts, expandedFilesets);
}

private static Iterable<Artifact> filterKnownInputs(
Expand Down Expand Up @@ -782,6 +805,7 @@ private static class ContinuationState {
ActionInputMap inputArtifactData = null;

Map<Artifact, Collection<Artifact>> expandedArtifacts = null;
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = null;
Token token = null;
Iterable<Artifact> discoveredInputs = null;
ActionExecutionValue value = null;
Expand Down
40 changes: 37 additions & 3 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,39 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionInputMap;
import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Collection;
import java.util.Map;

class ActionInputMapHelper {

// Adds a value obtained by an Artifact skyvalue lookup to the action input map
// Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe
// lookups.
static void addToMap(
ActionInputMap inputMap,
Map<Artifact, Collection<Artifact>> expandedArtifacts,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets,
Artifact key,
SkyValue value) {
SkyValue value,
Environment env) throws InterruptedException {
if (value instanceof AggregatingArtifactValue) {
AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getFileArtifacts()) {
inputMap.put(entry.first, entry.second);
Artifact artifact = entry.first;
inputMap.put(artifact, entry.second);
if (artifact.isFileset()) {
ImmutableList<FilesetOutputSymlink> expandedFileset = getFilesets(env, artifact);
if (expandedFileset != null) {
expandedFilesets.put(artifact, expandedFileset);
}
}
}
for (Pair<Artifact, TreeArtifactValue> entry : aggregatingValue.getTreeArtifacts()) {
expandTreeArtifactAndPopulateArtifactData(
Expand Down Expand Up @@ -70,6 +84,26 @@ static void addToMap(
}
}

static ImmutableList<FilesetOutputSymlink> getFilesets(Environment env,
Artifact actionInput) throws InterruptedException {
Preconditions.checkState(actionInput.isFileset(), actionInput);
ActionLookupKey filesetActionLookupKey = (ActionLookupKey) actionInput.getArtifactOwner();
// Index 0 for the Fileset ConfiguredTarget indicates the SkyframeFilesetManifestAction where
// we compute the fileset's outputSymlinks.
SkyKey filesetActionKey = ActionExecutionValue.key(filesetActionLookupKey, 0);
ActionExecutionValue filesetValue = (ActionExecutionValue) env.getValue(filesetActionKey);
if (filesetValue == null) {
// At this point skyframe does not guarantee that the filesetValue will be ready, since
// the current action does not directly depend on the outputs of the
// SkyframeFilesetManifestAction whose ActionExecutionValue (filesetValue) is needed here.
// TODO(kush): Get rid of this hack by making the outputSymlinks available in the Fileset
// artifact, which this action depends on, so its value will be guaranteed to be present.
// Also, unify handling of Fileset with Artifact expansion.
return null;
}
return filesetValue.getOutputSymlinks();
}

private static void expandTreeArtifactAndPopulateArtifactData(
Artifact treeArtifact,
TreeArtifactValue value,
Expand Down
12 changes: 11 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionInputMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ArtifactSkyKey;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
Expand Down Expand Up @@ -326,9 +328,11 @@ public SkyValue compute(SkyKey skyKey, Environment env)
boolean createPathResolver = pathResolverFactory.shouldCreatePathResolverForArtifactValues();
ActionInputMap inputMap = null;
Map<Artifact, Collection<Artifact>> expandedArtifacts = null;
Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = null;
if (createPathResolver) {
inputMap = new ActionInputMap(inputDeps.size());
expandedArtifacts = new HashMap<>();
expandedFilesets = new HashMap<>();
}

int missingCount = 0;
Expand All @@ -341,7 +345,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
try {
SkyValue artifactValue = depsEntry.getValue().get();
if (createPathResolver && artifactValue != null) {
ActionInputMapHelper.addToMap(inputMap, expandedArtifacts, input, artifactValue);
ActionInputMapHelper.addToMap(
inputMap,
expandedArtifacts,
expandedFilesets,
input,
artifactValue,
env);
}
} catch (MissingInputFileException e) {
missingCount++;
Expand Down
Loading

0 comments on commit 8dece49

Please sign in to comment.