Create a "MultiBuilder" for aggregating multiple TreeArtifactValues at once.
The basic (single-thread use case) version offers the optimization that it builds an ImmutableSortedMap so that a copy is avoided in the TreeArtifactValue constructor.
PiperOrigin-RevId: 320028967
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
index 1ec3765..890d38a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
@@ -27,7 +27,7 @@
*
* <p>This can be used to save filesystem operations when the metadata is already known.
*
- * <p>{@linkplain Artifact#isTreeArtifacts Tree artifacts} and their {@linkplain
+ * <p>{@linkplain Artifact#isTreeArtifact Tree artifacts} and their {@linkplain
* Artifact#isChildOfDeclaredDirectory children} must not be passed here. Instead, they should be
* passed to {@link #injectDirectory}.
*
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 9587c22..03df389 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
@@ -21,10 +21,12 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Maps;
+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.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.vfs.Dirent;
@@ -36,9 +38,13 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
/**
* Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s of its child
@@ -46,6 +52,31 @@
*/
public class TreeArtifactValue implements HasDigest, SkyValue {
+ /** 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}.
+ */
+ void putChild(TreeFileArtifact child, FileArtifactValue metadata);
+
+ /**
+ * For each unique parent seen by this builder, passes the aggregated metadata to {@link
+ * MetadataInjector#injectDirectory}.
+ */
+ void injectTo(MetadataInjector metadataInjector);
+ }
+
+ /** Returns a new {@link MultiBuilder}. */
+ public static MultiBuilder newMultiBuilder() {
+ return new BasicMultiBuilder();
+ }
+
+ /** Returns a new thread-safe {@link MultiBuilder}. */
+ public static MultiBuilder newConcurrentMultiBuilder() {
+ return new ConcurrentMultiBuilder();
+ }
+
@SerializationConstant @AutoCodec.VisibleForSerialization
static final TreeArtifactValue EMPTY =
new TreeArtifactValue(
@@ -71,8 +102,7 @@
* Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their
* corresponding FileArtifactValues.
*/
- static TreeArtifactValue create(
- Map<TreeFileArtifact, ? extends FileArtifactValue> childFileValues) {
+ static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childFileValues) {
if (childFileValues.isEmpty()) {
return EMPTY;
}
@@ -273,4 +303,38 @@
explodeDirectory(treeArtifactPath, PathFragment.EMPTY_FRAGMENT, explodedDirectory);
return explodedDirectory.build();
}
+
+ private static final class BasicMultiBuilder implements MultiBuilder {
+ private final Map<
+ SpecialArtifact, ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue>>
+ map = new HashMap<>();
+
+ @Override
+ public void putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ map.computeIfAbsent(child.getParent(), parent -> ImmutableSortedMap.naturalOrder())
+ .put(child, metadata);
+ }
+
+ @Override
+ public void injectTo(MetadataInjector metadataInjector) {
+ map.forEach((parent, builder) -> metadataInjector.injectDirectory(parent, builder.build()));
+ }
+ }
+
+ @ThreadSafe
+ private static final class ConcurrentMultiBuilder implements MultiBuilder {
+ private final ConcurrentMap<SpecialArtifact, ConcurrentMap<TreeFileArtifact, FileArtifactValue>>
+ map = new ConcurrentHashMap<>();
+
+ @Override
+ public void putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ map.computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>())
+ .put(child, metadata);
+ }
+
+ @Override
+ public void injectTo(MetadataInjector metadataInjector) {
+ map.forEach(metadataInjector::injectDirectory);
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index ad9354b..6179a52 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -33,6 +33,7 @@
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
+import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.actions.util.TestAction;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -888,17 +889,11 @@
private static ActionExecutionValue actionValueWithTreeArtifacts(
List<TreeFileArtifact> contents) {
- Map<Artifact, Map<TreeFileArtifact, FileArtifactValue>> directoryData = new HashMap<>();
+ TreeArtifactValue.MultiBuilder treeArtifacts = TreeArtifactValue.newMultiBuilder();
for (TreeFileArtifact output : contents) {
+ Path path = output.getPath();
try {
- Map<TreeFileArtifact, FileArtifactValue> dirDatum =
- directoryData.get(output.getParent());
- if (dirDatum == null) {
- dirDatum = new HashMap<>();
- directoryData.put(output.getParent(), dirDatum);
- }
- Path path = output.getPath();
FileArtifactValue noDigest =
ActionMetadataHandler.fileArtifactValueFromArtifact(
output,
@@ -907,17 +902,27 @@
FileArtifactValue withDigest =
FileArtifactValue.createFromInjectedDigest(
noDigest, path.getDigest(), !output.isConstantMetadata());
- dirDatum.put(output, withDigest);
+ treeArtifacts.putChild(output, withDigest);
} catch (IOException e) {
throw new IllegalStateException(e);
}
}
Map<Artifact, TreeArtifactValue> treeArtifactData = new HashMap<>();
- for (Map.Entry<Artifact, Map<TreeFileArtifact, FileArtifactValue>> dirDatum :
- directoryData.entrySet()) {
- treeArtifactData.put(dirDatum.getKey(), TreeArtifactValue.create(dirDatum.getValue()));
- }
+ treeArtifacts.injectTo(
+ new MetadataInjector() {
+ @Override
+ public void injectFile(Artifact output, FileArtifactValue metadata) {}
+
+ @Override
+ public void injectDirectory(
+ SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children) {
+ treeArtifactData.put(output, TreeArtifactValue.create(children));
+ }
+
+ @Override
+ public void markOmitted(Artifact output) {}
+ });
return ActionExecutionValue.create(
/*artifactData=*/ ImmutableMap.of(),
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
new file mode 100644
index 0000000..bd5c475
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -0,0 +1,148 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact;
+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;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.actions.cache.MetadataInjector;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.util.HashMap;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+/** Tests for {@link TreeArtifactValue}. */
+@RunWith(Parameterized.class)
+public final class TreeArtifactValueTest {
+
+ private static final ArtifactRoot ROOT =
+ ArtifactRoot.asDerivedRoot(new InMemoryFileSystem().getPath("/root"), "bin");
+
+ private enum MultiBuilderType {
+ BASIC {
+ @Override
+ TreeArtifactValue.MultiBuilder newMultiBuilder() {
+ return TreeArtifactValue.newMultiBuilder();
+ }
+ },
+ CONCURRENT {
+ @Override
+ TreeArtifactValue.MultiBuilder newMultiBuilder() {
+ return TreeArtifactValue.newConcurrentMultiBuilder();
+ }
+ };
+
+ abstract TreeArtifactValue.MultiBuilder newMultiBuilder();
+ }
+
+ @Parameter public MultiBuilderType multiBuilderType;
+ private final FakeMetadataInjector metadataInjector = new FakeMetadataInjector();
+
+ @Parameters(name = "{0}")
+ public static MultiBuilderType[] params() {
+ return MultiBuilderType.values();
+ }
+
+ @Test
+ public void empty() {
+ TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
+
+ treeArtifacts.injectTo(metadataInjector);
+
+ assertThat(metadataInjector.injectedTreeArtifacts).isEmpty();
+ }
+
+ @Test
+ public void singleTreeArtifact() {
+ TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
+ SpecialArtifact parent = createTreeArtifact("tree");
+ TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
+ TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2");
+
+ treeArtifacts.putChild(child1, metadataWithId(1));
+ treeArtifacts.putChild(child2, metadataWithId(2));
+ treeArtifacts.injectTo(metadataInjector);
+
+ assertThat(metadataInjector.injectedTreeArtifacts)
+ .containsExactly(
+ parent,
+ TreeArtifactValue.create(
+ ImmutableMap.of(child1, metadataWithId(1), child2, metadataWithId(2))));
+ }
+
+ @Test
+ public void multipleTreeArtifacts() {
+ TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
+ SpecialArtifact parent1 = createTreeArtifact("tree1");
+ TreeFileArtifact parent1Child1 = TreeFileArtifact.createTreeOutput(parent1, "child1");
+ TreeFileArtifact parent1Child2 = TreeFileArtifact.createTreeOutput(parent1, "child2");
+ SpecialArtifact parent2 = createTreeArtifact("tree2");
+ TreeFileArtifact parent2Child = TreeFileArtifact.createTreeOutput(parent2, "child");
+
+ treeArtifacts.putChild(parent1Child1, metadataWithId(1));
+ treeArtifacts.putChild(parent2Child, metadataWithId(3));
+ treeArtifacts.putChild(parent1Child2, metadataWithId(2));
+ treeArtifacts.injectTo(metadataInjector);
+
+ assertThat(metadataInjector.injectedTreeArtifacts)
+ .containsExactly(
+ parent1,
+ TreeArtifactValue.create(
+ ImmutableMap.of(
+ parent1Child1, metadataWithId(1), parent1Child2, metadataWithId(2))),
+ parent2,
+ TreeArtifactValue.create(ImmutableMap.of(parent2Child, metadataWithId(3))));
+ }
+
+ private static SpecialArtifact createTreeArtifact(String execPath) {
+ return ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ ROOT, PathFragment.create(execPath));
+ }
+
+ private static FileArtifactValue metadataWithId(int id) {
+ return new RemoteFileArtifactValue(new byte[] {(byte) id}, id, id);
+ }
+
+ private static final class FakeMetadataInjector implements MetadataInjector {
+ private final Map<SpecialArtifact, TreeArtifactValue> injectedTreeArtifacts = new HashMap<>();
+
+ @Override
+ public void injectFile(Artifact output, FileArtifactValue metadata) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void injectDirectory(
+ SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children) {
+ injectedTreeArtifacts.put(output, TreeArtifactValue.create(children));
+ }
+
+ @Override
+ public void markOmitted(Artifact output) {
+ throw new UnsupportedOperationException();
+ }
+ }
+}
