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

src/main/java/com/google/devtools/build/lib/actions/BUILD

@@ -85,6 +85,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_aware_action",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",

src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java

@@ -33,8 +33,8 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector {
    * <p>Freshly created output files (i.e. from an action that just executed) that require a stat to
    * obtain the metadata will first be set read-only and executable during this call. This ensures
    * that the returned metadata has an appropriate ctime, which is affected by chmod. Note that this
-   * does not apply to outputs injected via {@link #injectFile} or {@link #injectDirectory} since a
-   * stat is not required for them.
+   * does not apply to outputs injected via {@link #injectFile} or {@link #injectTree} since a stat
+   * is not required for them.
    */
   @Override
   @Nullable
@@ -47,7 +47,7 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector {
    * Constructs a {@link FileArtifactValue} for the given output whose digest is known.
    *
    * <p>This call does not inject the returned metadata. It should be injected with a followup call
-   * to {@link #injectFile} or {@link #injectDirectory} as appropriate.
+   * to {@link #injectFile} or {@link #injectTree} as appropriate.
    *
    * <p>chmod will not be called on the output.
    */

src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java

@@ -14,13 +14,11 @@
 package com.google.devtools.build.lib.actions.cache;
 
 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.FileArtifactValue;
-import java.util.Map;
+import com.google.devtools.build.lib.skyframe.TreeArtifactInjector;
 
 /** Supports metadata injection of action outputs into skyframe. */
-public interface MetadataInjector {
+public interface MetadataInjector extends TreeArtifactInjector {
 
   /**
    * Injects the metadata of a file.
@@ -29,20 +27,10 @@ public interface MetadataInjector {
    *
    * <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}.
+   * passed to {@link #injectTree}.
    *
    * @param output a regular output file
    * @param metadata the file metadata
    */
   void injectFile(Artifact output, FileArtifactValue metadata);
-
-  /**
-   * Injects the metadata of a tree artifact.
-   *
-   * <p>This can be used to save filesystem operations when the metadata is already known.
-   *
-   * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact}
-   * @param children the metadata of the files stored in the directory
-   */
-  void injectDirectory(SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children);
 }

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -78,6 +78,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/options",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:exit_code",

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.remote;
 
+import static com.google.common.util.concurrent.Futures.immediateFuture;
 import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
 
@@ -44,7 +45,6 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
-import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.actions.cache.MetadataInjector;
@@ -63,6 +63,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
 import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.Dirent;
@@ -98,13 +99,8 @@ interface OutputFilesLocker {
     void lock() throws InterruptedException, IOException;
   }
 
-  private static final ListenableFuture<Void> COMPLETED_SUCCESS = SettableFuture.create();
-  private static final ListenableFuture<byte[]> EMPTY_BYTES = SettableFuture.create();
-
-  static {
-    ((SettableFuture<Void>) COMPLETED_SUCCESS).set(null);
-    ((SettableFuture<byte[]>) EMPTY_BYTES).set(new byte[0]);
-  }
+  private static final ListenableFuture<Void> COMPLETED_SUCCESS = immediateFuture(null);
+  private static final ListenableFuture<byte[]> EMPTY_BYTES = immediateFuture(new byte[0]);
 
   protected final RemoteCacheClient cacheProtocol;
   protected final RemoteOptions options;
@@ -440,7 +436,7 @@ private void createSymlinks(Iterable<SymlinkMetadata> symlinks) throws IOExcepti
     }
   }
 
-  /** Download a file (that is not a directory). The content is fetched from the digest. */
+  /** Downloads a file (that is not a directory). The content is fetched from the digest. */
   public ListenableFuture<Void> downloadFile(Path path, Digest digest) throws IOException {
     Preconditions.checkNotNull(path.getParentDirectory()).createDirectoryAndParents();
     if (digest.getSizeBytes() == 0) {
@@ -621,8 +617,7 @@ private void injectRemoteArtifact(
                 + "--experimental_remote_download_outputs=minimal");
       }
       SpecialArtifact parent = (SpecialArtifact) output;
-      ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> childMetadata =
-          ImmutableMap.builderWithExpectedSize(directory.files.size());
+      TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent);
       for (FileMetadata file : directory.files()) {
         TreeFileArtifact child =
             TreeFileArtifact.createTreeOutput(parent, file.path().relativeTo(parent.getPath()));
@@ -632,9 +627,9 @@ private void injectRemoteArtifact(
                 file.digest().getSizeBytes(),
                 /*locationIndex=*/ 1,
                 actionId);
-        childMetadata.put(child, value);
+        tree.putChild(child, value);
       }
-      metadataInjector.injectDirectory(parent, childMetadata.build());
+      metadataInjector.injectTree(parent, tree.build());
     } else {
       FileMetadata outputMetadata = metadata.file(execRoot.getRelative(output.getExecPathString()));
       if (outputMetadata == null) {

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java

@@ -271,16 +271,15 @@ private static TreeArtifactValue transformSharedTree(
     }
 
     SpecialArtifact newParent = (SpecialArtifact) newArtifact;
+    TreeArtifactValue.Builder newTree = TreeArtifactValue.newBuilder(newParent);
 
-    Map<TreeFileArtifact, FileArtifactValue> newChildren =
-        Maps.newHashMapWithExpectedSize(tree.getChildValues().size());
     for (Map.Entry<TreeFileArtifact, FileArtifactValue> child : tree.getChildValues().entrySet()) {
-      newChildren.put(
+      newTree.putChild(
           TreeFileArtifact.createTreeOutput(newParent, child.getKey().getParentRelativePath()),
           child.getValue());
     }
 
-    return TreeArtifactValue.create(newChildren);
+    return newTree.build();
   }
 
   ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) {

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -20,7 +20,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Sets;
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.io.BaseEncoding;
@@ -322,8 +321,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
       setPathReadOnlyAndExecutable(treeDir);
     }
 
-    ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue> values =
-        ImmutableSortedMap.naturalOrder();
+    TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent);
 
     TreeArtifactValue.visitTree(
         treeDir,
@@ -347,10 +345,10 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
             throw new IOException(errorMessage, e);
           }
 
-          values.put(child, metadata);
+          tree.putChild(child, metadata);
         });
 
-    return TreeArtifactValue.create(values.build());
+    return tree.build();
   }
 
   @Override
@@ -379,21 +377,20 @@ public void injectFile(Artifact output, FileArtifactValue metadata) {
     checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output);
     checkArgument(
         !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(),
-        "Tree artifacts and their children must be injected via injectDirectory: %s",
+        "Tree artifacts and their children must be injected via injectTree: %s",
         output);
     checkState(executionMode.get(), "Tried to inject %s outside of execution", output);
 
     store.putArtifactData(output, metadata);
   }
 
   @Override
-  public void injectDirectory(
-      SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children) {
+  public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
     checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output);
     checkArgument(output.isTreeArtifact(), "Output must be a tree artifact: %s", output);
     checkState(executionMode.get(), "Tried to inject %s outside of execution", output);
 
-    store.putTreeArtifactData(output, TreeArtifactValue.create(children));
+    store.putTreeArtifactData(output, tree);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java

@@ -16,8 +16,6 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
@@ -27,6 +25,7 @@
 import com.google.devtools.build.lib.actions.ActionTemplate;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
+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.ArtifactOwner;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -189,11 +188,14 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey(
       return null;
     }
 
-    // Aggregate the ArtifactValues for individual TreeFileArtifacts into a TreeArtifactValue for
-    // the parent TreeArtifact.
-    ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> map = ImmutableMap.builder();
+    // Aggregate the metadata for individual TreeFileArtifacts into a TreeArtifactValue for the
+    // parent TreeArtifact.
+    SpecialArtifact parent = (SpecialArtifact) artifactDependencies.artifact;
+    TreeArtifactValue.Builder treeBuilder = TreeArtifactValue.newBuilder(parent);
     boolean omitted = false;
+
     for (ActionLookupData actionKey : expandedActionExecutionKeys) {
+      boolean sawTreeChild = false;
       ActionExecutionValue actionExecutionValue =
           (ActionExecutionValue)
               Preconditions.checkNotNull(
@@ -202,48 +204,47 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey(
                   artifactDependencies,
                   expansionValue,
                   expandedActionValueMap);
-      Iterable<TreeFileArtifact> treeFileArtifacts =
-          Iterables.transform(
-              Iterables.filter(
-                  actionExecutionValue.getAllFileValues().keySet(),
-                  artifact -> {
-                    Preconditions.checkState(
-                        artifact.hasParent(),
-                        "No parent: %s %s %s",
-                        artifact,
-                        actionExecutionValue,
-                        artifactDependencies);
-                    return artifact.getParent().equals(artifactDependencies.artifact);
-                  }),
-              artifact -> (TreeFileArtifact) artifact);
 
-      Preconditions.checkState(
-          !Iterables.isEmpty(treeFileArtifacts),
-          "Action denoted by %s does not output TreeFileArtifact from %s",
-          actionKey,
-          artifactDependencies);
+      for (Map.Entry<Artifact, FileArtifactValue> entry :
+          actionExecutionValue.getAllFileValues().entrySet()) {
+        Artifact artifact = entry.getKey();
+        Preconditions.checkState(
+            artifact.hasParent(),
+            "Parentless artifact %s found in ActionExecutionValue for %s: %s %s",
+            artifact,
+            actionKey,
+            actionExecutionValue,
+            artifactDependencies);
 
-      for (TreeFileArtifact treeFileArtifact : treeFileArtifacts) {
-        FileArtifactValue value =
-            actionExecutionValue.getExistingFileArtifactValue(treeFileArtifact);
-        if (FileArtifactValue.OMITTED_FILE_MARKER.equals(value)) {
-          omitted = true;
-        } else {
-          map.put(treeFileArtifact, value);
+        if (artifact.getParent().equals(parent)) {
+          sawTreeChild = true;
+          if (FileArtifactValue.OMITTED_FILE_MARKER.equals(entry.getValue())) {
+            omitted = true;
+          } else {
+            treeBuilder.putChild((TreeFileArtifact) artifact, entry.getValue());
+          }
         }
       }
+
+      Preconditions.checkState(
+          sawTreeChild,
+          "Action denoted by %s does not output any TreeFileArtifacts from %s",
+          actionKey,
+          artifactDependencies);
     }
 
-    ImmutableMap<TreeFileArtifact, FileArtifactValue> children = map.build();
+    TreeArtifactValue tree = treeBuilder.build();
 
     if (omitted) {
       Preconditions.checkState(
-          children.isEmpty(),
+          tree.getChildValues().isEmpty(),
           "Action template expansion has some but not all outputs omitted, present outputs: %s",
-          children);
+          artifactDependencies,
+          tree.getChildValues());
       return TreeArtifactValue.OMITTED_TREE_MARKER;
     }
-    return TreeArtifactValue.create(children);
+
+    return tree;
   }
 
   private static SkyValue createSourceValue(Artifact artifact, Environment env)

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -2674,11 +2674,14 @@ java_library(
 
 java_library(
     name = "tree_artifact_value",
-    srcs = ["TreeArtifactValue.java"],
+    srcs = [
+        "TreeArtifactInjector.java",
+        "TreeArtifactValue.java",
+    ],
     deps = [
-        "//src/main/java/com/google/devtools/build/lib/actions",
         "//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",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/util",

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java

@@ -0,0 +1,31 @@
+// 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 com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+
+/** Supports metadata injection of action outputs into skyframe. */
+public interface TreeArtifactInjector {
+
+  /**
+   * Injects the metadata of a tree artifact.
+   *
+   * <p>This can be used to save filesystem operations when the metadata is already known.
+   *
+   * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact}
+   * @param tree a {@link TreeArtifactValue} with the metadata of the files stored in the directory
+   */
+  void injectTree(SpecialArtifact output, TreeArtifactValue tree);
+}