Skip to content

Commit

Permalink
Consider each runfiles tree to be a distinct node when walking the sp…
Browse files Browse the repository at this point in the history 
…awn inputs.

This is a similar idea to 0f55d12, where we did the same for tree artifacts.

Currently, the sole beneficiary of this change is the Merkle tree cache, which becomes more efficient in the presence of runfiles trees shared by multiple actions. In the future, we might reuse this code path to produce a more compact execution log.

PiperOrigin-RevId: 585582119
Change-Id: I2caec62eb26cab6dc2530889ef075acc1d9e93d9
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Nov 27, 2023
1 parent 62024d6 commit 8947322
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 45 deletions.
24 changes: 24 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/SimpleSpawn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,30 @@ public SimpleSpawn(
localResourcesSupplier);
}

public SimpleSpawn(
ActionExecutionMetadata owner,
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
RunfilesSupplier runfilesSupplier,
NestedSet<? extends ActionInput> inputs,
Collection<? extends ActionInput> outputs,
ResourceSet localResources) {
this(
owner,
arguments,
environment,
executionInfo,
runfilesSupplier,
/* filesetMappings= */ ImmutableMap.of(),
inputs,
/* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
outputs,
/* mandatoryOutputs= */ null,
localResources,
/* localResourcesSupplier= */ null);
}

public SimpleSpawn(
ActionExecutionMetadata owner,
ImmutableList<String> arguments,
Expand Down
107 changes: 75 additions & 32 deletions src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
Expand Down Expand Up @@ -124,19 +125,37 @@ private static void addMapping(
void addRunfilesToInputs(
Map<PathFragment, ActionInput> inputMap,
RunfilesSupplier runfilesSupplier,
InputMetadataProvider actionFileCache,
InputMetadataProvider inputMetadataProvider,
ArtifactExpander artifactExpander,
PathMapper pathMapper,
PathFragment baseDirectory)
throws IOException, ForbiddenActionInputException {
Map<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
runfilesSupplier.getMappings();

for (Map.Entry<PathFragment, Map<PathFragment, Artifact>> rootAndMappings :
rootsAndMappings.entrySet()) {
PathFragment root = rootAndMappings.getKey();
Preconditions.checkState(!root.isAbsolute(), root);
for (Map.Entry<PathFragment, Artifact> mapping : rootAndMappings.getValue().entrySet()) {
addSingleRunfilesTreeToInputs(
inputMap,
rootAndMappings.getKey(),
rootAndMappings.getValue(),
inputMetadataProvider,
artifactExpander,
pathMapper,
baseDirectory);
}
}

private void addSingleRunfilesTreeToInputs(
Map<PathFragment, ActionInput> inputMap,
PathFragment root,
Map<PathFragment, Artifact> mappings,
InputMetadataProvider inputMetadataProvider,
ArtifactExpander artifactExpander,
PathMapper pathMapper,
PathFragment baseDirectory)
throws IOException, ForbiddenActionInputException {
Preconditions.checkState(!root.isAbsolute(), root);
for (Map.Entry<PathFragment, Artifact> mapping : mappings.entrySet()) {
PathFragment location = root.getRelative(mapping.getKey());
Artifact localArtifact = mapping.getValue();
if (localArtifact != null) {
Expand Down Expand Up @@ -175,18 +194,17 @@ void addRunfilesToInputs(
addFilesetManifest(location, localArtifact, filesetLinks, inputMap, baseDirectory);
} else {
if (strict) {
failIfDirectory(actionFileCache, localArtifact);
failIfDirectory(inputMetadataProvider, localArtifact);
}
addMapping(
inputMap, mapForRunfiles(pathMapper, root, location), localArtifact, baseDirectory);
}
} else {
addMapping(
inputMap,
mapForRunfiles(pathMapper, root, location),
VirtualActionInput.EMPTY_MARKER,
baseDirectory);
}
addMapping(
inputMap,
mapForRunfiles(pathMapper, root, location),
VirtualActionInput.EMPTY_MARKER,
baseDirectory);
}
}
}
Expand Down Expand Up @@ -366,31 +384,19 @@ public void walkInputs(
Spawn spawn,
ArtifactExpander artifactExpander,
PathFragment baseDirectory,
InputMetadataProvider actionInputFileCache,
InputMetadataProvider inputMetadataProvider,
InputVisitor visitor)
throws IOException, ForbiddenActionInputException {
walkNestedSetInputs(
baseDirectory, spawn.getInputFiles(), artifactExpander, spawn.getPathMapper(), visitor);

RunfilesSupplier runfilesSupplier = spawn.getRunfilesSupplier();
visitor.visit(
// Cache key for the sub-mapping containing the runfiles inputs for this spawn.
ImmutableList.of(runfilesSupplier, baseDirectory, spawn.getPathMapper().cacheKey()),
new InputWalker() {
@Override
public SortedMap<PathFragment, ActionInput> getLeavesInputMapping()
throws IOException, ForbiddenActionInputException {
TreeMap<PathFragment, ActionInput> inputMap = new TreeMap<>();
addRunfilesToInputs(
inputMap,
runfilesSupplier,
actionInputFileCache,
artifactExpander,
spawn.getPathMapper(),
baseDirectory);
return inputMap;
}
});
walkRunfilesInputs(
baseDirectory,
spawn.getRunfilesSupplier(),
inputMetadataProvider,
artifactExpander,
spawn.getPathMapper(),
visitor);

Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings = spawn.getFilesetMappings();
// filesetMappings is assumed to be very small, so no need to implement visitNonLeaves() for
Expand All @@ -409,6 +415,43 @@ public SortedMap<PathFragment, ActionInput> getLeavesInputMapping()
});
}

/** Visits {@link Spawn#getRunfilesSupplier}. */
private void walkRunfilesInputs(
PathFragment baseDirectory,
RunfilesSupplier runfilesSupplier,
InputMetadataProvider inputMetadataProvider,
ArtifactExpander artifactExpander,
PathMapper pathMapper,
InputVisitor visitor)
throws IOException, ForbiddenActionInputException {
ImmutableMap<PathFragment, Map<PathFragment, Artifact>> rootsAndMappings =
runfilesSupplier.getMappings();
for (Map.Entry<PathFragment, Map<PathFragment, Artifact>> rootAndMappings :
rootsAndMappings.entrySet()) {
PathFragment root = rootAndMappings.getKey();
Map<PathFragment, Artifact> mappings = rootAndMappings.getValue();
visitor.visit(
// Cache key for the sub-mapping containing this runfiles tree.
ImmutableList.of(root, baseDirectory, pathMapper.cacheKey()),
new InputWalker() {
@Override
public SortedMap<PathFragment, ActionInput> getLeavesInputMapping()
throws IOException, ForbiddenActionInputException {
TreeMap<PathFragment, ActionInput> inputMap = new TreeMap<>();
addSingleRunfilesTreeToInputs(
inputMap,
root,
mappings,
inputMetadataProvider,
artifactExpander,
pathMapper,
baseDirectory);
return inputMap;
}
});
}
}

/** Visits a {@link NestedSet} occurring in {@link Spawn#getInputFiles}. */
private void walkNestedSetInputs(
PathFragment baseDirectory,
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,10 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
"//src/main/java/com/google/devtools/build/lib/actions:runfiles_supplier",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
"//src/main/java/com/google/devtools/build/lib/authandtls",
Expand Down
71 changes: 58 additions & 13 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
Expand Down Expand Up @@ -65,16 +66,17 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.actions.StaticInputMetadataProvider;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
import com.google.devtools.build.lib.clock.JavaClock;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
Expand Down Expand Up @@ -118,6 +120,8 @@
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -2050,16 +2054,6 @@ public void buildMerkleTree_withMemoization_works() throws Exception {
// TODO: Would like to check that NestedSet.getNonLeaves() is only called once per node, but
// cannot Mockito.spy on NestedSet as it is final.

// arrange
/*
* First:
* /bar/file
* /foo1/file
* Second:
* /bar/file
* /foo2/file
*/

// arrange
// Single node NestedSets are folded, so always add a dummy file everywhere.
ActionInput dummyFile = ActionInputHelper.fromPath("file");
Expand Down Expand Up @@ -2097,24 +2091,34 @@ public void buildMerkleTree_withMemoization_works() throws Exception {
.addTransitive(nodeFoo2)
.build();

Artifact toolDat = ActionsTestUtil.createArtifact(artifactRoot, "tool.dat");
fakeFileCache.createScratchInput(toolDat, "tool.dat");

RunfilesSupplier runfilesSupplier =
createRunfilesSupplier("tools/tool.runfiles", ImmutableList.of(toolDat));

Spawn spawn1 =
new SimpleSpawn(
new FakeOwner("foo", "bar", "//dummy:label"),
/* arguments= */ ImmutableList.of(),
/* environment= */ ImmutableMap.of(),
/* executionInfo= */ ImmutableMap.of(),
/* runfilesSupplier= */ runfilesSupplier,
/* inputs= */ nodeRoot1,
/* outputs= */ ImmutableSet.of(),
ResourceSet.ZERO);

Spawn spawn2 =
new SimpleSpawn(
new FakeOwner("foo", "bar", "//dummy:label"),
/* arguments= */ ImmutableList.of(),
/* environment= */ ImmutableMap.of(),
/* executionInfo= */ ImmutableMap.of(),
/* runfilesSupplier= */ runfilesSupplier,
/* inputs= */ nodeRoot2,
/* outputs= */ ImmutableSet.of(),
ResourceSet.ZERO);

FakeSpawnExecutionContext context1 = newSpawnExecutionContext(spawn1);
FakeSpawnExecutionContext context2 = newSpawnExecutionContext(spawn2);
remoteOptions.remoteMerkleTreeCache = true;
Expand All @@ -2133,7 +2137,7 @@ public void buildMerkleTree_withMemoization_works() throws Exception {
PathFragment.EMPTY_FRAGMENT,
PathMapper.NOOP.getClass()), // fileset mapping
ImmutableList.of(
EmptyRunfilesSupplier.INSTANCE,
PathFragment.create("tools/tool.runfiles"),
PathFragment.EMPTY_FRAGMENT,
PathMapper.NOOP.getClass()),
ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT, PathMapper.NOOP.getClass()),
Expand All @@ -2156,7 +2160,7 @@ public void buildMerkleTree_withMemoization_works() throws Exception {
PathFragment.EMPTY_FRAGMENT,
PathMapper.NOOP.getClass()), // fileset mapping
ImmutableList.of(
EmptyRunfilesSupplier.INSTANCE,
PathFragment.create("tools/tool.runfiles"),
PathFragment.EMPTY_FRAGMENT,
PathMapper.NOOP.getClass()),
ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT, PathMapper.NOOP.getClass()),
Expand Down Expand Up @@ -2496,6 +2500,47 @@ private RemoteExecutionService newRemoteExecutionService(RemoteOptions remoteOpt
remoteOutputChecker);
}

private RunfilesSupplier createRunfilesSupplier(String root, Collection<Artifact> artifacts) {
return new RunfilesSupplier() {
@Override
public NestedSet<Artifact> getAllArtifacts() {
throw new UnsupportedOperationException();
}

@Override
public ImmutableSet<PathFragment> getRunfilesDirs() {
throw new UnsupportedOperationException();
}

@Override
public ImmutableMap<PathFragment, Map<PathFragment, Artifact>> getMappings() {
return ImmutableMap.of(
PathFragment.create(root),
artifacts.stream().collect(toImmutableMap(Artifact::getExecPath, a -> a)));
}

@Override
public ImmutableList<Artifact> getManifests() {
throw new UnsupportedOperationException();
}

@Override
public RunfileSymlinksMode getRunfileSymlinksMode(PathFragment runfilesDir) {
throw new UnsupportedOperationException();
}

@Override
public boolean isBuildRunfileLinks(PathFragment runfilesDir) {
throw new UnsupportedOperationException();
}

@Override
public RunfilesSupplier withOverriddenRunfilesDir(PathFragment newRunfilesDir) {
throw new UnsupportedOperationException();
}
};
}

private void createOutputDirectories(Spawn spawn) throws IOException {
for (ActionInput input : spawn.getOutputFiles()) {
Path dir = execRoot.getRelative(input.getExecPath());
Expand Down

0 comments on commit 8947322

Please sign in to comment.