commit 56a29d3e33f6271f3bd23f0c52a2c48e7bb1f2c5
author janakr <janakr@google.com> Mon Apr 05 12:06:12 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Mon Apr 05 12:07:16 2021 -0700
tree 623c6a2034216c8fa2d18a556e238353086fbeec
parent 74f35d4b2448d356b1fb86c6ba0d377f2e524aa2

In ActionExecutionFunction, delay construction of the skykey->artifact multimap used only for error processing until we know that it's required. As a result, don't unroll the all-inputs nested set until after we know all deps are present. Saves nested-set unrolling in the missing-dep+error-free case (up to 1/2 of all action executions) and saves iteration and multimap construction in the error-free case (~all actions).

PiperOrigin-RevId: 366845997

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

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 1c2bfd5..e9091ee 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
@@ -19,6 +19,7 @@
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -111,6 +112,7 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.IntFunction;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.StarlarkSemantics;
 
@@ -1339,23 +1341,23 @@
       AccumulateInputResultsFactory<S, R> accumulateInputResultsFactory,
       boolean allowValuesMissingEarlyReturn)
       throws ActionExecutionException, InterruptedException {
-    ImmutableList<Artifact> allInputsList = allInputs.toList();
-
-    // Some keys have more than 1 corresponding Artifact (e.g. actions with 2 outputs).
-    // For Artifacts whose Artifact::key isn't itself.
-    Multimap<SkyKey, Artifact> skyKeyToArtifactSet =
-        MultimapBuilder.hashKeys().hashSetValues().build();
-    allInputsList.forEach(
-        input -> {
-          SkyKey key = Artifact.key(input);
-          if (key != input) {
-            skyKeyToArtifactSet.put(key, input);
-          }
-        });
 
     ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler =
         new ActionExecutionFunctionExceptionHandler(
-            skyKeyToArtifactSet,
+            Suppliers.memoize(
+                () -> {
+                  ImmutableList<Artifact> allInputsList = allInputs.toList();
+                  Multimap<SkyKey, Artifact> skyKeyToArtifactSet =
+                      MultimapBuilder.hashKeys().hashSetValues().build();
+                  allInputsList.forEach(
+                      input -> {
+                        SkyKey key = Artifact.key(input);
+                        if (key != input) {
+                          skyKeyToArtifactSet.put(key, input);
+                        }
+                      });
+                  return skyKeyToArtifactSet;
+                }),
             inputDeps,
             action,
             mandatoryInputs,
@@ -1369,6 +1371,8 @@
       return null;
     }
 
+    ImmutableList<Artifact> allInputsList = allInputs.toList();
+
     // When there are no missing values or there was an error, we can start checking individual
     // files. We don't bother to optimize the error-ful case since it's rare.
     Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles =
@@ -1591,7 +1595,7 @@
 
   /** Helper subclass for the error-handling logic for ActionExecutionFunction#accumulateInputs. */
   private final class ActionExecutionFunctionExceptionHandler {
-    private final Multimap<SkyKey, Artifact> skyKeyToDerivedArtifactSet;
+    private final Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions;
     private final List<
             ValueOrException3<
                 SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>>
@@ -1605,7 +1609,7 @@
     private ActionExecutionException firstActionExecutionException;
 
     ActionExecutionFunctionExceptionHandler(
-        Multimap<SkyKey, Artifact> skyKeyToDerivedArtifactSet,
+        Supplier<Multimap<SkyKey, Artifact>> skyKeyToDerivedArtifactSetForExceptions,
         List<
                 ValueOrException3<
                     SourceArtifactException,
@@ -1616,7 +1620,7 @@
         Set<Artifact> mandatoryInputs,
         Iterable<SkyKey> requestedSkyKeys,
         boolean valuesMissing) {
-      this.skyKeyToDerivedArtifactSet = skyKeyToDerivedArtifactSet;
+      this.skyKeyToDerivedArtifactSetForExceptions = skyKeyToDerivedArtifactSetForExceptions;
       this.inputDeps = inputDeps;
       this.action = action;
       this.mandatoryInputs = mandatoryInputs;
@@ -1688,7 +1692,7 @@
         handleActionExecutionExceptionPerArtifact((Artifact) key, e);
         return;
       }
-      for (Artifact input : skyKeyToDerivedArtifactSet.get(key)) {
+      for (Artifact input : skyKeyToDerivedArtifactSetForExceptions.get().get(key)) {
         handleActionExecutionExceptionPerArtifact(input, e);
       }
     }