Skip to content

Commit

Permalink
Make CompactSpawnLogContext not call Spawn.getRunfilesSupplier().
Browse files Browse the repository at this point in the history 
As usual, this is achieved by findng the RunfilesTree instance by asking the InputMetadataProvider.

RELNOTES: None.
PiperOrigin-RevId: 608335415
Change-Id: I482cabb734a312f93e940935e4a24559acf4afd2
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Feb 19, 2024
1 parent a2ebdf7 commit fda4aff
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 39 deletions.
30 changes: 16 additions & 14 deletions src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.InputMetadataProvider;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
Expand Down Expand Up @@ -265,21 +264,13 @@ private int logInputs(
Spawn spawn, InputMetadataProvider inputMetadataProvider, FileSystem fileSystem)
throws IOException, InterruptedException {

// Add runfiles and filesets as additional direct members of the top-level nested set of inputs.
// This prevents it from being shared, but experimentally, the top-level input nested set for a
// spawn is almost never a transitive member of other nested sets, and not recording its entry
// ID turns out to be a very significant memory optimization.
// Add filesets as additional direct members of the top-level nested set of inputs. This
// prevents it from being shared, but experimentally, the top-level input nested set for a spawn
// is almost never a transitive member of other nested sets, and not recording its entry ID
// turns out to be a very significant memory optimization.

ImmutableList.Builder<Integer> additionalDirectoryIds = ImmutableList.builder();

RunfilesSupplier runfilesSupplier = spawn.getRunfilesSupplier();
for (RunfilesTree tree : runfilesSupplier.getRunfilesTrees()) {
// The runfiles symlink tree might not have been materialized on disk, so use the mapping.
additionalDirectoryIds.add(
logRunfilesDirectory(
tree.getExecPath(), tree.getMapping(), inputMetadataProvider, fileSystem));
}

for (Artifact fileset : spawn.getFilesetMappings().keySet()) {
// The fileset symlink tree is always materialized on disk.
additionalDirectoryIds.add(
Expand Down Expand Up @@ -353,14 +344,25 @@ private int logNestedSet(
}

for (ActionInput input : set.getLeaves()) {
// Runfiles are logged separately.
if (input instanceof Artifact && ((Artifact) input).isMiddlemanArtifact()) {
RunfilesTree runfilesTree =
inputMetadataProvider.getRunfilesMetadata(input).getRunfilesTree();
builder.addDirectoryIds(
// The runfiles symlink tree might not have been materialized on disk, so use the
// mapping.
logRunfilesDirectory(
runfilesTree.getExecPath(),
runfilesTree.getMapping(),
inputMetadataProvider,
fileSystem));
continue;
}

// Filesets are logged separately.
if (input instanceof Artifact && ((Artifact) input).isFileset()) {
continue;
}

Path path = fileSystem.getPath(execRoot.getRelative(input.getExecPath()));
if (isInputDirectory(input, inputMetadataProvider)) {
builder.addDirectoryIds(logDirectory(input, path, inputMetadataProvider));
Expand Down
65 changes: 40 additions & 25 deletions src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
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.FileArtifactValue;
import com.google.devtools.build.lib.actions.InputMetadataProvider;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnMetrics;
Expand All @@ -47,6 +45,7 @@
import com.google.devtools.build.lib.exec.Protos.File;
import com.google.devtools.build.lib.exec.Protos.Platform;
import com.google.devtools.build.lib.exec.Protos.SpawnExec;
import com.google.devtools.build.lib.exec.util.FakeActionInputFileCache;
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
import com.google.devtools.build.lib.server.FailureDetails.Crash;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
Expand Down Expand Up @@ -76,6 +75,8 @@ public abstract class SpawnLogContextTestBase {
protected final ArtifactRoot rootDir = ArtifactRoot.asSourceRoot(Root.fromPath(execRoot));
protected final ArtifactRoot outputDir =
ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "out");
protected final ArtifactRoot middlemanDir =
ArtifactRoot.asDerivedRoot(execRoot, RootType.Middleman, "middlemen");

// A fake action filesystem that provides a fast digest, but refuses to compute it from the
// file contents (which won't be available when building without the bytes).
Expand Down Expand Up @@ -286,21 +287,26 @@ public void testUnresolvedSymlinkInput(@TestParameter InputsMode inputsMode) thr
@Test
public void testRunfilesFileInput() throws Exception {
Artifact runfilesInput = ActionsTestUtil.createArtifact(rootDir, "data.txt");
Artifact runfilesMiddleman = ActionsTestUtil.createArtifact(middlemanDir, "runfiles");

writeFile(runfilesInput, "abc");

PathFragment runfilesRoot = outputDir.getExecPath().getRelative("foo.runfiles");
RunfilesSupplier runfilesSupplier =
createRunfilesSupplier(runfilesRoot, ImmutableMap.of("data.txt", runfilesInput));
RunfilesTree runfilesTree =
createRunfilesTree(runfilesRoot, ImmutableMap.of("data.txt", runfilesInput));

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn = defaultSpawnBuilder().withInput(runfilesMiddleman).build();

SpawnLogContext context = createSpawnLogContext();

FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache();
inputMetadataProvider.putRunfilesTree(runfilesMiddleman, runfilesTree);
inputMetadataProvider.put(runfilesInput, FileArtifactValue.createForTesting(runfilesInput));

context.logSpawn(
spawn,
createInputMetadataProvider(runfilesInput),
createInputMap(runfilesSupplier),
inputMetadataProvider,
createInputMap(runfilesTree),
fs,
defaultTimeout(),
defaultSpawnResult());
Expand All @@ -315,6 +321,7 @@ public void testRunfilesFileInput() throws Exception {

@Test
public void testRunfilesDirectoryInput(@TestParameter DirContents dirContents) throws Exception {
Artifact runfilesMiddleman = ActionsTestUtil.createArtifact(middlemanDir, "runfiles");
Artifact runfilesInput = ActionsTestUtil.createArtifact(rootDir, "dir");

runfilesInput.getPath().createDirectoryAndParents();
Expand All @@ -323,17 +330,21 @@ public void testRunfilesDirectoryInput(@TestParameter DirContents dirContents) t
}

PathFragment runfilesRoot = outputDir.getExecPath().getRelative("foo.runfiles");
RunfilesSupplier runfilesSupplier =
createRunfilesSupplier(runfilesRoot, ImmutableMap.of("dir", runfilesInput));
RunfilesTree runfilesTree =
createRunfilesTree(runfilesRoot, ImmutableMap.of("dir", runfilesInput));

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn = defaultSpawnBuilder().withInput(runfilesMiddleman).build();

SpawnLogContext context = createSpawnLogContext();

FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache();
inputMetadataProvider.putRunfilesTree(runfilesMiddleman, runfilesTree);
inputMetadataProvider.put(runfilesInput, FileArtifactValue.createForTesting(runfilesInput));

context.logSpawn(
spawn,
createInputMetadataProvider(runfilesInput),
createInputMap(runfilesSupplier),
inputMetadataProvider,
createInputMap(runfilesTree),
fs,
defaultTimeout(),
defaultSpawnResult());
Expand All @@ -354,19 +365,23 @@ public void testRunfilesDirectoryInput(@TestParameter DirContents dirContents) t

@Test
public void testRunfilesEmptyInput() throws Exception {
Artifact runfilesMiddleman = ActionsTestUtil.createArtifact(middlemanDir, "runfiles");
PathFragment runfilesRoot = outputDir.getExecPath().getRelative("foo.runfiles");
HashMap<String, Artifact> mapping = new HashMap<>();
mapping.put("__init__.py", null);
RunfilesSupplier runfilesSupplier = createRunfilesSupplier(runfilesRoot, mapping);
RunfilesTree runfilesTree = createRunfilesTree(runfilesRoot, mapping);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn = defaultSpawnBuilder().withInput(runfilesMiddleman).build();

SpawnLogContext context = createSpawnLogContext();

FakeActionInputFileCache inputMetadataProvider = new FakeActionInputFileCache();
inputMetadataProvider.putRunfilesTree(runfilesMiddleman, runfilesTree);

context.logSpawn(
spawn,
createInputMetadataProvider(),
createInputMap(runfilesSupplier),
inputMetadataProvider,
createInputMap(runfilesTree),
fs,
defaultTimeout(),
defaultSpawnResult());
Expand Down Expand Up @@ -884,7 +899,7 @@ protected static SpawnExec.Builder defaultSpawnExecBuilder() {
.setMetrics(Protos.SpawnMetrics.getDefaultInstance());
}

protected static RunfilesSupplier createRunfilesSupplier(
protected static RunfilesTree createRunfilesTree(
PathFragment root, Map<String, Artifact> mapping) {
HashMap<PathFragment, Artifact> mappingByPath = new HashMap<>();
for (Map.Entry<String, Artifact> entry : mapping.entrySet()) {
Expand All @@ -893,9 +908,7 @@ protected static RunfilesSupplier createRunfilesSupplier(
RunfilesTree runfilesTree = mock(RunfilesTree.class);
when(runfilesTree.getExecPath()).thenReturn(root);
when(runfilesTree.getMapping()).thenReturn(mappingByPath);
RunfilesSupplier runfilesSupplier = mock(RunfilesSupplier.class);
when(runfilesSupplier.getRunfilesTrees()).thenReturn(ImmutableList.of(runfilesTree));
return runfilesSupplier;
return runfilesTree;
}

protected static InputMetadataProvider createInputMetadataProvider(Artifact... artifacts)
Expand All @@ -921,22 +934,24 @@ protected static InputMetadataProvider createInputMetadataProvider(Artifact... a

protected static SortedMap<PathFragment, ActionInput> createInputMap(Artifact... artifacts)
throws Exception {
return createInputMap(EmptyRunfilesSupplier.INSTANCE, artifacts);
return createInputMap(null, artifacts);
}

protected static SortedMap<PathFragment, ActionInput> createInputMap(
RunfilesSupplier runfilesSupplier, Artifact... artifacts) throws Exception {
RunfilesTree runfilesTree, Artifact... artifacts) throws Exception {
ImmutableSortedMap.Builder<PathFragment, ActionInput> builder =
ImmutableSortedMap.naturalOrder();
for (RunfilesTree tree : runfilesSupplier.getRunfilesTrees()) {

if (runfilesTree != null) {
// Emulate SpawnInputExpander: expand runfiles, replacing nulls with empty inputs.
PathFragment root = tree.getExecPath();
for (Map.Entry<PathFragment, Artifact> entry : tree.getMapping().entrySet()) {
PathFragment root = runfilesTree.getExecPath();
for (Map.Entry<PathFragment, Artifact> entry : runfilesTree.getMapping().entrySet()) {
PathFragment execPath = root.getRelative(entry.getKey());
Artifact artifact = entry.getValue();
builder.put(execPath, artifact != null ? artifact : VirtualActionInput.EMPTY_MARKER);
}
}

for (Artifact artifact : artifacts) {
if (artifact.isTreeArtifact()) {
// Emulate SpawnInputExpander: expand to children, preserve if empty.
Expand Down

0 comments on commit fda4aff

Please sign in to comment.