Introduce ArgChunk, a layer of indirection in CommandLine expansi…

…on that allows lazy command lines to provide an efficient way to obtain the total argument length without materializing the arguments.

An `AbstractCommandLine` class is split out to provide simple implementations of the modified interface. Currently, all implementations use `AbstractCommandLine`, but soon `StarlarkCustomCommandLine` won't. At that point, it will use a lazy `Iterable` and an efficient custom implementation of `ArgChunk#totalArgLength`.

PiperOrigin-RevId: 610413772
Change-Id: Id6216cdea28345ddd17d7fa15f1cd3857bc92715

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

@@ -0,0 +1,62 @@
+// Copyright 2014 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.actions;
+
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.util.Fingerprint;
+import javax.annotation.Nullable;
+
+/**
+ * Partial implementation of a {@link CommandLine} suitable for when expansion eagerly materializes
+ * strings.
+ */
+public abstract class AbstractCommandLine extends CommandLine {
+
+  @Override
+  public final ArgChunk expand() throws CommandLineExpansionException, InterruptedException {
+    return new SimpleArgChunk(arguments());
+  }
+
+  @Override
+  public final ArgChunk expand(ArtifactExpander artifactExpander, PathMapper pathMapper)
+      throws CommandLineExpansionException, InterruptedException {
+    return new SimpleArgChunk(arguments(artifactExpander, pathMapper));
+  }
+
+  /**
+   * Returns the expanded command line with enclosed artifacts expanded by {@code artifactExpander}
+   * at execution time.
+   *
+   * <p>By default, this method just delegates to {@link #arguments()}, without performing any
+   * artifact expansion. Subclasses should override this method if they contain tree artifacts and
+   * need to expand them for proper argument evaluation.
+   */
+  @Override
+  public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
+      throws CommandLineExpansionException, InterruptedException {
+    return arguments();
+  }
+
+  @Override
+  public void addToFingerprint(
+      ActionKeyContext actionKeyContext,
+      @Nullable ArtifactExpander artifactExpander,
+      Fingerprint fingerprint)
+      throws CommandLineExpansionException, InterruptedException {
+    for (String s : arguments()) {
+      fingerprint.addString(s);
+    }
+  }
+}

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

@@ -31,6 +31,7 @@ java_library(
     name = "actions",
     srcs = [
         "AbstractAction.java",
+        "AbstractCommandLine.java",
         "Action.java",
         "ActionAnalysisMetadata.java",
         "ActionCacheAwareAction.java",
@@ -169,7 +170,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/causes",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
-        "//src/main/java/com/google/devtools/build/lib/collect",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset:fingerprint_cache",
         "//src/main/java/com/google/devtools/build/lib/concurrent",

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
@@ -23,51 +24,115 @@
 
 /** A representation of a list of arguments. */
 public abstract class CommandLine {
-  private static class EmptyCommandLine extends CommandLine {
-    @Override
-    public Iterable<String> arguments() {
-      return ImmutableList.of();
-    }
-  }
 
   public static final CommandLine EMPTY = new EmptyCommandLine();
 
-  /** Returns the command line. */
-  public abstract Iterable<String> arguments()
-      throws CommandLineExpansionException, InterruptedException;
+  /** Returns a {@link CommandLine} backed by the given list of arguments. */
+  public static CommandLine of(ImmutableList<String> arguments) {
+    return arguments.isEmpty() ? CommandLine.EMPTY : new SimpleCommandLine(arguments);
+  }
 
   /**
-   * Returns the evaluated command line with enclosed artifacts expanded by {@code artifactExpander}
-   * at execution time.
-   *
-   * <p>By default, this method just delegates to {@link #arguments()}, without performing any
-   * artifact expansion. Subclasses should override this method if they contain TreeArtifacts and
-   * need to expand them for proper argument evaluation.
+   * Returns a {@link CommandLine} that is constructed by appending the {@code args} to {@code
+   * commandLine}.
    */
-  public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
-      throws CommandLineExpansionException, InterruptedException {
-    return arguments();
+  public static CommandLine concat(CommandLine commandLine, ImmutableList<String> args) {
+    if (args.isEmpty()) {
+      return commandLine;
+    }
+    if (commandLine == EMPTY) {
+      return CommandLine.of(args);
+    }
+    return new SuffixedCommandLine(args, commandLine);
   }
 
   /**
-   * Adds the command line to the provided {@link Fingerprint}.
+   * Post-expansion representation of command line arguments.
    *
-   * <p>Some of the implementations may require the to expand provided directory in order to produce
-   * a unique key. Consequently, the result of calling this function can be different depending on
-   * whether the {@link ArtifactExpander} is provided. Moreover, without it, the produced key may
-   * not always be unique.
+   * <p>This differs from {@link CommandLine} in that consuming the arguments is guaranteed to be
+   * free of {@link CommandLineExpansionException} and {@link InterruptedException}.
+   */
+  public interface ArgChunk {
+
+    /**
+     * Returns the arguments.
+     *
+     * <p>The returned {@link Iterable} may lazily materialize strings during iteration, so
+     * consumers should attempt to avoid iterating more times than necessary.
+     */
+    Iterable<String> arguments();
+
+    /**
+     * Counts the total length of all arguments in this chunk.
+     *
+     * <p>Implementations that lazily materialize strings may be able to compute the total argument
+     * length without actually materializing the arguments.
+     */
+    int totalArgLength();
+  }
+
+  /** Implementation of {@link ArgChunk} that delegates to an {@link Iterable}. */
+  public static final class SimpleArgChunk implements ArgChunk {
+    private final Iterable<String> args;
+
+    public SimpleArgChunk(Iterable<String> args) {
+      this.args = args;
+    }
+
+    @Override
+    public Iterable<String> arguments() {
+      return args;
+    }
+
+    @Override
+    public int totalArgLength() {
+      int total = 0;
+      for (String arg : args) {
+        total += arg.length() + 1;
+      }
+      return total;
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this).add("args", args).toString();
+    }
+  }
+
+  /** Returns the expanded command line. */
+  public abstract ArgChunk expand() throws CommandLineExpansionException, InterruptedException;
+
+  /**
+   * Returns the expanded command line with enclosed artifacts expanded by {@code artifactExpander}
+   * at execution time.
    */
-  public void addToFingerprint(
+  public abstract ArgChunk expand(ArtifactExpander artifactExpander, PathMapper pathMapper)
+      throws CommandLineExpansionException, InterruptedException;
+
+  /** Identical to calling {@code expand().arguments()}. */
+  public abstract Iterable<String> arguments()
+      throws CommandLineExpansionException, InterruptedException;
+
+  /** Identical to calling {@code expand(artifactExpander, pathMapper).arguments()}. */
+  public abstract Iterable<String> arguments(
+      ArtifactExpander artifactExpander, PathMapper pathMapper)
+      throws CommandLineExpansionException, InterruptedException;
+
+  /** Adds this command line to the provided {@link Fingerprint}. */
+  public abstract void addToFingerprint(
       ActionKeyContext actionKeyContext,
       @Nullable ArtifactExpander artifactExpander,
       Fingerprint fingerprint)
-      throws CommandLineExpansionException, InterruptedException {
-    for (String s : arguments()) {
-      fingerprint.addString(s);
+      throws CommandLineExpansionException, InterruptedException;
+
+  private static final class EmptyCommandLine extends AbstractCommandLine {
+    @Override
+    public ImmutableList<String> arguments() {
+      return ImmutableList.of();
     }
   }
 
-  private static final class SimpleCommandLine extends CommandLine {
+  private static final class SimpleCommandLine extends AbstractCommandLine {
     private final ImmutableList<String> args;
 
     SimpleCommandLine(ImmutableList<String> args) {
@@ -80,12 +145,7 @@ public ImmutableList<String> arguments() {
     }
   }
 
-  /** Returns a {@link CommandLine} backed by the given list of arguments. */
-  public static CommandLine of(ImmutableList<String> arguments) {
-    return arguments.isEmpty() ? CommandLine.EMPTY : new SimpleCommandLine(arguments);
-  }
-
-  private static final class SuffixedCommandLine extends CommandLine {
+  private static final class SuffixedCommandLine extends AbstractCommandLine {
     private final ImmutableList<String> executableArgs;
     private final CommandLine commandLine;
 
@@ -106,26 +166,12 @@ public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper
     }
   }
 
-  /**
-   * Returns a {@link CommandLine} that is constructed by appending the {@code args} to {@code
-   * commandLine}.
-   */
-  public static CommandLine concat(CommandLine commandLine, ImmutableList<String> args) {
-    if (args.isEmpty()) {
-      return commandLine;
-    }
-    if (commandLine == EMPTY) {
-      return CommandLine.of(args);
-    }
-    return new SuffixedCommandLine(args, commandLine);
-  }
-
   /**
    * This helps when debugging Blaze code that uses {@link CommandLine}s, as you can see their
    * content directly in the variable inspector.
    */
   @Override
-  public String toString() {
+  public final String toString() {
     try {
       return Joiner.on(' ').join(arguments());
     } catch (CommandLineExpansionException e) {

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

@@ -18,6 +18,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.CommandLine.ArgChunk;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -100,16 +101,16 @@ ExpandedCommandLines expand(
     for (CommandLineAndParamFileInfo pair : commandLines) {
       CommandLine commandLine = pair.commandLine;
       ParamFileInfo paramFileInfo = pair.paramFileInfo;
-      Iterable<String> args = commandLine.arguments(artifactExpander, pathMapper);
+      ArgChunk chunk = commandLine.expand(artifactExpander, pathMapper);
       if (paramFileInfo == null) {
-        arguments.addAll(args);
-        cmdLineLength += totalArgLen(args);
+        arguments.addAll(chunk.arguments());
+        cmdLineLength += chunk.totalArgLength();
       } else {
         boolean useParamFile = true;
         if (!paramFileInfo.always()) {
-          int tentativeCmdLineLength = cmdLineLength + totalArgLen(args);
+          int tentativeCmdLineLength = cmdLineLength + chunk.totalArgLength();
           if (tentativeCmdLineLength <= conservativeMaxLength) {
-            arguments.addAll(args);
+            arguments.addAll(chunk.arguments());
             cmdLineLength = tentativeCmdLineLength;
             useParamFile = false;
           }
@@ -132,18 +133,18 @@ ExpandedCommandLines expand(
             paramFiles.add(
                 new ParamFileActionInput(
                     paramFileExecPath,
-                    ParameterFile.flagsOnly(args),
+                    ParameterFile.flagsOnly(chunk.arguments()),
                     paramFileInfo.getFileType(),
                     paramFileInfo.getCharset()));
-            for (String positionalArg : ParameterFile.nonFlags(args)) {
+            for (String positionalArg : ParameterFile.nonFlags(chunk.arguments())) {
               arguments.add(positionalArg);
               cmdLineLength += positionalArg.length() + 1;
             }
           } else {
             paramFiles.add(
                 new ParamFileActionInput(
                     paramFileExecPath,
-                    args,
+                    chunk.arguments(),
                     paramFileInfo.getFileType(),
                     paramFileInfo.getCharset()));
           }
@@ -277,14 +278,6 @@ public Iterable<String> getArguments() {
    */
   public abstract ImmutableList<CommandLineAndParamFileInfo> unpack();
 
-  private static int totalArgLen(Iterable<String> args) {
-    int result = 0;
-    for (String s : args) {
-      result += s.length() + 1;
-    }
-    return result;
-  }
-
   private static void addParamFileInfoToFingerprint(
       ParamFileInfo paramFileInfo, Fingerprint fingerprint) {
     fingerprint.addUUID(PARAM_FILE_UUID);
@@ -521,7 +514,7 @@ public ImmutableList<CommandLineAndParamFileInfo> unpack() {
     }
   }
 
-  private static class SingletonCommandLine extends CommandLine {
+  private static class SingletonCommandLine extends AbstractCommandLine {
     private final Object arg;
 
     SingletonCommandLine(Object arg) {

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

@@ -22,12 +22,12 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.actions.AbstractCommandLine;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.CommandLineItem;
 import com.google.devtools.build.lib.actions.CommandLineItem.ExceptionlessMapFn;
@@ -61,7 +61,7 @@
 
 /** A customizable, serializable class for building memory efficient command lines. */
 @Immutable
-public class CustomCommandLine extends CommandLine {
+public class CustomCommandLine extends AbstractCommandLine {
   private interface ArgvFragment {
     /**
      * Expands this fragment into the passed command line vector.

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

@@ -17,6 +17,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.AbstractCommandLine;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -27,7 +28,7 @@
  * and {@link #command}. Supports optionally padding the command line with an empty argument, which
  * can be useful to ensure that any subsequent arguments get assigned to {@code $1} etc.
  */
-final class ShellCommand extends CommandLine {
+final class ShellCommand extends AbstractCommandLine {
 
   private final PathFragment shExecutable;
   private final String command;