Store runfiles as parallel lists instead of a list of pairs.
The cost of the `Pair` object is greater than the additional array cost of storing the artifacts and metadata in separate lists.
Getter methods for lists of pairs on `RunfilesArtifactValue` are replaced with functional-style `forEach` methods.
PiperOrigin-RevId: 515635185
Change-Id: Iee0757f47b5cf202100a71d1a708682db9cc16a1
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index 10eb5f6..cddb489 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -28,7 +28,6 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Map;
@@ -85,33 +84,34 @@
// Instead, the way the SpawnInputExpander expands runfiles is via the Runfiles class
// which contains all artifacts in the runfiles tree minus the MANIFEST file.
RunfilesArtifactValue runfilesArtifactValue = (RunfilesArtifactValue) value;
- for (Pair<Artifact, FileArtifactValue> entry : runfilesArtifactValue.getFileArtifacts()) {
- Artifact artifact = entry.first;
- inputMap.put(artifact, entry.getSecond(), /*depOwner=*/ key);
- if (artifact.isFileset()) {
- ImmutableList<FilesetOutputSymlink> expandedFileset =
- getFilesets(env, (SpecialArtifact) artifact);
- if (expandedFileset != null) {
- filesetsInsideRunfiles.put(artifact, expandedFileset);
- consumer.accumulate(expandedFileset);
- }
- } else {
- consumer.accumulate(entry.getSecond());
- }
- }
- for (Pair<Artifact, TreeArtifactValue> entry : runfilesArtifactValue.getTreeArtifacts()) {
- expandTreeArtifactAndPopulateArtifactData(
- entry.getFirst(),
- Preconditions.checkNotNull(entry.getSecond()),
- expandedArtifacts,
- archivedTreeArtifacts,
- inputMap,
- /*depOwner=*/ key);
- consumer.accumulate(entry.getSecond());
- }
+ runfilesArtifactValue.forEachFile(
+ (artifact, metadata) -> {
+ inputMap.put(artifact, metadata, /* depOwner= */ key);
+ if (artifact.isFileset()) {
+ ImmutableList<FilesetOutputSymlink> expandedFileset =
+ getFilesets(env, (SpecialArtifact) artifact);
+ if (expandedFileset != null) {
+ filesetsInsideRunfiles.put(artifact, expandedFileset);
+ consumer.accumulate(expandedFileset);
+ }
+ } else {
+ consumer.accumulate(metadata);
+ }
+ });
+ runfilesArtifactValue.forEachTree(
+ (treeArtifact, metadata) -> {
+ expandTreeArtifactAndPopulateArtifactData(
+ treeArtifact,
+ metadata,
+ expandedArtifacts,
+ archivedTreeArtifacts,
+ inputMap,
+ /* depOwner= */ key);
+ consumer.accumulate(metadata);
+ });
// We have to cache the "digest" of the aggregating value itself, because the action cache
// checker may want it.
- inputMap.put(key, runfilesArtifactValue.getMetadata(), /*depOwner=*/ key);
+ inputMap.put(key, runfilesArtifactValue.getMetadata(), /* depOwner= */ key);
} else if (value instanceof TreeArtifactValue) {
TreeArtifactValue treeArtifactValue = (TreeArtifactValue) value;
expandTreeArtifactAndPopulateArtifactData(
@@ -154,7 +154,7 @@
}
@Nullable
- static ImmutableList<FilesetOutputSymlink> getFilesets(
+ private static ImmutableList<FilesetOutputSymlink> getFilesets(
Environment env, SpecialArtifact actionInput) throws InterruptedException {
Preconditions.checkState(actionInput.isFileset(), actionInput);
ActionLookupData generatingActionKey = actionInput.getGeneratingActionKey();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index a701a9f..8c2a068 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -46,7 +46,6 @@
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFile;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.XattrProvider;
@@ -57,8 +56,6 @@
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.io.IOException;
-import java.util.Comparator;
-import java.util.List;
import java.util.Map;
import java.util.function.Supplier;
import javax.annotation.Nullable;
@@ -353,28 +350,34 @@
FileArtifactValue value,
SkyFunction.Environment env)
throws InterruptedException {
- ImmutableList.Builder<Pair<Artifact, FileArtifactValue>> fileInputsBuilder =
- ImmutableList.builder();
- ImmutableList.Builder<Pair<Artifact, TreeArtifactValue>> directoryInputsBuilder =
- ImmutableList.builder();
- List<Artifact> inputs = action.getInputs().toList();
+ ImmutableList<Artifact> inputs = action.getInputs().toList();
SkyframeLookupResult values = env.getValuesAndExceptions(Artifact.keys(inputs));
if (env.valuesMissing()) {
return null;
}
- for (Artifact input : inputs) {
+
+ ImmutableList.Builder<Artifact> files = ImmutableList.builder();
+ ImmutableList.Builder<FileArtifactValue> fileValues = ImmutableList.builder();
+ ImmutableList.Builder<Artifact> trees = ImmutableList.builder();
+ ImmutableList.Builder<TreeArtifactValue> treeValues = ImmutableList.builder();
+
+ // Sort for better equality in RunfilesArtifactValue.
+ ImmutableList<Artifact> sortedInputs =
+ ImmutableList.sortedCopyOf(Artifact.EXEC_PATH_COMPARATOR, inputs);
+ for (Artifact input : sortedInputs) {
SkyValue inputValue = values.get(Artifact.key(input));
if (inputValue == null) {
return null;
}
if (inputValue instanceof FileArtifactValue) {
- fileInputsBuilder.add(Pair.of(input, (FileArtifactValue) inputValue));
+ files.add(input);
+ fileValues.add((FileArtifactValue) inputValue);
} else if (inputValue instanceof ActionExecutionValue) {
- fileInputsBuilder.add(
- Pair.of(
- input, ((ActionExecutionValue) inputValue).getExistingFileArtifactValue(input)));
+ files.add(input);
+ fileValues.add(((ActionExecutionValue) inputValue).getExistingFileArtifactValue(input));
} else if (inputValue instanceof TreeArtifactValue) {
- directoryInputsBuilder.add(Pair.of(input, (TreeArtifactValue) inputValue));
+ trees.add(input);
+ treeValues.add((TreeArtifactValue) inputValue);
} else {
// We do not recurse in middleman artifacts.
Preconditions.checkState(
@@ -386,16 +389,8 @@
}
}
- ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs =
- ImmutableList.sortedCopyOf(
- Comparator.comparing(pair -> pair.getFirst().getExecPathString()),
- fileInputsBuilder.build());
- ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs =
- ImmutableList.sortedCopyOf(
- Comparator.comparing(pair -> pair.getFirst().getExecPathString()),
- directoryInputsBuilder.build());
-
- return new RunfilesArtifactValue(fileInputs, directoryInputs, value);
+ return new RunfilesArtifactValue(
+ value, files.build(), fileValues.build(), trees.build(), treeValues.build());
}
/**
@@ -413,7 +408,7 @@
}
@Nullable
- public static ActionLookupValue getActionLookupValue(
+ static ActionLookupValue getActionLookupValue(
ActionLookupKey actionLookupKey, SkyFunction.Environment env) throws InterruptedException {
ActionLookupValue value = (ActionLookupValue) env.getValue(actionLookupKey);
if (value == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 2073969..6ae08bb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -488,7 +488,6 @@
"//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/analysis:actions/symlink_action",
- "//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
@@ -589,7 +588,7 @@
":tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
- "//src/main/java/com/google/devtools/build/lib/util",
+ "//src/main/java/com/google/devtools/build/lib/util:hash_codes",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
],
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
index afe3c78..ec76bfc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
@@ -13,41 +13,49 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.util.HashCodes;
import com.google.devtools.build.skyframe.SkyValue;
-import java.util.Collection;
/** The artifacts behind a runfiles middleman. */
-class RunfilesArtifactValue implements SkyValue {
- private final ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs;
- private final ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs;
+final class RunfilesArtifactValue implements SkyValue {
+
+ @FunctionalInterface
+ interface RunfilesConsumer<T> {
+ void accept(Artifact artifact, T metadata) throws InterruptedException;
+ }
+
private final FileArtifactValue metadata;
+ // Parallel lists.
+ private final ImmutableList<Artifact> files;
+ private final ImmutableList<FileArtifactValue> fileValues;
+
+ // Parallel lists.
+ private final ImmutableList<Artifact> trees;
+ private final ImmutableList<TreeArtifactValue> treeValues;
+
RunfilesArtifactValue(
- ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs,
- ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs,
- FileArtifactValue metadata) {
- this.fileInputs = Preconditions.checkNotNull(fileInputs);
- this.directoryInputs = Preconditions.checkNotNull(directoryInputs);
- this.metadata = Preconditions.checkNotNull(metadata);
- }
-
- /** Returns the none tree artifacts that this artifact expands to, together with their data. */
- Collection<Pair<Artifact, FileArtifactValue>> getFileArtifacts() {
- return fileInputs;
- }
-
- /**
- * Returns the tree artifacts that this artifact expands to, together with the information to
- * which artifacts the tree artifacts expand to.
- */
- Collection<Pair<Artifact, TreeArtifactValue>> getTreeArtifacts() {
- return directoryInputs;
+ FileArtifactValue metadata,
+ ImmutableList<Artifact> files,
+ ImmutableList<FileArtifactValue> fileValues,
+ ImmutableList<Artifact> trees,
+ ImmutableList<TreeArtifactValue> treeValues) {
+ this.metadata = checkNotNull(metadata);
+ this.files = checkNotNull(files);
+ this.fileValues = checkNotNull(fileValues);
+ this.trees = checkNotNull(trees);
+ this.treeValues = checkNotNull(treeValues);
+ checkArgument(
+ files.size() == fileValues.size() && trees.size() == treeValues.size(),
+ "Size mismatch: %s",
+ this);
}
/** Returns the data of the artifact for this value, as computed by the action cache checker. */
@@ -55,31 +63,49 @@
return metadata;
}
- @SuppressWarnings("EqualsGetClass") // RunfilesArtifactValue not equal to Aggregating.
+ /** Visits the file artifacts that this runfiles artifact expands to, together with their data. */
+ void forEachFile(RunfilesConsumer<FileArtifactValue> consumer) throws InterruptedException {
+ for (int i = 0; i < files.size(); i++) {
+ consumer.accept(files.get(i), fileValues.get(i));
+ }
+ }
+
+ /** Visits the tree artifacts that this runfiles artifact expands to, together with their data. */
+ void forEachTree(RunfilesConsumer<TreeArtifactValue> consumer) throws InterruptedException {
+ for (int i = 0; i < trees.size(); i++) {
+ consumer.accept(trees.get(i), treeValues.get(i));
+ }
+ }
+
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
- if (o == null || getClass() != o.getClass()) {
+ if (!(o instanceof RunfilesArtifactValue)) {
return false;
}
RunfilesArtifactValue that = (RunfilesArtifactValue) o;
return metadata.equals(that.metadata)
- && fileInputs.equals(that.fileInputs)
- && directoryInputs.equals(that.directoryInputs);
+ && files.equals(that.files)
+ && fileValues.equals(that.fileValues)
+ && trees.equals(that.trees)
+ && treeValues.equals(that.treeValues);
}
@Override
public int hashCode() {
- return 31 * 31 * directoryInputs.hashCode() + 31 * fileInputs.hashCode() + metadata.hashCode();
+ return HashCodes.hashObjects(metadata, files, fileValues, trees, treeValues);
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("fileInputs", fileInputs)
- .add("directoryInputs", directoryInputs)
+ .add("metadata", metadata)
+ .add("files", files)
+ .add("fileValues", fileValues)
+ .add("trees", trees)
+ .add("treeValues", treeValues)
.toString();
}
}
