Make getPrimaryOutput() always return the first artifact in getOutputs().

This is already the case everywhere except `CppCompileAction` but was not documented, leading to `SpawnAction` unnecessarily storing a field for the primary output when it's already just the first element in its outputs. This change saves 8 bytes per `SpawnAction` and `GenRuleAction`, and moves other `SpawnAction` subclasses closer to an 8-byte threshold.

`CppCompileAction` had been using the coverage artifact (if present) as the first output but not the primary output. This is easy to change by storing the coverage artifact in a field instead of the primary output. It also removes a boolean field since we can just check the nullness of the coverage artifact field, although this does not change the shallow heap of `CppCompileAction`.

PiperOrigin-RevId: 523196083
Change-Id: Id6fe4ef126334a2b4cd2d2dc63409e6668e5c8bd

Author: justinhorvitz

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -309,10 +309,8 @@ public Artifact getPrimaryInput() {
   }
 
   @Override
-  public Artifact getPrimaryOutput() {
-    // Default behavior is to return the first output artifact.
-    // Use the method rather than field in case of overriding in subclasses.
-    return Iterables.getFirst(getOutputs(), null);
+  public final Artifact getPrimaryOutput() {
+    return Iterables.get(outputs, 0);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java

@@ -186,7 +186,8 @@ NestedSet<Artifact> getInputFilesForExtraAction(ActionExecutionContext actionExe
   Artifact getPrimaryInput();
 
   /**
-   * Returns the "primary" output of this action.
+   * Returns the "primary" output of this action, which is the same as the first artifact in {@link
+   * #getOutputs}.
    *
    * <p>For example, the linked library would be the primary output of a LinkAction.
    *

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java

@@ -48,9 +48,9 @@ default String getProgressMessage(RepositoryMapping mainRepositoryMapping) {
   }
 
   /**
-   * Returns a human-readable description of the inputs to {@link #getKey(ActionKeyContext)}. Used
-   * in the output from '--explain', and in error messages for '--check_up_to_date' and
-   * '--check_tests_up_to_date'. May return null, meaning no extra information is available.
+   * Returns a human-readable description of the inputs to {@link #getKey}. Used in the output from
+   * '--explain', and in error messages for '--check_up_to_date' and '--check_tests_up_to_date'. May
+   * return null, meaning no extra information is available.
    *
    * <p>If the return value is non-null, for consistency it should be a multiline message of the
    * form:

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis.actions;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
@@ -103,7 +104,7 @@ public interface ExtraActionInfoSupplier {
 
   private static final String GUID = "ebd6fce3-093e-45ee-adb6-bf513b602f0d";
 
-  public static final Interner<ImmutableSortedMap<String, String>> executionInfoInterner =
+  private static final Interner<ImmutableSortedMap<String, String>> executionInfoInterner =
       BlazeInterners.newWeakInterner();
 
   private final CommandLines commandLines;
@@ -118,7 +119,6 @@ public interface ExtraActionInfoSupplier {
   private final ImmutableMap<String, String> executionInfo;
 
   private final ExtraActionInfoSupplier extraActionInfoSupplier;
-  private final Artifact primaryOutput;
   private final Consumer<Pair<ActionExecutionContext, List<SpawnResult>>> resultConsumer;
   private final boolean stripOutputPaths;
 
@@ -132,7 +132,6 @@ public interface ExtraActionInfoSupplier {
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified.
    * @param outputs the set of all files written by this action; must not be subsequently modified.
-   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action.
    * @param env the action environment
    * @param commandLines the command lines to execute. This includes the main argv vector and any
@@ -148,7 +147,6 @@ public SpawnAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<Artifact> outputs,
-      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -161,7 +159,6 @@ public SpawnAction(
         tools,
         inputs,
         outputs,
-        primaryOutput,
         resourceSetOrBuilder,
         commandLines,
         commandLineLimits,
@@ -188,7 +185,6 @@ public SpawnAction(
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified
    * @param outputs the set of all files written by this action; must not be subsequently modified.
-   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action.
    * @param env the action's environment
    * @param executionInfo out-of-band information for scheduling the spawn
@@ -206,7 +202,6 @@ public SpawnAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<? extends Artifact> outputs,
-      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -221,7 +216,6 @@ public SpawnAction(
       Consumer<Pair<ActionExecutionContext, List<SpawnResult>>> resultConsumer,
       boolean stripOutputPaths) {
     super(owner, tools, inputs, runfilesSupplier, outputs, env);
-    this.primaryOutput = primaryOutput;
     this.resourceSetOrBuilder = resourceSetOrBuilder;
     this.executionInfo =
         executionInfo.isEmpty()
@@ -238,11 +232,6 @@ public SpawnAction(
     this.stripOutputPaths = stripOutputPaths;
   }
 
-  @Override
-  public Artifact getPrimaryOutput() {
-    return primaryOutput;
-  }
-
   @VisibleForTesting
   public CommandLines getCommandLines() {
     return commandLines;
@@ -261,12 +250,10 @@ public List<String> getArguments() throws CommandLineExpansionException, Interru
   }
 
   @Override
-  public Sequence<CommandLineArgsApi> getStarlarkArgs() throws EvalException {
+  public Sequence<CommandLineArgsApi> getStarlarkArgs() {
     ImmutableList.Builder<CommandLineArgsApi> result = ImmutableList.builder();
     ImmutableSet<Artifact> directoryInputs =
-        getInputs().toList().stream()
-            .filter(artifact -> artifact.isDirectory())
-            .collect(ImmutableSet.toImmutableSet());
+        getInputs().toList().stream().filter(Artifact::isDirectory).collect(toImmutableSet());
 
     for (CommandLineAndParamFileInfo commandLine : commandLines.getCommandLines()) {
       result.add(Args.forRegisteredAction(commandLine, directoryInputs));
@@ -536,7 +523,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont
    * <p>Subclasses of SpawnAction may override this in order to provide action-specific behaviour.
    * This can be necessary, for example, when the action discovers inputs.
    */
-  protected SpawnInfo getExtraActionSpawnInfo()
+  private SpawnInfo getExtraActionSpawnInfo()
       throws CommandLineExpansionException, InterruptedException {
     SpawnInfo.Builder info = SpawnInfo.newBuilder();
     Spawn spawn = getSpawnForExtraAction();
@@ -601,7 +588,7 @@ private ActionSpawn(
         throws CommandLineExpansionException {
       super(
           arguments,
-          ImmutableMap.<String, String>of(),
+          ImmutableMap.of(),
           parent.getExecutionInfo(),
           parent.getRunfilesSupplier(),
           parent,
@@ -805,8 +792,7 @@ private SpawnAction buildSpawnAction(
           owner,
           tools,
           inputsAndTools,
-          ImmutableList.copyOf(outputs),
-          outputs.get(0),
+          ImmutableSet.copyOf(outputs),
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -826,8 +812,7 @@ protected SpawnAction createSpawnAction(
         ActionOwner owner,
         NestedSet<Artifact> tools,
         NestedSet<Artifact> inputsAndTools,
-        ImmutableList<Artifact> outputs,
-        Artifact primaryOutput,
+        ImmutableSet<Artifact> outputs,
         ResourceSetOrBuilder resourceSetOrBuilder,
         CommandLines commandLines,
         CommandLineLimits commandLineLimits,
@@ -843,7 +828,6 @@ protected SpawnAction createSpawnAction(
           tools,
           inputsAndTools,
           outputs,
-          primaryOutput,
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -949,13 +933,6 @@ public Builder addOutputs(Iterable<Artifact> artifacts) {
       return this;
     }
 
-    /**
-     * Checks whether the action produces any outputs
-     */
-    public boolean hasOutputs() {
-      return !outputs.isEmpty();
-    }
-
     /**
      * Sets RecourceSet for builder. If ResourceSetBuilder set, then ResourceSetBuilder will
      * override setResources.

src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java

@@ -18,6 +18,8 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Action;
@@ -81,7 +83,6 @@ public final class StarlarkAction extends SpawnAction implements ActionCacheAwar
    * @param inputs the set of all files potentially read by this action; must not be subsequently
    *     modified
    * @param outputs the set of all files written by this action; must not be subsequently modified.
-   * @param primaryOutput the primary output of this action
    * @param resourceSetOrBuilder the resources consumed by executing this Action
    * @param commandLines the command lines to execute. This includes the main argv vector and any
    *     param file-backed command lines.
@@ -103,7 +104,6 @@ private StarlarkAction(
       NestedSet<Artifact> tools,
       NestedSet<Artifact> inputs,
       Iterable<Artifact> outputs,
-      Artifact primaryOutput,
       ResourceSetOrBuilder resourceSetOrBuilder,
       CommandLines commandLines,
       CommandLineLimits commandLineLimits,
@@ -123,7 +123,6 @@ private StarlarkAction(
             ? createInputs(shadowedAction.get().getInputs(), inputs)
             : inputs,
         outputs,
-        primaryOutput,
         resourceSetOrBuilder,
         commandLines,
         commandLineLimits,
@@ -376,8 +375,7 @@ protected SpawnAction createSpawnAction(
         ActionOwner owner,
         NestedSet<Artifact> tools,
         NestedSet<Artifact> inputsAndTools,
-        ImmutableList<Artifact> outputs,
-        Artifact primaryOutput,
+        ImmutableSet<Artifact> outputs,
         ResourceSetOrBuilder resourceSetOrBuilder,
         CommandLines commandLines,
         CommandLineLimits commandLineLimits,
@@ -403,7 +401,6 @@ protected SpawnAction createSpawnAction(
           tools,
           inputsAndTools,
           outputs,
-          primaryOutput,
           resourceSetOrBuilder,
           commandLines,
           commandLineLimits,
@@ -415,7 +412,7 @@ protected SpawnAction createSpawnAction(
           mnemonic,
           unusedInputsList,
           shadowedAction,
-          stripOutputPaths(mnemonic, inputsAndTools, primaryOutput, configuration));
+          stripOutputPaths(mnemonic, inputsAndTools, outputs, configuration));
     }
 
     /**
@@ -435,7 +432,7 @@ protected SpawnAction createSpawnAction(
     private static boolean stripOutputPaths(
         String mnemonic,
         NestedSet<Artifact> inputs,
-        Artifact primaryOutput,
+        ImmutableSet<Artifact> outputs,
         BuildConfigurationValue configuration) {
       ImmutableList<String> qualifyingMnemonics =
           ImmutableList.of(
@@ -456,7 +453,8 @@ private static boolean stripOutputPaths(
       CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class);
       return coreOptions.outputPathsMode == OutputPathsMode.STRIP
           && qualifyingMnemonics.contains(mnemonic)
-          && PathStripper.isPathStrippable(inputs, primaryOutput.getExecPath().subFragment(0, 1));
+          && PathStripper.isPathStrippable(
+              inputs, Iterables.get(outputs, 0).getExecPath().subFragment(0, 1));
     }
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java

@@ -14,10 +14,8 @@
 
 package com.google.devtools.build.lib.analysis.extra;
 
-import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.Action;
@@ -56,9 +54,6 @@ public final class ExtraAction extends SpawnAction {
   private final boolean createDummyOutput;
   private final NestedSet<Artifact> extraActionInputs;
 
-  public static final Function<ExtraAction, Action> GET_SHADOWED_ACTION =
-      e -> e != null ? e.getShadowedAction() : null;
-
   ExtraAction(
       NestedSet<Artifact> extraActionInputs,
       RunfilesSupplier runfilesSupplier,
@@ -78,7 +73,6 @@ public final class ExtraAction extends SpawnAction {
             NestedSetBuilder.emptySet(Order.STABLE_ORDER),
             extraActionInputs),
         outputs,
-        Iterables.getFirst(outputs, null),
         AbstractAction.DEFAULT_RESOURCE_SET,
         CommandLines.of(argv),
         CommandLineLimits.UNLIMITED,
@@ -128,7 +122,7 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
     updateInputs(
         createInputs(shadowedAction.getInputs(), inputFilesForExtraAction, extraActionInputs));
     return NestedSetBuilder.wrap(
-        Order.STABLE_ORDER, Sets.<Artifact>difference(getInputs().toSet(), oldInputs.toSet()));
+        Order.STABLE_ORDER, Sets.difference(getInputs().toSet(), oldInputs.toSet()));
   }
 
   private static NestedSet<Artifact> createInputs(

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -676,7 +676,7 @@ public void setupEnvVariables(Map<String, String> env, Duration timeout) {
       env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1));
     }
     // TODO(b/184206260): Actually set TEST_RANDOM_SEED with random seed.
-    // The above TEST_RANDOM_SEED has histroically been set with the run number, but we should
+    // The above TEST_RANDOM_SEED has historically been set with the run number, but we should
     // explicitly set TEST_RUN_NUMBER to indicate the run number and actually set TEST_RANDOM_SEED
     // with a random seed. However, much code has come to depend on it being set to the run number
     // and this is an externally documented behavior. Modifying TEST_RANDOM_SEED should be done
@@ -923,11 +923,6 @@ public String getRunfilesPrefix() {
     return workspaceName;
   }
 
-  @Override
-  public Artifact getPrimaryOutput() {
-    return testLog;
-  }
-
   public PackageSpecificationProvider getNetworkAllowlist() {
     return networkAllowlist;
   }

src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java

@@ -1732,7 +1732,7 @@ private void createModuleCodegenAction(
         configuration, featureConfiguration, builder, ruleErrorConsumer);
     CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(compileAction);
-    Artifact objectFile = compileAction.getOutputFile();
+    Artifact objectFile = compileAction.getPrimaryOutput();
     if (pic) {
       result.addPicObjectFile(objectFile);
     } else {
@@ -1775,7 +1775,7 @@ private void createHeaderAction(
         configuration, featureConfiguration, builder, ruleErrorConsumer);
     CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(compileAction);
-    Artifact tokenFile = compileAction.getOutputFile();
+    Artifact tokenFile = compileAction.getPrimaryOutput();
     result.addHeaderTokenFile(tokenFile);
   }
 
@@ -1868,13 +1868,13 @@ private ImmutableList<Artifact> createSourceAction(
           configuration, featureConfiguration, picBuilder, ruleErrorConsumer);
       CppCompileAction picAction = picBuilder.buildOrThrowRuleError(ruleErrorConsumer);
       actionRegistry.registerAction(picAction);
-      directOutputs.add(picAction.getOutputFile());
+      directOutputs.add(picAction.getPrimaryOutput());
       if (addObject) {
-        result.addPicObjectFile(picAction.getOutputFile());
+        result.addPicObjectFile(picAction.getPrimaryOutput());
 
         if (bitcodeOutput) {
           result.addLtoBitcodeFile(
-              picAction.getOutputFile(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
+              picAction.getPrimaryOutput(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
         }
       }
       if (dwoFile != null) {
@@ -1939,7 +1939,7 @@ private ImmutableList<Artifact> createSourceAction(
           configuration, featureConfiguration, builder, ruleErrorConsumer);
       CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
       actionRegistry.registerAction(compileAction);
-      Artifact objectFile = compileAction.getOutputFile();
+      Artifact objectFile = compileAction.getPrimaryOutput();
       directOutputs.add(objectFile);
       if (addObject) {
         result.addObjectFile(objectFile);
@@ -2118,7 +2118,7 @@ private ImmutableList<Artifact> createTempsActions(
     CppCompileAction sdAction = sdBuilder.buildOrThrowRuleError(ruleErrorConsumer);
     actionRegistry.registerAction(sdAction);
 
-    return ImmutableList.of(dAction.getOutputFile(), sdAction.getOutputFile());
+    return ImmutableList.of(dAction.getPrimaryOutput(), sdAction.getPrimaryOutput());
   }
 
   /**