Use CustomCommandLine.Builder in SpawnAction always.

This takes advantage of CustomCommandLine's ability to defer argument expansion. I can't think of any situation where this would be inferior, at it also cleans up the code a little.

After the execution phase when Artifact strings will have been prodded and interned, I expect the memory difference to be less pronounced.

PiperOrigin-RevId: 164996323

4 files changed, 47 insertions, 47 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index 83adf2e6ac..9ef6545700 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -605,12 +605,22 @@ public final class CustomCommandLine extends CommandLine {
     public CustomCommandLine build() {
       return new CustomCommandLine(arguments);
     }
+
+    public boolean isEmpty() {
+      return arguments.isEmpty();
+    }
   }
 
   public static Builder builder() {
     return new Builder();
   }
 
+  public static Builder builder(Builder other) {
+    Builder builder = new Builder();
+    builder.arguments.addAll(other.arguments);
+    return builder;
+  }
+
   private final ImmutableList<Object> arguments;
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
index 9da449dd5b..8224804091 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileHelper.java
@@ -40,16 +40,15 @@ public final class ParamFileHelper {
   /**
    * Returns a params file artifact or null for a given command description.
    *
-   *  <p>Returns null if parameter files are not to be used according to paramFileInfo, or if the
+   * <p>Returns null if parameter files are not to be used according to paramFileInfo, or if the
    * command line is short enough that a parameter file is not needed.
    *
    * <p>Make sure to add the returned artifact (if not null) as an input of the corresponding
    * action.
    *
    * @param executableArgs leading arguments that should never be wrapped in a parameter file
-   * @param arguments arguments to the command (in addition to executableArgs), OR
    * @param commandLine a {@link CommandLine} that provides the arguments (in addition to
-   *        executableArgs)
+   *     executableArgs)
    * @param paramFileInfo parameter file information
    * @param configuration the configuration
    * @param analysisEnvironment the analysis environment
@@ -57,8 +56,7 @@ public final class ParamFileHelper {
    */
   static Artifact getParamsFileMaybe(
       List<String> executableArgs,
-      @Nullable Iterable<String> arguments,
-      @Nullable CommandLine commandLine,
+      CommandLine commandLine,
       @Nullable ParamFileInfo paramFileInfo,
       BuildConfiguration configuration,
       AnalysisEnvironment analysisEnvironment,
@@ -67,8 +65,7 @@ public final class ParamFileHelper {
       return null;
     }
     if (!paramFileInfo.always()
-        && getParamFileSize(executableArgs, arguments, commandLine)
-            < configuration.getMinParamFileSize()) {
+        && getParamFileSize(executableArgs, commandLine) < configuration.getMinParamFileSize()) {
       return null;
     }
 
@@ -98,7 +95,6 @@ public final class ParamFileHelper {
   /**
    * Creates an action to write the parameter file.
    *
-   * @param arguments arguments to the command (in addition to executableArgs), OR
    * @param commandLine a {@link CommandLine} that provides the arguments (in addition to
    *     executableArgs)
    * @param owner owner of the action
@@ -106,15 +102,12 @@ public final class ParamFileHelper {
    * @param paramFileInfo parameter file information
    */
   public static ParameterFileWriteAction createParameterFileWriteAction(
-      @Nullable Iterable<String> arguments,
-      @Nullable CommandLine commandLine,
+      CommandLine commandLine,
       ActionOwner owner,
       Artifact parameterFile,
       ParamFileInfo paramFileInfo) {
-    CommandLine paramFileContents = (commandLine != null) ? commandLine : CommandLine.of(arguments);
-
-    return new ParameterFileWriteAction(owner, parameterFile, paramFileContents,
-        paramFileInfo.getFileType(), paramFileInfo.getCharset());
+    return new ParameterFileWriteAction(
+        owner, parameterFile, commandLine, paramFileInfo.getFileType(), paramFileInfo.getCharset());
   }
 
   /**
@@ -123,30 +116,20 @@ public final class ParamFileHelper {
    * <p>Call this if {@link #getParamsFileMaybe} returns null.
    *
    * @param executableArgs leading arguments that should never be wrapped in a parameter file
-   * @param arguments arguments to the command (in addition to executableArgs), OR
    * @param commandLine a {@link CommandLine} that provides the arguments (in addition to
    *     executableArgs)
    */
   public static CommandLine createWithoutParamsFile(
-      List<String> executableArgs, Iterable<String> arguments, CommandLine commandLine) {
-    if (commandLine == null) {
-      Iterable<String> commandArgv = Iterables.concat(executableArgs, arguments);
-      return CommandLine.of(commandArgv);
-    }
-
+      List<String> executableArgs, CommandLine commandLine) {
     if (executableArgs.isEmpty()) {
       return commandLine;
     }
-
     return CommandLine.concat(ImmutableList.copyOf(executableArgs), commandLine);
   }
 
-  /**
-   * Estimates the params file size for the given arguments.
-   */
-  private static int getParamFileSize(
-      List<String> executableArgs, Iterable<String> arguments, CommandLine commandLine) {
-    Iterable<String> actualArguments = (commandLine != null) ? commandLine.arguments() : arguments;
+  /** Estimates the params file size for the given arguments. */
+  private static int getParamFileSize(List<String> executableArgs, CommandLine commandLine) {
+    Iterable<String> actualArguments = commandLine.arguments();
     return getParamFileSize(executableArgs) + getParamFileSize(actualArguments);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index ffbd8bcdf5..94066c76e2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -49,8 +49,6 @@ import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.collect.CollectionUtils;
-import com.google.devtools.build.lib.collect.IterablesChain;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -517,8 +515,8 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     private PathFragment executable;
     // executableArgs does not include the executable itself.
     private List<String> executableArgs;
-    private final IterablesChain.Builder<String> argumentsBuilder = IterablesChain.builder();
-    private CommandLine commandLine;
+    private CustomCommandLine.Builder commandLineBuilder = CustomCommandLine.builder();
+    @Nullable private CommandLine commandLine;
 
     private CharSequence progressMessage;
     private ParamFileInfo paramFileInfo = null;
@@ -549,7 +547,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
       this.executableArgs = (other.executableArgs != null)
           ? Lists.newArrayList(other.executableArgs)
           : null;
-      this.argumentsBuilder.add(other.argumentsBuilder.build());
+      this.commandLineBuilder = CustomCommandLine.builder(other.commandLineBuilder);
       this.commandLine = other.commandLine;
       this.progressMessage = other.progressMessage;
       this.paramFileInfo = other.paramFileInfo;
@@ -582,11 +580,11 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     @VisibleForTesting @CheckReturnValue
     public Action[] build(ActionOwner owner, AnalysisEnvironment analysisEnvironment,
         BuildConfiguration configuration) {
-      Iterable<String> arguments = argumentsBuilder.build();
+      CommandLine commandLine =
+          this.commandLine != null ? this.commandLine : this.commandLineBuilder.build();
       // Check to see if we need to use param file.
       Artifact paramsFile = ParamFileHelper.getParamsFileMaybe(
           buildExecutableArgs(configuration.getShellExecutable()),
-          arguments,
           commandLine,
           paramFileInfo,
           configuration,
@@ -598,13 +596,14 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
       if (paramsFile != null) {
         paramFileWriteAction =
             ParamFileHelper.createParameterFileWriteAction(
-                arguments, commandLine, owner, paramsFile, paramFileInfo);
+                commandLine, owner, paramsFile, paramFileInfo);
       }
 
       List<Action> actions = new ArrayList<>(2);
       actions.add(
           buildSpawnAction(
               owner,
+              commandLine,
               configuration.getActionEnvironment(),
               configuration.getShellExecutable(),
               paramsFile));
@@ -634,17 +633,17 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      */
     SpawnAction buildSpawnAction(
         ActionOwner owner,
+        CommandLine commandLine,
         @Nullable ActionEnvironment configEnv,
         @Nullable PathFragment defaultShellExecutable,
         @Nullable Artifact paramsFile) {
       ImmutableList<String> argv = buildExecutableArgs(defaultShellExecutable);
-      Iterable<String> arguments = argumentsBuilder.build();
       CommandLine actualCommandLine;
       if (paramsFile != null) {
         inputsBuilder.add(paramsFile);
         actualCommandLine = ParamFileHelper.createWithParamsFile(argv, paramFileInfo, paramsFile);
       } else {
-        actualCommandLine = ParamFileHelper.createWithoutParamsFile(argv, arguments, commandLine);
+        actualCommandLine = ParamFileHelper.createWithoutParamsFile(argv, commandLine);
       }
 
       NestedSet<Artifact> tools = toolsBuilder.build();
@@ -1050,7 +1049,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      */
     public Builder addArgument(String argument) {
       Preconditions.checkState(commandLine == null);
-      argumentsBuilder.addElement(argument);
+      commandLineBuilder.add(argument);
       return this;
     }
 
@@ -1059,7 +1058,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      */
     public Builder addArguments(String... arguments) {
       Preconditions.checkState(commandLine == null);
-      argumentsBuilder.add(ImmutableList.copyOf(arguments));
+      commandLineBuilder.add(ImmutableList.copyOf(arguments));
       return this;
     }
 
@@ -1068,7 +1067,11 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      */
     public Builder addArguments(Iterable<String> arguments) {
       Preconditions.checkState(commandLine == null);
-      argumentsBuilder.add(CollectionUtils.makeImmutable(arguments));
+      if (arguments instanceof NestedSet) {
+        commandLineBuilder.add((NestedSet) arguments);
+      } else {
+        commandLineBuilder.add(ImmutableList.copyOf(arguments));
+      }
       return this;
     }
 
@@ -1079,7 +1082,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     public Builder addInputArgument(Artifact argument) {
       Preconditions.checkState(commandLine == null);
       addInput(argument);
-      addArgument(argument.getExecPathString());
+      commandLineBuilder.add(argument);
       return this;
     }
 
@@ -1088,8 +1091,11 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      * arguments.
      */
     public Builder addInputArguments(Iterable<Artifact> arguments) {
-      for (Artifact argument : arguments) {
-        addInputArgument(argument);
+      addInputs(arguments);
+      if (arguments instanceof NestedSet) {
+        commandLineBuilder.add((NestedSet) arguments);
+      } else {
+        commandLineBuilder.add(ImmutableList.copyOf(arguments));
       }
       return this;
     }
@@ -1101,7 +1107,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     public Builder addOutputArgument(Artifact argument) {
       Preconditions.checkState(commandLine == null);
       outputs.add(argument);
-      argumentsBuilder.addElement(argument.getExecPathString());
+      commandLineBuilder.add(argument);
       return this;
     }
 
@@ -1115,7 +1121,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      * Objects passed to this method MUST be immutable.
      */
     public Builder setCommandLine(CommandLine commandLine) {
-      Preconditions.checkState(argumentsBuilder.isEmpty());
+      Preconditions.checkState(commandLineBuilder.isEmpty());
       this.commandLine = commandLine;
       return this;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
index f489cea11f..db40f23f51 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
@@ -128,6 +128,7 @@ public final class SpawnActionTemplate implements ActionTemplate<SpawnAction> {
     // explicitly via builder method #setExecutable and #setEnvironment.
     return actionBuilder.buildSpawnAction(
         getOwner(),
+        commandLine,
         /*configEnv=*/ null,
         /*defaultShellExecutable=*/ null,
         /*paramsFile=*/ null);
@@ -205,7 +206,7 @@ public final class SpawnActionTemplate implements ActionTemplate<SpawnAction> {
   @Override
   public Iterable<String> getClientEnvironmentVariables() {
     return spawnActionBuilder
-        .buildSpawnAction(getOwner(), null, null, null)
+        .buildSpawnAction(getOwner(), CommandLine.of(ImmutableList.of()), null, null, null)
         .getClientEnvironmentVariables();
   }