Do not flatten tree artifacts in `ActionInputMap`.
When constructing an `ActionInputMap`, we will add all of the action inputs to it, hashed on the exec path. For tree artifacts, we would add both the tree and all of the children from its expansion. That poses a memory usage problem in presence of large tree artifacts -- for one such tree artifact, we would create `O(actions depending on it)` references to each of the files in the tree artifacts.
Reduce the memory usage of `ActionInputMap` by storing a composite entry for the tree children instead of an entry for each individual child. This entry is a reference to the map of children which already is stored in `TreeArtifactValue`. Adjust the lookup logic to work with composite entries in the map. Add a trie representation of the tree artifact locations to allow for faster response when searching for paths which do not exist in the map.
Change `ActionInputMap::getMetadata(String)` to `ActionInputMap::getMetadata(PathString)` so that we can pass the value from the callers (all of them already have it) and prevent creating garbage `PathFragment` when walking the trie.
PiperOrigin-RevId: 384297684
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputDepOwnerMap.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputDepOwnerMap.java
index 66a4e86..f816050 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputDepOwnerMap.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputDepOwnerMap.java
@@ -16,6 +16,8 @@
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import java.util.Collection;
import javax.annotation.Nullable;
@@ -36,6 +38,13 @@
return addOwner(input, depOwner);
}
+ @Override
+ public void putTreeArtifact(
+ SpecialArtifact tree, TreeArtifactValue treeArtifactValue, @Nullable Artifact depOwner) {
+ treeArtifactValue.getChildren().forEach(treeFile -> addOwner(treeFile, depOwner));
+ addOwner(tree, depOwner);
+ }
+
public boolean addOwner(ActionInput input, @Nullable Artifact depOwner) {
if (depOwner == null || !inputsOfInterest.contains(input)) {
return false;
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java
index 8b5a641..9dfc2aa 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java
@@ -13,13 +13,22 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;
+import static com.google.common.base.Preconditions.checkArgument;
import static java.util.stream.Collectors.toList;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.bugreport.BugReporter;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
+import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
import javax.annotation.Nullable;
/**
@@ -34,8 +43,67 @@
* <p>This class is thread-compatible.
*/
public final class ActionInputMap implements MetadataProvider, ActionInputMapSink {
+
+ private static final Object PLACEHOLDER = new Object();
+
+ static class TrieArtifact {
+ private Map<String, TrieArtifact> subFolders = ImmutableMap.of();
+ @Nullable private TreeArtifactValue treeArtifactValue;
+
+ TrieArtifact add(PathFragment treeExecPath, TreeArtifactValue treeArtifactValue) {
+ TrieArtifact current = this;
+ for (String segment : treeExecPath.segments()) {
+ current = current.getOrPutSubFolder(segment);
+ }
+ current.treeArtifactValue = treeArtifactValue;
+ return current;
+ }
+
+ TrieArtifact getOrPutSubFolder(String name) {
+ TrieArtifact trieArtifact;
+ // We special case when we have 0 or 1 child in order to save memory.
+ // This way, we do not allocate hash maps for leaf entries and path entries with a single
+ // child (prefixes of unbranched paths, e.g. [a/b/c/d]/tree{1..n}).
+ // Invariant: subFolders is an immutable map iff subFolders.size() <= 1.
+ switch (subFolders.size()) {
+ case 0:
+ trieArtifact = new TrieArtifact();
+ subFolders = ImmutableMap.of(name, trieArtifact);
+ break;
+ case 1:
+ trieArtifact = subFolders.get(name);
+ if (trieArtifact == null) {
+ trieArtifact = new TrieArtifact();
+ subFolders = new HashMap<>(subFolders);
+ subFolders.put(name, trieArtifact);
+ }
+ break;
+ default:
+ trieArtifact = subFolders.computeIfAbsent(name, ignored -> new TrieArtifact());
+ }
+ return trieArtifact;
+ }
+
+ @Nullable
+ TreeArtifactValue findTreeArtifactNodeAtPrefix(PathFragment execPath) {
+ TrieArtifact current = this;
+ for (String segment : execPath.segments()) {
+ current = current.subFolders.get(segment);
+ if (current == null) {
+ break;
+ }
+ if (current.treeArtifactValue != null) {
+ return current.treeArtifactValue;
+ }
+ }
+ return null;
+ }
+ }
+
+ private final BugReporter bugReporter;
+
/** The number of elements contained in this map. */
- int size;
+ private int size;
/**
* The hash buckets. Values are indexes into the four arrays. The number of buckets is always the
@@ -65,7 +133,16 @@
*/
private Object[] values;
+ private TrieArtifact treeArtifactsRoot = new TrieArtifact();
+
+ @Deprecated
+ @VisibleForTesting
public ActionInputMap(int sizeHint) {
+ this(BugReporter.defaultInstance(), sizeHint);
+ }
+
+ public ActionInputMap(BugReporter bugReporter, int sizeHint) {
+ this.bugReporter = bugReporter;
sizeHint = Math.max(1, sizeHint);
int tableSize = Integer.highestOneBit(sizeHint) << 1;
size = 0;
@@ -98,24 +175,103 @@
@Nullable
@Override
public FileArtifactValue getMetadata(ActionInput input) {
- return getMetadata(input.getExecPathString());
+ if (input instanceof TreeFileArtifact) {
+ TreeFileArtifact treeFileArtifact = (TreeFileArtifact) input;
+ int treeIndex = getIndex(treeFileArtifact.getParent().getExecPathString());
+ if (treeIndex != -1) {
+ checkArgument(
+ values[treeIndex] instanceof TrieArtifact,
+ "Requested tree file artifact under non-tree/omitted tree artifact: %s",
+ input);
+ return ((TrieArtifact) values[treeIndex])
+ .treeArtifactValue
+ .getChildValues()
+ .get(treeFileArtifact);
+ }
+ }
+ int index = getIndex(input.getExecPathString());
+ if (index != -1) {
+ return values[index] instanceof TrieArtifact
+ ? ((TrieArtifact) values[index]).treeArtifactValue.getMetadata()
+ : (FileArtifactValue) values[index];
+ }
+ if (input instanceof Artifact) {
+ // Non tree-artifacts can overlap with tree files, but need to be registered separately
+ // therefore we can skip searching the parents.
+ return null;
+ }
+
+ // Check the trees in case input is a plain action input pointing to a tree artifact file.
+ FileArtifactValue result = getMetadataFromTreeArtifacts(input.getExecPath());
+
+ if (result != null) {
+ bugReporter.sendBugReport(
+ new IllegalArgumentException(
+ String.format(
+ "Tree artifact file: '%s' referred to as an action input", input.getExecPath())));
+ }
+ return result;
}
+ /**
+ * Returns metadata for given path.
+ *
+ * <p>This method is less efficient than {@link #getMetadata(ActionInput)}, please use it instead
+ * of this one when looking up {@linkplain ActionInput action inputs}.
+ */
@Nullable
- public FileArtifactValue getMetadata(String execPathString) {
- int index = getIndex(execPathString);
- return index == -1 ? null : (FileArtifactValue) values[index];
+ public FileArtifactValue getMetadata(PathFragment execPath) {
+ int index = getIndex(execPath.getPathString());
+ if (index != -1) {
+ Object value = values[index];
+ return (value instanceof TrieArtifact)
+ ? ((TrieArtifact) value).treeArtifactValue.getMetadata()
+ : (FileArtifactValue) value;
+ }
+
+ // Fall back to searching the tree artifacts.
+ return getMetadataFromTreeArtifacts(execPath);
+ }
+
+ private FileArtifactValue getMetadataFromTreeArtifacts(PathFragment execPath) {
+ TreeArtifactValue tree = treeArtifactsRoot.findTreeArtifactNodeAtPrefix(execPath);
+ if (tree == null) {
+ return null;
+ }
+
+ Map.Entry<?, FileArtifactValue> entry = tree.findChildEntryByExecPath(execPath);
+ return entry != null ? entry.getValue() : null;
}
@Nullable
@Override
public ActionInput getInput(String execPathString) {
int index = getIndex(execPathString);
- return index == -1 ? null : (ActionInput) keys[index];
+ if (index != -1) {
+ return (ActionInput) keys[index];
+ }
+
+ // Search ancestor paths since execPathString may point to a TreeFileArtifact within one of the
+ // tree artifacts.
+ PathFragment execPath = PathFragment.create(execPathString);
+ TreeArtifactValue tree = treeArtifactsRoot.findTreeArtifactNodeAtPrefix(execPath);
+ if (tree == null) {
+ return null;
+ }
+
+ // We must return an entry from the map since a duplicate would not have the generating action
+ // key set.
+ Map.Entry<TreeFileArtifact, ?> entry = tree.findChildEntryByExecPath(execPath);
+ return entry != null ? entry.getKey() : null;
}
- /** Count of contained entries. */
- public int size() {
+ /**
+ * Returns count of unique, top-level {@linkplain ActionInput action inputs} in the map.
+ *
+ * <p>Top-level means that each tree artifact, counts as 1, irrespective of the number of children
+ * it has. Same goes for nested trees/files.
+ */
+ public int sizeForDebugging() {
return size;
}
@@ -124,7 +280,42 @@
return putWithNoDepOwner(input, metadata);
}
+ @Override
+ public void putTreeArtifact(
+ SpecialArtifact tree, TreeArtifactValue treeArtifactValue, @Nullable Artifact depOwner) {
+ // Use a placeholder value so that we don't have to create a new trie entry if the entry is
+ // already in the map.
+ int oldIndex = putIfAbsent(tree, PLACEHOLDER);
+ if (oldIndex != -1) {
+ checkArgument(
+ isATreeArtifact((ActionInput) keys[oldIndex]),
+ "Tried to overwrite file with a tree artifact: '%s' with the same exec path",
+ tree);
+ return;
+ }
+
+ // Overwrite the placeholder.
+ values[size - 1] =
+ treeArtifactValue.equals(TreeArtifactValue.OMITTED_TREE_MARKER)
+ // No trie entry for omitted tree -- it cannot have any children anyway.
+ ? FileArtifactValue.OMITTED_FILE_MARKER
+ : treeArtifactsRoot.add(tree.getExecPath(), treeArtifactValue);
+ }
+
public boolean putWithNoDepOwner(ActionInput input, FileArtifactValue metadata) {
+ checkArgument(
+ !isATreeArtifact(input),
+ "Can't add tree artifact: %s using put -- please use putTreeArtifact for that",
+ input);
+ int oldIndex = putIfAbsent(input, metadata);
+ checkArgument(
+ oldIndex == -1 || !isATreeArtifact((ActionInput) keys[oldIndex]),
+ "Tried to overwrite tree artifact with a file: '%s' with the same exec path",
+ input);
+ return oldIndex == -1;
+ }
+
+ private int putIfAbsent(ActionInput input, Object metadata) {
Preconditions.checkNotNull(input);
if (size >= keys.length) {
resize();
@@ -139,7 +330,7 @@
do {
index = nextIndex;
if (hashCode == paths[index].hashCode() && Objects.equal(path, paths[index])) {
- return false;
+ return index;
}
nextIndex = next[index];
} while (nextIndex != -1);
@@ -150,7 +341,7 @@
paths[size] = input.getExecPathString();
values[size] = metadata;
size++;
- return true;
+ return -1;
}
@VisibleForTesting
@@ -161,6 +352,7 @@
Arrays.fill(paths, null);
Arrays.fill(values, null);
size = 0;
+ treeArtifactsRoot = new TrieArtifact();
}
private void resize() {
@@ -187,10 +379,15 @@
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("size", size())
+ .add("size", size)
+ .add("all-files", sizeForDebugging())
.add("first-fifty-keys", Arrays.stream(keys).limit(50).collect(toList()))
.add("first-fifty-values", Arrays.stream(values).limit(50).collect(toList()))
.add("first-fifty-paths", Arrays.stream(paths).limit(50).collect(toList()))
.toString();
}
+
+ private static boolean isATreeArtifact(ActionInput input) {
+ return input instanceof SpecialArtifact && ((SpecialArtifact) input).isTreeArtifact();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMapSink.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMapSink.java
index 8d598d6..091fde4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMapSink.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMapSink.java
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import javax.annotation.Nullable;
/**
@@ -22,6 +24,14 @@
*/
public interface ActionInputMapSink {
- /** Returns true if an entry was added, false if the map already contains {@code input}. */
+ /**
+ * Adds an input if it is not present and returns true iff a new entry was added.
+ *
+ * <p>Does not handle tree artifacts, please use {@link #putTreeArtifact} for those.
+ */
boolean put(ActionInput input, FileArtifactValue metadata, @Nullable Artifact depOwner);
+
+ /** Adds a tree artifact entry with given value. */
+ void putTreeArtifact(
+ SpecialArtifact tree, TreeArtifactValue treeArtifactValue, @Nullable Artifact depOwner);
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java
index 486aaec..9adb7c1 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehaviorWithoutError;
import com.google.devtools.build.lib.bugreport.BugReport;
+import com.google.devtools.build.lib.bugreport.BugReporter;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
@@ -40,7 +41,7 @@
ImmutableMap.of(),
ImmutableMap.of(),
ArtifactPathResolver.IDENTITY,
- new ActionInputMap(0),
+ new ActionInputMap(BugReporter.defaultInstance(), 0),
false,
false);
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
index 94f3ae1..dc5018a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -320,23 +320,13 @@
};
}
- // -------------------- Implementation Helpers --------------------
-
- private String execPathString(PathFragment path) {
- return path.relativeTo(execRoot).getPathString();
- }
-
@Nullable
private RemoteFileArtifactValue getRemoteInputMetadata(PathFragment path) {
if (!path.startsWith(outputBase)) {
return null;
}
- return getRemoteInputMetadata(execPathString(path));
- }
-
- @Nullable
- private RemoteFileArtifactValue getRemoteInputMetadata(String execPathString) {
- FileArtifactValue m = inputArtifactData.getMetadata(execPathString);
+ PathFragment execPath = path.relativeTo(execRoot);
+ FileArtifactValue m = inputArtifactData.getMetadata(execPath);
if (m != null && m.isRemote()) {
return (RemoteFileArtifactValue) m;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 89e3d3e..cc8c54d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -979,11 +979,7 @@
if (retrievedMetadata instanceof TreeArtifactValue) {
TreeArtifactValue treeValue = (TreeArtifactValue) retrievedMetadata;
expandedArtifacts.put(input, treeValue.getChildren());
- for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
- treeValue.getChildValues().entrySet()) {
- inputData.putWithNoDepOwner(child.getKey(), child.getValue());
- }
- inputData.putWithNoDepOwner(input, treeValue.getMetadata());
+ inputData.putTreeArtifact((SpecialArtifact) input, treeValue, /*depOwner=*/ null);
treeValue
.getArchivedRepresentation()
.ifPresent(
@@ -1101,7 +1097,7 @@
inputDeps,
allInputs,
requestedSkyKeys,
- ActionInputMap::new,
+ sizeHint -> new ActionInputMap(bugReporter, sizeHint),
CheckInputResults::new,
/*allowValuesMissingEarlyReturn=*/ true,
actionLookupDataForError);
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 adc5992..a55636b 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
@@ -199,16 +199,12 @@
ActionInputMapSink inputMap,
Artifact depOwner) {
if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(value)) {
- inputMap.put(treeArtifact, FileArtifactValue.OMITTED_FILE_MARKER, depOwner);
+ inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
return;
}
- for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
- value.getChildValues().entrySet()) {
- inputMap.put(child.getKey(), child.getValue(), depOwner);
- }
+
+ inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
expandedArtifacts.put(treeArtifact, value.getChildren());
- // Again, we cache the "digest" of the value for cache checking.
- inputMap.put(treeArtifact, value.getMetadata(), depOwner);
value
.getArchivedRepresentation()
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 39293ca..1f3a662 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -482,7 +482,7 @@
return MoreObjects.toStringHelper(this)
.add("outputs", outputs)
.add("store", store)
- .add("inputArtifactDataSize", inputArtifactData.size())
+ .add("inputArtifactDataSize", inputArtifactData.sizeForDebugging())
.toString();
}
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 d914242..9e9a99a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2707,6 +2707,7 @@
"TreeArtifactValue.java",
],
deps = [
+ "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
"//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/actions:has_digest",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index 5ff2ef4..779799e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -176,7 +176,7 @@
boolean allArtifactsAreImportant = artifactsToBuild.areAllOutputGroupsImportant();
- ActionInputMap inputMap = new ActionInputMap(inputDeps.size());
+ ActionInputMap inputMap = new ActionInputMap(bugReporter, inputDeps.size());
// Prepare an ActionInputMap for important artifacts separately, to be used by BEP events. The
// _validation output group can contain orders of magnitude more unimportant artifacts than
// there are important artifacts, and BEP events will retain the ActionInputMap until the
@@ -191,7 +191,7 @@
ImmutableList<Artifact> importantArtifacts =
artifactsToBuild.getImportantArtifacts().toList();
importantArtifactSet = new HashSet<>(importantArtifacts);
- importantInputMap = new ActionInputMap(importantArtifacts.size());
+ importantInputMap = new ActionInputMap(bugReporter, importantArtifacts.size());
}
Map<Artifact, ImmutableCollection<? extends Artifact>> expandedArtifacts = new HashMap<>();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
index 184e1b3..9b282bb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -24,6 +24,8 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
+import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
@@ -41,6 +43,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.util.Arrays;
+import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
@@ -52,6 +55,24 @@
*/
public class TreeArtifactValue implements HasDigest, SkyValue {
+ /**
+ * Comparator based on exec path which works on {@link ActionInput} as opposed to {@link
+ * com.google.devtools.build.lib.actions.Artifact}. This way, we can use an {@link ActionInput} to
+ * search {@link #childData}.
+ */
+ @SerializationConstant @AutoCodec.VisibleForSerialization
+ static final Comparator<ActionInput> EXEC_PATH_COMPARATOR =
+ (input1, input2) -> input1.getExecPath().compareTo(input2.getExecPath());
+
+ private static final ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> EMPTY_MAP =
+ childDataBuilder().build();
+
+ @SuppressWarnings("unchecked")
+ private static ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue>
+ childDataBuilder() {
+ return new ImmutableSortedMap.Builder<>(EXEC_PATH_COMPARATOR);
+ }
+
/** Returns an empty {@link TreeArtifactValue}. */
public static TreeArtifactValue empty() {
return EMPTY;
@@ -150,7 +171,7 @@
static final TreeArtifactValue EMPTY =
new TreeArtifactValue(
MetadataDigestUtils.fromMetadata(ImmutableMap.of()),
- ImmutableSortedMap.of(),
+ EMPTY_MAP,
0L,
/*archivedRepresentation=*/ null,
/*entirelyRemote=*/ false);
@@ -180,7 +201,7 @@
this.entirelyRemote = entirelyRemote;
}
- FileArtifactValue getMetadata() {
+ public FileArtifactValue getMetadata() {
return FileArtifactValue.createProxy(digest);
}
@@ -216,10 +237,26 @@
: null;
}
- ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
+ public ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
return childData;
}
+ /** Returns an entry for child with given exec path or null if no such child is present. */
+ @SuppressWarnings("unchecked")
+ @Nullable
+ public Map.Entry<TreeFileArtifact, FileArtifactValue> findChildEntryByExecPath(
+ PathFragment execPath) {
+ ActionInput searchToken = ActionInputHelper.fromPath(execPath);
+ // Not really a copy -- original map is already an ImmutableSortedMap using the same comparator.
+ ImmutableSortedMap<ActionInput, FileArtifactValue> casted =
+ ImmutableSortedMap.copyOf(childData, EXEC_PATH_COMPARATOR);
+ checkState(casted == (Object) childData, "Casting children resulted with a copy");
+ Map.Entry<? extends ActionInput, FileArtifactValue> entry = casted.floorEntry(searchToken);
+ return entry != null && entry.getKey().getExecPath().equals(execPath)
+ ? (Map.Entry<TreeFileArtifact, FileArtifactValue>) entry
+ : null;
+ }
+
/** Returns true if the {@link TreeFileArtifact}s are only stored remotely. */
public boolean isEntirelyRemote() {
return entirelyRemote;
@@ -271,23 +308,19 @@
private static TreeArtifactValue createMarker(String toStringRepresentation) {
return new TreeArtifactValue(
- null,
- ImmutableSortedMap.of(),
- 0L,
- /*archivedRepresentation=*/ null,
- /*entirelyRemote=*/ false) {
+ null, EMPTY_MAP, 0L, /*archivedRepresentation=*/ null, /*entirelyRemote=*/ false) {
@Override
public ImmutableSet<TreeFileArtifact> getChildren() {
throw new UnsupportedOperationException(toString());
}
@Override
- ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
+ public ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
throw new UnsupportedOperationException(toString());
}
@Override
- FileArtifactValue getMetadata() {
+ public FileArtifactValue getMetadata() {
throw new UnsupportedOperationException(toString());
}
@@ -408,7 +441,7 @@
/** Builder for a {@link TreeArtifactValue}. */
public static final class Builder {
private final ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue> childData =
- ImmutableSortedMap.naturalOrder();
+ childDataBuilder();
private ArchivedRepresentation archivedRepresentation;
private final SpecialArtifact parent;
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
index a8dfb27..a7d26a8 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
@@ -15,9 +15,25 @@
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.US_ASCII;
+import static org.junit.Assert.assertThrows;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import com.google.auto.value.AutoValue;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.bugreport.BugReporter;
+import com.google.devtools.build.lib.clock.BlazeClock;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
@@ -25,52 +41,379 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
/** Unit test for {@link ActionInputMap}. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public final class ActionInputMapTest {
- private ActionInputMap map;
+ // small hint to stress the map
+ private final ActionInputMap map = new ActionInputMap(BugReporter.defaultInstance(), 1);
+ private ArtifactRoot artifactRoot;
@Before
- public void init() {
- map = new ActionInputMap(1); // small hint to stress the map
+ public void createArtifactRoot() {
+ FileSystem fs = new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256);
+ artifactRoot =
+ ArtifactRoot.asDerivedRoot(fs.getPath("/execroot"), RootType.Output, "bazel-out");
}
@Test
public void basicPutAndLookup() {
assertThat(put("/abc/def", 5)).isTrue();
- assertThat(map.size()).isEqualTo(1);
+ assertThat(map.sizeForDebugging()).isEqualTo(1);
assertContains("/abc/def", 5);
- assertThat(map.getMetadata("blah")).isNull();
+ assertThat(map.getMetadata(PathFragment.create("blah"))).isNull();
assertThat(map.getInput("blah")).isNull();
}
@Test
- public void ignoresSubsequent() {
+ public void put_ignoresSubsequentPuts() {
assertThat(put("/abc/def", 5)).isTrue();
- assertThat(map.size()).isEqualTo(1);
+ assertThat(map.sizeForDebugging()).isEqualTo(1);
assertThat(put("/abc/def", 6)).isFalse();
- assertThat(map.size()).isEqualTo(1);
+ assertThat(map.sizeForDebugging()).isEqualTo(1);
assertThat(put("/ghi/jkl", 7)).isTrue();
- assertThat(map.size()).isEqualTo(2);
+ assertThat(map.sizeForDebugging()).isEqualTo(2);
assertThat(put("/ghi/jkl", 8)).isFalse();
- assertThat(map.size()).isEqualTo(2);
+ assertThat(map.sizeForDebugging()).isEqualTo(2);
assertContains("/abc/def", 5);
assertContains("/ghi/jkl", 7);
}
@Test
- public void clear() {
- assertThat(put("/abc/def", 5)).isTrue();
- assertThat(map.size()).isEqualTo(1);
- assertThat(put("/ghi/jkl", 7)).isTrue();
- assertThat(map.size()).isEqualTo(2);
+ public void clear_removesAllElements() {
+ ActionInput input1 = new TestInput("/abc/def");
+ ActionInput input2 = new TestInput("/ghi/jkl");
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact treeChild = TreeFileArtifact.createTreeOutput(tree, "child");
+ map.putWithNoDepOwner(input1, TestMetadata.create(1));
+ map.putWithNoDepOwner(input2, TestMetadata.create(2));
+ map.putTreeArtifact(
+ tree,
+ TreeArtifactValue.newBuilder(tree).putChild(treeChild, TestMetadata.create(3)).build(),
+ /*depOwner=*/ null);
+ // Sanity check
+ assertThat(map.sizeForDebugging()).isEqualTo(3);
+
map.clear();
- assertThat(map.size()).isEqualTo(0);
- assertThat(map.getMetadata("/abc/def")).isNull();
- assertThat(map.getMetadata("/ghi/jkl")).isNull();
+
+ assertThat(map.sizeForDebugging()).isEqualTo(0);
+ assertDoesNotContain(input1);
+ assertDoesNotContain(input2);
+ assertDoesNotContain(tree);
+ assertDoesNotContain(treeChild);
+ }
+
+ @Test
+ public void putTreeArtifact_addsEmptyTreeArtifact() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+
+ map.putTreeArtifact(tree, TreeArtifactValue.empty(), /*depOwner=*/ null);
+
+ assertThat(map.sizeForDebugging()).isEqualTo(1);
+ assertContainsEqualMetadata(tree, TreeArtifactValue.empty().getMetadata());
+ }
+
+ @Test
+ public void putTreeArtifact_addsTreeArtifactAndAllChildren() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(tree, "child1");
+ FileArtifactValue child1Metadata = TestMetadata.create(1);
+ TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(tree, "child2");
+ FileArtifactValue child2Metadata = TestMetadata.create(2);
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(child1, child1Metadata)
+ .putChild(child2, child2Metadata)
+ .build();
+
+ map.putTreeArtifact(tree, treeValue, /*depOwner=*/ null);
+
+ assertThat(map.sizeForDebugging()).isEqualTo(1);
+ assertContainsEqualMetadata(tree, treeValue.getMetadata());
+ assertContainsSameInstance(child1, child1Metadata);
+ assertContainsSameInstance(child2, child2Metadata);
+ }
+
+ @Test
+ public void putTreeArtifact_mixedTreeAndFiles_addsTreeAndChildren() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "child");
+ FileArtifactValue childMetadata = TestMetadata.create(1);
+ ActionInput file = ActionInputHelper.fromPath("file");
+ FileArtifactValue fileMetadata = TestMetadata.create(2);
+ map.putWithNoDepOwner(file, fileMetadata);
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(child, childMetadata).build();
+
+ map.putTreeArtifact(tree, treeValue, /*depOwner=*/ null);
+
+ assertContainsEqualMetadata(tree, treeValue.getMetadata());
+ assertContainsSameInstance(child, childMetadata);
+ assertContainsSameInstance(file, fileMetadata);
+ }
+
+ @Test
+ public void putTreeArtifact_multipleTrees_addsAllTreesAndChildren() {
+ SpecialArtifact tree1 = createTreeArtifact("tree1");
+ TreeFileArtifact tree1Child = TreeFileArtifact.createTreeOutput(tree1, "child");
+ FileArtifactValue tree1ChildMetadata = TestMetadata.create(1);
+ SpecialArtifact tree2 = createTreeArtifact("tree2");
+ TreeFileArtifact tree2Child = TreeFileArtifact.createTreeOutput(tree2, "child");
+ FileArtifactValue tree2ChildMetadata = TestMetadata.create(2);
+ TreeArtifactValue tree1Value =
+ TreeArtifactValue.newBuilder(tree1).putChild(tree1Child, tree1ChildMetadata).build();
+ TreeArtifactValue tree2Value =
+ TreeArtifactValue.newBuilder(tree2).putChild(tree2Child, tree2ChildMetadata).build();
+
+ map.putTreeArtifact(tree1, tree1Value, /*depOwner=*/ null);
+ map.putTreeArtifact(tree2, tree2Value, /*depOwner=*/ null);
+
+ assertContainsEqualMetadata(tree1, tree1Value.getMetadata());
+ assertContainsSameInstance(tree1Child, tree1ChildMetadata);
+ assertContainsEqualMetadata(tree2, tree2Value.getMetadata());
+ assertContainsSameInstance(tree2Child, tree2ChildMetadata);
+ }
+
+ @Test
+ public void putTreeArtifact_multipleTreesUnderSameDirectory_addsAllTrees() {
+ SpecialArtifact tree1 = createTreeArtifact("dir/tree1");
+ SpecialArtifact tree2 = createTreeArtifact("dir/tree2");
+ SpecialArtifact tree3 = createTreeArtifact("dir/tree3");
+
+ map.putTreeArtifact(tree1, TreeArtifactValue.empty(), /*depOwner=*/ null);
+ map.putTreeArtifact(tree2, TreeArtifactValue.empty(), /*depOwner=*/ null);
+ map.putTreeArtifact(tree3, TreeArtifactValue.empty(), /*depOwner=*/ null);
+
+ assertThat(map.getInput(tree1.getExecPathString())).isEqualTo(tree1);
+ assertThat(map.getInput(tree2.getExecPathString())).isEqualTo(tree2);
+ assertThat(map.getInput(tree3.getExecPathString())).isEqualTo(tree3);
+ }
+
+ @Test
+ public void putTreeArtifact_afterPutTreeArtifactWithSameExecPath_doesNothing() {
+ SpecialArtifact tree1 = createTreeArtifact("tree");
+ SpecialArtifact tree2 = createTreeArtifact("tree");
+ TreeFileArtifact tree2File = TreeFileArtifact.createTreeOutput(tree2, "file");
+ TreeArtifactValue tree2Value =
+ TreeArtifactValue.newBuilder(tree2).putChild(tree2File, TestMetadata.create(1)).build();
+ map.putTreeArtifact(tree1, TreeArtifactValue.empty(), /*depOwner=*/ null);
+
+ map.putTreeArtifact(tree2, tree2Value, /*depOwner=*/ null);
+
+ assertContainsEqualMetadata(tree1, TreeArtifactValue.empty().getMetadata());
+ assertThat(map.getMetadata(tree2)).isEqualTo(TreeArtifactValue.empty().getMetadata());
+ assertDoesNotContain(tree2File);
+ }
+
+ @Test
+ public void putTreeArtifact_sameExecPathAsARegularFile_fails() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ ActionInput file = ActionInputHelper.fromPath(tree.getExecPath());
+ map.put(file, TestMetadata.create(1), /*depOwner=*/ null);
+
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> map.putTreeArtifact(tree, TreeArtifactValue.empty(), /*depOwner=*/ null));
+ }
+
+ private enum PutOrder {
+ DECLARED,
+ REVERSE {
+ @Override
+ void runPuts(Runnable put1, Runnable put2) {
+ super.runPuts(put2, put1);
+ }
+ };
+
+ void runPuts(Runnable put1, Runnable put2) {
+ put1.run();
+ put2.run();
+ }
+ }
+
+ @Test
+ public void putTreeArtifact_nestedFile_returnsNestedFileFromExecPath(
+ @TestParameter PutOrder putOrder) {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact treeFile = TreeFileArtifact.createTreeOutput(tree, "file");
+ FileArtifactValue treeFileMetadata = TestMetadata.create(1);
+ ActionInput file = ActionInputHelper.fromPath(treeFile.getExecPath());
+ FileArtifactValue fileMetadata = TestMetadata.create(1); // identical to `tree/file` file.
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(treeFile, treeFileMetadata).build();
+
+ putOrder.runPuts(
+ () -> map.put(file, fileMetadata, /*depOwner=*/ null),
+ () -> map.putTreeArtifact(tree, treeValue, /*depOwner=*/ null));
+
+ assertThat(map.getMetadata(file)).isSameInstanceAs(fileMetadata);
+ assertThat(map.getMetadata(treeFile)).isSameInstanceAs(treeFileMetadata);
+ assertThat(map.getMetadata(treeFile.getExecPath())).isSameInstanceAs(fileMetadata);
+ assertThat(map.getInput(treeFile.getExecPathString())).isSameInstanceAs(file);
+ }
+
+ @Test
+ public void putTreeArtifact_nestedTree_returnsOuterEntriesForOverlappingFiles(
+ @TestParameter PutOrder putOrder) {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact onlyOuterTreeFile = TreeFileArtifact.createTreeOutput(tree, "file1");
+ FileArtifactValue onlyOuterTreeFileMetadata = TestMetadata.create(1);
+ TreeFileArtifact treeFile = TreeFileArtifact.createTreeOutput(tree, "dir/file2");
+ FileArtifactValue treeFileMetadata = TestMetadata.create(2);
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(treeFile, treeFileMetadata)
+ .putChild(onlyOuterTreeFile, onlyOuterTreeFileMetadata)
+ .build();
+ SpecialArtifact nestedTree = createTreeArtifact("tree/dir");
+ TreeFileArtifact nestedTreeFile = TreeFileArtifact.createTreeOutput(nestedTree, "file2");
+ FileArtifactValue nestedTreeFileMetadata = TestMetadata.create(2); // Same as treeFileMetadata.
+ TreeArtifactValue nestedTreeValue =
+ TreeArtifactValue.newBuilder(nestedTree)
+ .putChild(nestedTreeFile, nestedTreeFileMetadata)
+ .build();
+
+ putOrder.runPuts(
+ () -> map.putTreeArtifact(tree, treeValue, /*depOwner=*/ null),
+ () -> map.putTreeArtifact(nestedTree, nestedTreeValue, /*depOwner=*/ null));
+
+ assertContainsEqualMetadata(tree, treeValue.getMetadata());
+ assertContainsEqualMetadata(nestedTree, nestedTreeValue.getMetadata());
+ assertThat(map.getMetadata(treeFile)).isSameInstanceAs(treeFileMetadata);
+ assertThat(map.getMetadata(nestedTreeFile)).isSameInstanceAs(nestedTreeFileMetadata);
+ assertThat(map.getMetadata(treeFile.getExecPath())).isSameInstanceAs(treeFileMetadata);
+ assertThat(map.getInput(treeFile.getExecPathString())).isSameInstanceAs(treeFile);
+ assertContainsSameInstance(onlyOuterTreeFile, onlyOuterTreeFileMetadata);
+ }
+
+ @Test
+ public void putTreeArtifact_omittedTree_addsEntryWithNoChildren() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+
+ map.putTreeArtifact(tree, TreeArtifactValue.OMITTED_TREE_MARKER, /*depOwner=*/ null);
+
+ assertContainsSameInstance(tree, FileArtifactValue.OMITTED_FILE_MARKER);
+ }
+
+ @Test
+ public void put_treeFileArtifact_addsEntry() {
+ TreeFileArtifact treeFile =
+ TreeFileArtifact.createTreeOutput(createTreeArtifact("tree"), "file");
+ FileArtifactValue metadata = TestMetadata.create(1);
+
+ map.put(treeFile, metadata, /*depOwner=*/ null);
+
+ assertContainsSameInstance(treeFile, metadata);
+ }
+
+ @Test
+ public void put_sameExecPathAsATree_fails() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ ActionInput file = ActionInputHelper.fromPath(tree.getExecPath());
+ FileArtifactValue fileMetadata = TestMetadata.create(1);
+ map.putTreeArtifact(tree, TreeArtifactValue.empty(), /*depOwner=*/ null);
+
+ assertThrows(
+ IllegalArgumentException.class, () -> map.put(file, fileMetadata, /*depOwner=*/ null));
+ }
+
+ @Test
+ public void put_treeArtifact_fails() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ FileArtifactValue metadata = TestMetadata.create(1);
+
+ assertThrows(IllegalArgumentException.class, () -> map.put(tree, metadata, /*depOwner=*/ null));
+ }
+
+ @Test
+ public void getMetadata_actionInputWithTreeExecPath_returnsTreeArtifactEntries() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ map.putTreeArtifact(tree, TreeArtifactValue.empty(), /*depOwner=*/ null);
+ ActionInput input = ActionInputHelper.fromPath(tree.getExecPath());
+
+ assertThat(map.getMetadata(input)).isEqualTo(TreeArtifactValue.empty().getMetadata());
+ }
+
+ @Test
+ public void getMetadata_actionInputWithTreeFileExecPath_returnsTreeArtifactEntries() {
+ BugReporter bugReporter = mock(BugReporter.class);
+ ArgumentCaptor<Throwable> exceptionCaptor = ArgumentCaptor.forClass(Throwable.class);
+ doNothing().when(bugReporter).sendBugReport(exceptionCaptor.capture());
+ ActionInputMap inputMap = new ActionInputMap(bugReporter, /*sizeHint=*/ 1);
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact treeFile = TreeFileArtifact.createTreeOutput(tree, "file");
+ FileArtifactValue treeFileMetadata = TestMetadata.create(1);
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(treeFile, treeFileMetadata).build();
+ inputMap.putTreeArtifact(tree, treeValue, /*depOwner=*/ null);
+ ActionInput input = ActionInputHelper.fromPath(treeFile.getExecPath());
+
+ FileArtifactValue metadata = inputMap.getMetadata(input);
+
+ assertThat(metadata).isSameInstanceAs(treeFileMetadata);
+ assertThat(exceptionCaptor.getValue()).isInstanceOf(IllegalArgumentException.class);
+ assertThat(exceptionCaptor.getValue())
+ .hasMessageThat()
+ .isEqualTo("Tree artifact file: 'bazel-out/tree/file' referred to as an action input");
+ }
+
+ @Test
+ public void getMetadata_artifactWithTreeFileExecPath_returnsNull() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact treeFile = TreeFileArtifact.createTreeOutput(tree, "file");
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(treeFile, TestMetadata.create(1)).build();
+ map.putTreeArtifact(tree, treeValue, /*depOwner=*/ null);
+ Artifact artifact =
+ ActionsTestUtil.createArtifactWithExecPath(artifactRoot, treeFile.getExecPath());
+
+ // Even though we could match the artifact by exec path, it was not registered as a nested
+ // artifact -- only the tree file was.
+ assertThat(map.getMetadata(artifact)).isNull();
+ }
+
+ @Test
+ public void getMetadata_missingFileWithinTree_returnsNull() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ map.putTreeArtifact(
+ tree,
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(TreeFileArtifact.createTreeOutput(tree, "file"), TestMetadata.create(1))
+ .build(),
+ /*depOwner=*/ null);
+ TreeFileArtifact nonexistentTreeFile = TreeFileArtifact.createTreeOutput(tree, "nonexistent");
+
+ assertDoesNotContain(nonexistentTreeFile);
+ }
+
+ @Test
+ public void getMetadata_treeFileUnderOmittedParent_fails() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "file");
+ map.putTreeArtifact(tree, TreeArtifactValue.OMITTED_TREE_MARKER, /*depOwner=*/ null);
+
+ assertThrows(IllegalArgumentException.class, () -> map.getMetadata(child));
+ }
+
+ @Test
+ public void getMetadata_treeFileUnderFile_fails() {
+ SpecialArtifact tree = createTreeArtifact("tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "file");
+ ActionInput file = ActionInputHelper.fromPath(tree.getExecPath());
+ map.put(file, TestMetadata.create(1), /*depOwner=*/ null);
+
+ assertThrows(IllegalArgumentException.class, () -> map.getMetadata(child));
+ }
+
+ @Test
+ public void getters_missingTree_returnNull() {
+ map.putTreeArtifact(createTreeArtifact("tree"), TreeArtifactValue.empty(), /*depOwner=*/ null);
+ SpecialArtifact otherTree = createTreeArtifact("other");
+
+ assertDoesNotContain(otherTree);
+ assertDoesNotContain(TreeFileArtifact.createTreeOutput(otherTree, "child"));
}
@Test
@@ -87,7 +430,7 @@
}
TestInput nextInput = new TestInput(new String(bytes, US_ASCII));
if (deduper.add(nextInput)) {
- data.add(new TestEntry(nextInput, new TestMetadata(i)));
+ data.add(new TestEntry(nextInput, TestMetadata.create(i)));
}
}
}
@@ -98,7 +441,7 @@
TestEntry entry = data.get(i);
assertThat(map.putWithNoDepOwner(entry.input, entry.metadata)).isTrue();
}
- assertThat(map.size()).isEqualTo(data.size());
+ assertThat(map.sizeForDebugging()).isEqualTo(data.size());
for (int i = 0; i < data.size(); ++i) {
TestEntry entry = data.get(i);
assertThat(map.getMetadata(entry.input)).isEqualTo(entry.metadata);
@@ -107,15 +450,34 @@
}
private boolean put(String execPath, int value) {
- return map.putWithNoDepOwner(new TestInput(execPath), new TestMetadata(value));
+ return map.putWithNoDepOwner(new TestInput(execPath), TestMetadata.create(value));
}
private void assertContains(String execPath, int value) {
- assertThat(map.getMetadata(new TestInput(execPath))).isEqualTo(new TestMetadata(value));
- assertThat(map.getMetadata(execPath)).isEqualTo(new TestMetadata(value));
+ assertThat(map.getMetadata(new TestInput(execPath))).isEqualTo(TestMetadata.create(value));
+ assertThat(map.getMetadata(PathFragment.create(execPath)))
+ .isEqualTo(TestMetadata.create(value));
assertThat(map.getInput(execPath)).isEqualTo(new TestInput(execPath));
}
+ private void assertDoesNotContain(ActionInput input) {
+ assertThat(map.getMetadata(input)).isNull();
+ assertThat(map.getMetadata(input.getExecPath())).isNull();
+ assertThat(map.getInput(input.getExecPathString())).isNull();
+ }
+
+ private void assertContainsEqualMetadata(ActionInput input, FileArtifactValue metadata) {
+ assertThat(map.getMetadata(input)).isEqualTo(metadata);
+ assertThat(map.getMetadata(input.getExecPath())).isEqualTo(metadata);
+ assertThat(map.getInput(input.getExecPathString())).isSameInstanceAs(input);
+ }
+
+ private void assertContainsSameInstance(ActionInput input, FileArtifactValue metadata) {
+ assertThat(map.getMetadata(input)).isSameInstanceAs(metadata);
+ assertThat(map.getMetadata(input.getExecPath())).isSameInstanceAs(metadata);
+ assertThat(map.getInput(input.getExecPathString())).isSameInstanceAs(input);
+ }
+
private static class TestEntry {
public final TestInput input;
public final TestMetadata metadata;
@@ -165,26 +527,32 @@
}
}
- private static class TestMetadata extends FileArtifactValue {
- private final int id;
+ private SpecialArtifact createTreeArtifact(String relativeExecPath) {
+ return ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ artifactRoot, artifactRoot.getExecPath().getRelative(relativeExecPath));
+ }
- public TestMetadata(int id) {
- this.id = id;
+ @AutoValue
+ abstract static class TestMetadata extends FileArtifactValue {
+ abstract int id();
+
+ static TestMetadata create(int id) {
+ return new AutoValue_ActionInputMapTest_TestMetadata(id);
}
@Override
public FileStateType getType() {
- throw new UnsupportedOperationException();
+ return FileStateType.REGULAR_FILE;
}
@Override
public byte[] getDigest() {
- throw new UnsupportedOperationException();
+ return DigestHashFunction.SHA256.getHashFunction().hashInt(id()).asBytes();
}
@Override
public long getSize() {
- throw new UnsupportedOperationException();
+ return id();
}
@Override
@@ -199,21 +567,12 @@
@Override
public boolean isRemote() {
- throw new UnsupportedOperationException();
+ return false;
}
@Override
public FileContentsProxy getContentsProxy() {
throw new UnsupportedOperationException();
}
-
- @Override
- @SuppressWarnings("EqualsHashCode")
- public boolean equals(Object o) {
- if (!(o instanceof TestMetadata)) {
- return false;
- }
- return id == ((TestMetadata) o).id;
- }
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/BUILD b/src/test/java/com/google/devtools/build/lib/actions/BUILD
index 74b741b..29414e9 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/actions/BUILD
@@ -43,6 +43,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis/platform",
+ "//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
@@ -51,6 +52,7 @@
"//src/main/java/com/google/devtools/build/lib/exec:single_build_file_cache",
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
"//src/main/java/com/google/devtools/build/lib/rules/java:java-compilation",
+ "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils:depsutils",
@@ -72,6 +74,7 @@
"//src/test/java/com/google/devtools/build/lib/testutil:TestThread",
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
+ "//third_party:auto_value",
"//third_party:guava",
"//third_party:guava-testlib",
"//third_party:jsr305",
@@ -79,6 +82,7 @@
"//third_party:mockito",
"//third_party:truth",
"//third_party/protobuf:protobuf_java",
+ "@com_google_testparameterinjector//:testparameterinjector",
],
)
diff --git a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java
index 2006f97..fb4cbc0 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.actions.CompletionContext.ArtifactReceiver;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -56,9 +57,13 @@
TreeFileArtifact treeFile1 = TreeFileArtifact.createTreeOutput(tree, "file1");
TreeFileArtifact treeFile2 = TreeFileArtifact.createTreeOutput(tree, "file2");
ActionInputMap inputMap = new ActionInputMap(0);
- inputMap.putWithNoDepOwner(tree, DUMMY_METADATA);
- inputMap.putWithNoDepOwner(treeFile1, DUMMY_METADATA);
- inputMap.putWithNoDepOwner(treeFile2, DUMMY_METADATA);
+ inputMap.putTreeArtifact(
+ tree,
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(treeFile1, DUMMY_METADATA)
+ .putChild(treeFile2, DUMMY_METADATA)
+ .build(),
+ /*depOwner=*/ null);
CompletionContext completionContext =
createNewCompletionContext(
ImmutableMap.of(tree, ImmutableList.of(treeFile1, treeFile2)), inputMap);
@@ -73,7 +78,7 @@
public void visitArtifacts_skipsOmittedTreeArtifact() {
SpecialArtifact tree = createTreeArtifact("tree");
ActionInputMap inputMap = new ActionInputMap(0);
- inputMap.putWithNoDepOwner(tree, FileArtifactValue.OMITTED_FILE_MARKER);
+ inputMap.putTreeArtifact(tree, TreeArtifactValue.OMITTED_TREE_MARKER, /*depOwner=*/ null);
CompletionContext completionContext =
createNewCompletionContext(/*expandedArtifacts=*/ ImmutableMap.of(), inputMap);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
index e1dd516..fb18346 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -21,6 +21,7 @@
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Maps;
import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
@@ -39,6 +40,8 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
@@ -46,10 +49,9 @@
import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
/** Tests for {@link TreeArtifactValue}. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public final class TreeArtifactValueTest {
private static final PathFragment BIN_PATH = PathFragment.create("bin");
@@ -315,6 +317,41 @@
}
@Test
+ public void findChildEntryByExecPath_returnsCorrectEntry() {
+ SpecialArtifact tree = createTreeArtifact("bin/tree");
+ TreeFileArtifact file1 = TreeFileArtifact.createTreeOutput(tree, "file1");
+ FileArtifactValue file1Metadata = metadataWithIdNoDigest(1);
+ TreeArtifactValue treeArtifactValue =
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(file1, file1Metadata)
+ .putChild(TreeFileArtifact.createTreeOutput(tree, "file2"), metadataWithIdNoDigest(2))
+ .build();
+
+ assertThat(treeArtifactValue.findChildEntryByExecPath(PathFragment.create("bin/tree/file1")))
+ .isEqualTo(Maps.immutableEntry(file1, file1Metadata));
+ }
+
+ @Test
+ public void findChildEntryByExecPath_nonExistentChild_returnsNull(
+ @TestParameter({"bin/nonexistent", "a_before_nonexistent", "z_after_nonexistent"})
+ String nonexistentPath) {
+ SpecialArtifact tree = createTreeArtifact("bin/tree");
+ TreeArtifactValue treeArtifactValue =
+ TreeArtifactValue.newBuilder(tree)
+ .putChild(TreeFileArtifact.createTreeOutput(tree, "file"), metadataWithIdNoDigest(1))
+ .build();
+
+ assertThat(treeArtifactValue.findChildEntryByExecPath(PathFragment.create(nonexistentPath)))
+ .isNull();
+ }
+
+ @Test
+ public void findChildEntryByExecPath_emptyTreeArtifactValue_returnsNull() {
+ TreeArtifactValue treeArtifactValue = TreeArtifactValue.empty();
+ assertThat(treeArtifactValue.findChildEntryByExecPath(PathFragment.create("file"))).isNull();
+ }
+
+ @Test
public void visitTree_visitsEachChild() throws Exception {
Path treeDir = scratch.dir("tree");
scratch.file("tree/file1");
