Skip to content

Commit

Permalink
Introduce FilesetOutputTree.
Browse files Browse the repository at this point in the history
This is a mechanical refactoring to replace `ImmutableList<FilesetOutputSymlink>` with an extra abstraction layer called `FilesetOutputTree`.

In this change, `FilesetOutputTree` only contains `ImmutableList<FilesetOutputSymlink>`. In a subsequent change, we will also store a `boolean` indicating whether the fileset has any relative symlinks. If the fileset does not have any relative symlinks (expected to be a common case), we can make some significant performance optimizations.

PiperOrigin-RevId: 681116734
Change-Id: If49f8acf15122f6287cd426cb36a081952424b84
  • Loading branch information
justinhorvitz authored and copybara-github committed Oct 1, 2024
1 parent 96f48e9 commit fd67506
Show file tree
Hide file tree
Showing 45 changed files with 340 additions and 247 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -211,14 +211,14 @@ public enum ShowSubcommands {
private final FileOutErr fileOutErr;
private final ExtendedEventHandler eventHandler;
private final ImmutableMap<String, String> clientEnv;
private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets;
private final ImmutableMap<Artifact, FilesetOutputTree> topLevelFilesets;
@Nullable private final ArtifactExpander artifactExpander;
@Nullable private final Environment env;

@Nullable private final FileSystem actionFileSystem;
@Nullable private final Object skyframeDepsResult;

private ImmutableList<FilesetOutputSymlink> outputSymlinks = ImmutableList.of();
private FilesetOutputTree filesetOutput = FilesetOutputTree.EMPTY;

private final ArtifactPathResolver pathResolver;
private final DiscoveredModulesPruner discoveredModulesPruner;
Expand All @@ -236,7 +236,7 @@ private ActionExecutionContext(
FileOutErr fileOutErr,
ExtendedEventHandler eventHandler,
Map<String, String> clientEnv,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
ImmutableMap<Artifact, FilesetOutputTree> topLevelFilesets,
@Nullable ArtifactExpander artifactExpander,
@Nullable Environment env,
@Nullable FileSystem actionFileSystem,
Expand Down Expand Up @@ -278,7 +278,7 @@ public ActionExecutionContext(
FileOutErr fileOutErr,
ExtendedEventHandler eventHandler,
Map<String, String> clientEnv,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
ImmutableMap<Artifact, FilesetOutputTree> topLevelFilesets,
ArtifactExpander artifactExpander,
@Nullable FileSystem actionFileSystem,
@Nullable Object skyframeDepsResult,
Expand Down Expand Up @@ -425,21 +425,21 @@ public ExtendedEventHandler getEventHandler() {
return eventHandler;
}

public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getTopLevelFilesets() {
public ImmutableMap<Artifact, FilesetOutputTree> getTopLevelFilesets() {
return topLevelFilesets;
}

public ImmutableList<FilesetOutputSymlink> getOutputSymlinks() {
return outputSymlinks;
public FilesetOutputTree getFilesetOutput() {
return filesetOutput;
}

public void setOutputSymlinks(ImmutableList<FilesetOutputSymlink> outputSymlinks) {
public void setFilesetOutput(FilesetOutputTree filesetOutput) {
checkState(
this.outputSymlinks.isEmpty(),
"Unexpected reassignment of the outputSymlinks of a Fileset from\n:%s to:\n%s",
this.outputSymlinks,
outputSymlinks);
this.outputSymlinks = checkNotNull(outputSymlinks);
this.filesetOutput.isEmpty(),
"Unexpected reassignment of Fileset output from\n:%s to:\n%s",
this.filesetOutput,
filesetOutput);
this.filesetOutput = checkNotNull(filesetOutput);
}

@Override
Expand Down Expand Up @@ -586,7 +586,7 @@ public ActionExecutionContext withOutputsAsInputs(
ImmutableMap.builder();

for (ActionInput input : additionalInputs) {
additionalInputMap.put(input, getOutputMetadataStore().getOutputMetadata(input));
additionalInputMap.put(input, outputMetadataStore.getOutputMetadata(input));
}

StaticInputMetadataProvider additionalInputMetadata =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package com.google.devtools.build.lib.actions;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
Expand Down Expand Up @@ -52,8 +51,7 @@ default ImmutableSortedSet<TreeFileArtifact> tryExpandTreeArtifact(Artifact tree
* @throws MissingExpansionException if the expander is missing data needed to expand provided
* fileset.
*/
default ImmutableList<FilesetOutputSymlink> expandFileset(Artifact fileset)
throws MissingExpansionException {
default FilesetOutputTree expandFileset(Artifact fileset) throws MissingExpansionException {
throw new MissingExpansionException("Cannot expand fileset " + fileset);
}

Expand Down
12 changes: 11 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ java_library(
":execution_requirements",
":file_metadata",
":fileset_output_symlink",
":fileset_output_tree",
":localhost_capacity",
":middleman_type",
":package_roots",
Expand Down Expand Up @@ -359,7 +360,7 @@ java_library(
srcs = ["ArtifactExpander.java"],
deps = [
":artifacts",
":fileset_output_symlink",
":fileset_output_tree",
"//third_party:guava",
"//third_party:jsr305",
],
Expand Down Expand Up @@ -398,6 +399,15 @@ java_library(
deps = ["//src/main/protobuf:failure_details_java_proto"],
)

java_library(
name = "fileset_output_tree",
srcs = ["FilesetOutputTree.java"],
deps = [
":fileset_output_symlink",
"//third_party:guava",
],
)

java_library(
name = "fileset_output_symlink",
srcs = ["FilesetOutputSymlink.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public ImmutableList<String> getArguments() {
}

@Override
public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMappings() {
public ImmutableMap<Artifact, FilesetOutputTree> getFilesetMappings() {
return ImmutableMap.of();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public final class CompletionContext implements ArtifactExpander {
private final Path execRoot;
private final ArtifactPathResolver pathResolver;
private final Map<Artifact, TreeArtifactValue> treeArtifacts;
private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets;
private final Map<Artifact, FilesetOutputTree> filesets;
@Nullable private final FileArtifactValue baselineCoverageValue;
// Only contains the metadata for 'important' artifacts of the Target/Aspect that completed. Any
// 'unimportant' artifacts produced by internal output groups (most importantly, _validation) will
Expand All @@ -70,7 +70,7 @@ public final class CompletionContext implements ArtifactExpander {
public CompletionContext(
Path execRoot,
Map<Artifact, TreeArtifactValue> treeArtifacts,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets,
Map<Artifact, FilesetOutputTree> filesets,
@Nullable FileArtifactValue baselineCoverageValue,
ArtifactPathResolver pathResolver,
ActionInputMap importantInputMap,
Expand All @@ -88,7 +88,7 @@ public CompletionContext(

public static CompletionContext create(
Map<Artifact, TreeArtifactValue> treeArtifacts,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets,
Map<Artifact, FilesetOutputTree> filesets,
@Nullable FileArtifactValue baselineCoverageValue,
boolean expandFilesets,
boolean fullyResolveFilesetSymlinks,
Expand Down Expand Up @@ -124,7 +124,7 @@ public ActionInputMap getImportantInputMap() {
return importantInputMap;
}

public Map<Artifact, ImmutableList<FilesetOutputSymlink>> getExpandedFilesets() {
public Map<Artifact, FilesetOutputTree> getExpandedFilesets() {
return filesets;
}

Expand Down Expand Up @@ -174,7 +174,7 @@ public void visitArtifacts(Iterable<Artifact> artifacts, ArtifactReceiver receiv
}

private void visitFileset(Artifact filesetArtifact, ArtifactReceiver receiver) {
ImmutableList<FilesetOutputSymlink> links = filesets.get(filesetArtifact);
ImmutableList<FilesetOutputSymlink> links = filesets.get(filesetArtifact).symlinks();
FilesetManifest filesetManifest =
FilesetManifest.constructFilesetManifestWithoutError(
links,
Expand Down Expand Up @@ -207,15 +207,14 @@ public ArchivedTreeArtifact getArchivedTreeArtifact(Artifact treeArtifact) {
}

@Override
public ImmutableList<FilesetOutputSymlink> expandFileset(Artifact fileset)
throws MissingExpansionException {
public FilesetOutputTree expandFileset(Artifact fileset) throws MissingExpansionException {
checkArgument(fileset.isFileset(), fileset);
checkState(expandFilesets, "Fileset expansion disabled, cannot expand %s", fileset);
ImmutableList<FilesetOutputSymlink> links = filesets.get(fileset);
if (links == null) {
FilesetOutputTree filesetOutput = filesets.get(fileset);
if (filesetOutput == null) {
throw new MissingExpansionException("Missing expansion for fileset: " + fileset);
}
return links;
return filesetOutput;
}

/** A function that accepts an {@link Artifact}. */
Expand All @@ -230,7 +229,7 @@ public interface PathResolverFactory {
ArtifactPathResolver createPathResolverForArtifactValues(
ActionInputMap actionInputMap,
Map<Artifact, ImmutableSortedSet<TreeFileArtifact>> treeArtifacts,
Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets,
Map<Artifact, FilesetOutputTree> filesets,
String workspaceName);

boolean shouldCreatePathResolverForArtifactValues();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright 2024 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.actions;

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;

/** A collection of {@link FilesetOutputSymlink}s comprising the output tree of a fileset. */
public final class FilesetOutputTree {

public static final FilesetOutputTree EMPTY = new FilesetOutputTree(ImmutableList.of());

public static FilesetOutputTree create(ImmutableList<FilesetOutputSymlink> symlinks) {
return symlinks.isEmpty() ? EMPTY : new FilesetOutputTree(symlinks);
}

private final ImmutableList<FilesetOutputSymlink> symlinks;

private FilesetOutputTree(ImmutableList<FilesetOutputSymlink> symlinks) {
this.symlinks = checkNotNull(symlinks);
}

public ImmutableList<FilesetOutputSymlink> symlinks() {
return symlinks;
}

public int size() {
return symlinks.size();
}

public boolean isEmpty() {
return symlinks.isEmpty();
}

@Override
public int hashCode() {
return symlinks.hashCode();
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof FilesetOutputTree that)) {
return false;
}
return symlinks.equals(that.symlinks);
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this).add("symlinks", symlinks).toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public final class SimpleSpawn implements Spawn {
private final ImmutableMap<String, String> executionInfo;
private final NestedSet<? extends ActionInput> inputs;
private final NestedSet<? extends ActionInput> tools;
private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings;
private final ImmutableMap<Artifact, FilesetOutputTree> filesetMappings;
private final ImmutableList<ActionInput> outputs;
// If null, all outputs are mandatory.
@Nullable private final Set<? extends ActionInput> mandatoryOutputs;
Expand All @@ -50,7 +50,7 @@ private SimpleSpawn(
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
ImmutableMap<Artifact, FilesetOutputTree> filesetMappings,
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
Collection<? extends ActionInput> outputs,
Expand Down Expand Up @@ -87,7 +87,7 @@ public SimpleSpawn(
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
ImmutableMap<Artifact, FilesetOutputTree> filesetMappings,
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
Collection<? extends ActionInput> outputs,
Expand All @@ -114,7 +114,7 @@ public SimpleSpawn(
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
ImmutableMap<Artifact, FilesetOutputTree> filesetMappings,
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
Collection<? extends ActionInput> outputs,
Expand All @@ -140,7 +140,7 @@ public SimpleSpawn(
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
ImmutableMap<Artifact, FilesetOutputTree> filesetMappings,
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
Collection<? extends ActionInput> outputs,
Expand All @@ -167,7 +167,7 @@ public SimpleSpawn(
ImmutableList<String> arguments,
ImmutableMap<String, String> environment,
ImmutableMap<String, String> executionInfo,
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetMappings,
ImmutableMap<Artifact, FilesetOutputTree> filesetMappings,
NestedSet<? extends ActionInput> inputs,
NestedSet<? extends ActionInput> tools,
Collection<? extends ActionInput> outputs,
Expand Down Expand Up @@ -247,7 +247,7 @@ public ImmutableMap<String, String> getEnvironment() {
}

@Override
public ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMappings() {
public ImmutableMap<Artifact, FilesetOutputTree> getFilesetMappings() {
return filesetMappings;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public interface Spawn extends DescribableExecutionUnit {
* Map of the execpath at which we expect the Fileset symlink trees, to a list of
* FilesetOutputSymlinks which contains the details of the Symlink trees.
*/
ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> getFilesetMappings();
ImmutableMap<Artifact, FilesetOutputTree> getFilesetMappings();

/**
* Returns the list of files that are required to execute this spawn (e.g. the compiler binary),
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//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:fileset_output_symlink",
"//src/main/java/com/google/devtools/build/lib/actions:fileset_output_tree",
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
"//src/main/java/com/google/devtools/build/lib/actions:package_roots",
"//src/main/java/com/google/devtools/build/lib/actions:runfiles_tree",
Expand Down
Loading

0 comments on commit fd67506

Please sign in to comment.