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();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 3865ed0..5c45405 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -20,7 +20,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionLookupData;
@@ -45,7 +44,6 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.skyframe.ArtifactFunction.SourceArtifactException;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
import com.google.devtools.build.lib.vfs.Path;
@@ -59,7 +57,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
@@ -88,7 +85,7 @@
Artifact output = createDerivedArtifact("output");
Path path = output.getPath();
file(path, "contents");
- assertValueMatches(path.stat(), path.getDigest(), evaluateFAN(output));
+ assertValueMatches(path.stat(), path.getDigest(), evaluateFileArtifactValue(output));
}
@Test
@@ -134,7 +131,7 @@
@Test
public void testMiddlemanArtifact() throws Throwable {
- Artifact output = createMiddlemanArtifact("output");
+ DerivedArtifact output = createMiddlemanArtifact("output");
Artifact input1 = createSourceArtifact("input1");
Artifact input2 = createDerivedArtifact("input2");
SpecialArtifact tree = createDerivedTreeArtifactWithAction("treeArtifact");
@@ -150,16 +147,21 @@
actions.add(action);
file(input2.getPath(), "contents");
file(input1.getPath(), "source contents");
- evaluate(Iterables.toArray(Artifact.keys(ImmutableSet.of(input2, input1, tree)), SkyKey.class));
+
SkyValue value = evaluateArtifactValue(output);
- ArrayList<Pair<Artifact, ?>> inputs = new ArrayList<>();
- inputs.addAll(((RunfilesArtifactValue) value).getFileArtifacts());
- inputs.addAll(((RunfilesArtifactValue) value).getTreeArtifacts());
- assertThat(inputs)
- .containsExactly(
- Pair.of(input1, createForTesting(input1)),
- Pair.of(input2, createForTesting(input2)),
- Pair.of(tree, ((TreeArtifactValue) evaluateArtifactValue(tree))));
+
+ ActionLookupData generatingActionKey = output.getGeneratingActionKey();
+ EvaluationResult<ActionExecutionValue> runfilesActionResult = evaluate(generatingActionKey);
+ FileArtifactValue expectedMetadata =
+ runfilesActionResult.get(generatingActionKey).getExistingFileArtifactValue(output);
+ assertThat(value)
+ .isEqualTo(
+ new RunfilesArtifactValue(
+ expectedMetadata,
+ ImmutableList.of(input1, input2),
+ ImmutableList.of(createForTesting(input1), createForTesting(input2)),
+ ImmutableList.of(tree),
+ ImmutableList.of((TreeArtifactValue) evaluateArtifactValue(tree))));
}
/**
@@ -331,7 +333,7 @@
return output;
}
- private Artifact createMiddlemanArtifact(String path) {
+ private DerivedArtifact createMiddlemanArtifact(String path) {
ArtifactRoot middlemanRoot =
ArtifactRoot.asDerivedRoot(middlemanPath, RootType.Middleman, PathFragment.create("out"));
return DerivedArtifact.create(
@@ -391,8 +393,10 @@
}
}
- private FileArtifactValue evaluateFAN(Artifact artifact) throws Exception {
- return ((FileArtifactValue) evaluateArtifactValue(artifact));
+ private FileArtifactValue evaluateFileArtifactValue(Artifact artifact) throws Exception {
+ SkyValue value = evaluateArtifactValue(artifact);
+ assertThat(value).isInstanceOf(FileArtifactValue.class);
+ return (FileArtifactValue) value;
}
private SkyValue evaluateArtifactValue(Artifact artifact) throws Exception {
