Allow SpawnActions to always use a params file

and use it to work around an analysis performance regression caused by header
compilation. SpawnAction expands the full argument list and compares the length
to --min_param_file_size when deciding whether to use a params file, and header
compilation actions often have very long JoinExecPathsArg fragemnts that are
expensive to expand.

--
MOS_MIGRATED_REVID=127354241

4 files changed, 38 insertions, 17 deletions

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 ab83c5ebb4..6d4b7670f7 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
@@ -23,9 +23,7 @@ import com.google.devtools.build.lib.actions.ParameterFile;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
 import java.util.List;
-
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
@@ -65,8 +63,11 @@ public final class ParamFileHelper {
       BuildConfiguration configuration,
       AnalysisEnvironment analysisEnvironment,
       Iterable<Artifact> outputs) {
-    if (paramFileInfo == null ||
-        getParamFileSize(executableArgs, arguments, commandLine)
+    if (paramFileInfo == null) {
+      return null;
+    }
+    if (!paramFileInfo.always()
+        && getParamFileSize(executableArgs, arguments, commandLine)
             < configuration.getMinParamFileSize()) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileInfo.java
index 28a3b26eb7..8d3b0e5316 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParamFileInfo.java
@@ -16,10 +16,8 @@ package com.google.devtools.build.lib.analysis.actions;
 
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.util.Preconditions;
-
 import java.nio.charset.Charset;
 import java.util.Objects;
-
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -31,11 +29,13 @@ public final class ParamFileInfo {
   private final ParameterFileType fileType;
   private final Charset charset;
   private final String flag;
+  private final boolean always;
 
-  public ParamFileInfo(ParameterFileType fileType, Charset charset, String flag) {
+  public ParamFileInfo(ParameterFileType fileType, Charset charset, String flag, boolean always) {
     this.fileType = Preconditions.checkNotNull(fileType);
     this.charset = Preconditions.checkNotNull(charset);
     this.flag = Preconditions.checkNotNull(flag);
+    this.always = always;
   }
 
   /**
@@ -59,9 +59,14 @@ public final class ParamFileInfo {
     return flag;
   }
 
+  /** Returns true if a params file should always be used. */
+  public boolean always() {
+    return always;
+  }
+
   @Override
   public int hashCode() {
-    return Objects.hash(charset, flag, fileType);
+    return Objects.hash(charset, flag, fileType, always);
   }
 
   @Override
@@ -73,7 +78,9 @@ public final class ParamFileInfo {
       return false;
     }
     ParamFileInfo other = (ParamFileInfo) obj;
-    return fileType.equals(other.fileType) && charset.equals(other.charset)
-        && flag.equals(other.flag);
+    return fileType.equals(other.fileType)
+        && charset.equals(other.charset)
+        && flag.equals(other.flag)
+        && always == other.always;
   }
 }
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 a458972bf3..5832fdcc3f 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
@@ -51,14 +51,12 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.protobuf.GeneratedMessage.GeneratedExtension;
-
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-
 import javax.annotation.CheckReturnValue;
 import javax.annotation.Nullable;
 
@@ -1039,6 +1037,15 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     }
 
     /**
+     * Force the use of a parameter file and set the encoding to ISO-8859-1 (latin1).
+     *
+     * <p>In order to use parameter files, at least one output artifact must be specified.
+     */
+    public Builder alwaysUseParameterFile(ParameterFileType parameterFileType) {
+      return useParameterFile(parameterFileType, ISO_8859_1, "@", /*always=*/ true);
+    }
+
+    /**
      * Enable or disable the use of a parameter file, set the encoding to the given value, and
      * specify the argument prefix to use in passing the parameter file name to the tool.
      *
@@ -1047,7 +1054,12 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
      */
     public Builder useParameterFile(
         ParameterFileType parameterFileType, Charset charset, String flagPrefix) {
-      paramFileInfo = new ParamFileInfo(parameterFileType, charset, flagPrefix);
+      return useParameterFile(parameterFileType, charset, flagPrefix, /*always=*/ false);
+    }
+
+    private Builder useParameterFile(
+        ParameterFileType parameterFileType, Charset charset, String flagPrefix, boolean always) {
+      paramFileInfo = new ParamFileInfo(parameterFileType, charset, flagPrefix, always);
       return this;
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java
index 9f5e90fe5b..5ce87bc5bc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java
@@ -32,11 +32,9 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-
 import javax.annotation.Nullable;
 
 /**
@@ -249,11 +247,14 @@ public class JavaHeaderCompileActionBuilder {
       builder.addOutput(outputDepsProto);
     }
 
-    builder.useParameterFile(ParameterFileType.UNQUOTED);
+    // Always use a params file. The arguments (classpath in particular) may be so large that
+    // comparing the command line length to the minimum param file size regresses analysis
+    // performance (see b/29410356).
+    builder.alwaysUseParameterFile(ParameterFileType.UNQUOTED);
     builder.setCommandLine(buildCommandLine(ruleContext.getConfiguration().getHostPathSeparator()));
 
     builder.addTransitiveInputs(javabaseInputs);
-    builder.addInputs(classpathEntries);
+    builder.addTransitiveInputs(classpathEntries);
     builder.addInputs(bootclasspathEntries);
     builder.addInputs(processorPath);
     builder.addInputs(sourceJars);