Always create TreeArtifactValue through a builder.
Ensures that no unnecessary (unsorted) temporary maps are created prior to the ImmutableSortedMap being created for the final TreeArtifactValue.
MetadataInjector becomes aware of TreeArtifactValue now that the dependency cycle is broken after work in b/161911915, so it can take the TreeArtifactValue instead of a map of its children. The method is renamed to injectTree.
RELNOTES: None.
PiperOrigin-RevId: 323080321
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 1f570d4..6727ecf 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
@@ -13,10 +13,11 @@
// 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 static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
@@ -25,7 +26,6 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.HasDigest;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
-import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.Fingerprint;
@@ -49,19 +49,36 @@
*/
public class TreeArtifactValue implements HasDigest, SkyValue {
+ /** Returns an empty {@link TreeArtifactValue}. */
+ public static TreeArtifactValue empty() {
+ return EMPTY;
+ }
+
+ /**
+ * Returns a new {@link Builder} for the given parent tree artifact.
+ *
+ * <p>The returned builder only supports adding children under this parent. To build multiple tree
+ * artifacts at once, use {@link MultiBuilder}.
+ */
+ public static Builder newBuilder(SpecialArtifact parent) {
+ return new Builder(parent);
+ }
+
/** Builder for constructing multiple instances of {@link TreeArtifactValue} at once. */
public interface MultiBuilder {
/**
* Puts a child tree file into this builder under its {@linkplain TreeFileArtifact#getParent
* parent}.
+ *
+ * @return {@code this} for convenience
*/
- void putChild(TreeFileArtifact child, FileArtifactValue metadata);
+ MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata);
/**
* For each unique parent seen by this builder, passes the aggregated metadata to {@link
- * MetadataInjector#injectDirectory}.
+ * TreeArtifactInjector#injectTree}.
*/
- void injectTo(MetadataInjector metadataInjector);
+ void injectTo(TreeArtifactInjector treeInjector);
}
/** Returns a new {@link MultiBuilder}. */
@@ -85,8 +102,7 @@
private final ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData;
private final boolean entirelyRemote;
- @AutoCodec.VisibleForSerialization
- TreeArtifactValue(
+ private TreeArtifactValue(
byte[] digest,
ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData,
boolean entirelyRemote) {
@@ -95,37 +111,6 @@
this.entirelyRemote = entirelyRemote;
}
- /**
- * Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their
- * corresponding FileArtifactValues.
- */
- static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childData) {
- if (childData.isEmpty()) {
- return EMPTY;
- }
-
- // Sort the children to ensure a deterministic TreeArtifactValue (including digest).
- ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> sortedChildData =
- ImmutableSortedMap.copyOf(childData);
- Fingerprint fingerprint = new Fingerprint();
- boolean entirelyRemote = true;
-
- for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : sortedChildData.entrySet()) {
- TreeFileArtifact child = e.getKey();
- FileArtifactValue value = e.getValue();
- Preconditions.checkState(
- !FileArtifactValue.OMITTED_FILE_MARKER.equals(value),
- "Cannot construct TreeArtifactValue because child %s was omitted",
- child);
- // Tolerate a tree artifact having a mix of local and remote children (b/152496153#comment80).
- entirelyRemote &= value.isRemote();
- fingerprint.addPath(child.getParentRelativePath());
- value.addTo(fingerprint);
- }
-
- return new TreeArtifactValue(fingerprint.digestAndReset(), sortedChildData, entirelyRemote);
- }
-
FileArtifactValue getMetadata() {
return FileArtifactValue.createProxy(digest);
}
@@ -136,7 +121,6 @@
.collect(toImmutableSet());
}
- @Nullable
@Override
public byte[] getDigest() {
return digest.clone();
@@ -180,7 +164,7 @@
@Override
public String toString() {
- return MoreObjects.toStringHelper(TreeArtifactValue.class)
+ return MoreObjects.toStringHelper(this)
.add("digest", digest)
.add("childData", childData)
.toString();
@@ -277,7 +261,7 @@
* artifact directory, or if {@link TreeArtifactVisitor#visit} throws {@link IOException}
*/
public static void visitTree(Path parentDir, TreeArtifactVisitor visitor) throws IOException {
- visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, Preconditions.checkNotNull(visitor));
+ visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, checkNotNull(visitor));
}
private static void visitTree(Path parentDir, PathFragment subdir, TreeArtifactVisitor visitor)
@@ -330,20 +314,82 @@
}
}
+ /** Builder for a {@link TreeArtifactValue}. */
+ public static final class Builder {
+ private final ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue> childData =
+ ImmutableSortedMap.naturalOrder();
+ private final SpecialArtifact parent;
+
+ Builder(SpecialArtifact parent) {
+ checkArgument(parent.isTreeArtifact(), "%s is not a tree artifact", parent);
+ this.parent = parent;
+ }
+
+ /**
+ * Adds a child to this builder.
+ *
+ * <p>The child's {@linkplain TreeFileArtifact#getParent parent} <em>must</em> match the parent
+ * with which this builder was initialized.
+ *
+ * <p>Children may be added in any order. The children are sorted prior to constructing the
+ * final {@link TreeArtifactValue}.
+ *
+ * <p>It is illegal to call this method with {@link FileArtifactValue.OMITTED_FILE_MARKER}. When
+ * children are omitted, use {@link TreeArtifactValue#OMITTED_TREE_MARKER}.
+ *
+ * @return {@code this} for convenience
+ */
+ public Builder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ checkArgument(
+ child.getParent().equals(parent),
+ "While building TreeArtifactValue for %s, got %s with parent %s",
+ parent,
+ child,
+ child.getParent());
+ checkArgument(
+ !FileArtifactValue.OMITTED_FILE_MARKER.equals(metadata),
+ "Cannot construct TreeArtifactValue for %s because child %s was omitted",
+ parent,
+ child);
+ childData.put(child, metadata);
+ return this;
+ }
+
+ /** Builds the final {@link TreeArtifactValue}. */
+ public TreeArtifactValue build() {
+ ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> finalChildData = childData.build();
+ if (finalChildData.isEmpty()) {
+ return EMPTY;
+ }
+
+ Fingerprint fingerprint = new Fingerprint();
+ boolean entirelyRemote = true;
+
+ for (Map.Entry<TreeFileArtifact, FileArtifactValue> childData : finalChildData.entrySet()) {
+ // Digest will be deterministic because children are sorted.
+ fingerprint.addPath(childData.getKey().getParentRelativePath());
+ childData.getValue().addTo(fingerprint);
+
+ // Tolerate a mix of local and remote children(b/152496153#comment80).
+ entirelyRemote &= childData.getValue().isRemote();
+ }
+
+ return new TreeArtifactValue(fingerprint.digestAndReset(), finalChildData, entirelyRemote);
+ }
+ }
+
private static final class BasicMultiBuilder implements MultiBuilder {
- private final Map<
- SpecialArtifact, ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue>>
- map = new HashMap<>();
+ private final Map<SpecialArtifact, Builder> map = new HashMap<>();
@Override
- public void putChild(TreeFileArtifact child, FileArtifactValue metadata) {
- map.computeIfAbsent(child.getParent(), parent -> ImmutableSortedMap.naturalOrder())
- .put(child, metadata);
+ public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ map.computeIfAbsent(child.getParent(), Builder::new).putChild(child, metadata);
+ return this;
}
@Override
- public void injectTo(MetadataInjector metadataInjector) {
- map.forEach((parent, builder) -> metadataInjector.injectDirectory(parent, builder.build()));
+ public void injectTo(TreeArtifactInjector treeInjector) {
+ map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build()));
}
}
@@ -353,14 +399,20 @@
map = new ConcurrentHashMap<>();
@Override
- public void putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
map.computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>())
.put(child, metadata);
+ return this;
}
@Override
- public void injectTo(MetadataInjector metadataInjector) {
- map.forEach(metadataInjector::injectDirectory);
+ public void injectTo(TreeArtifactInjector treeInjector) {
+ map.forEach(
+ (parent, children) -> {
+ Builder builder = new Builder(parent);
+ children.forEach(builder::putChild);
+ treeInjector.injectTree(parent, builder.build());
+ });
}
}
}
