Improve CustomCommandLine.

Instead of having custom ArgvFragments for every combination of desired things, we make a combined "interpreter" of argvs. This saves memory and simplifies things as we do not have to allocate a strategy instance per call to args (instead pushing a single shared instance, followed by the args).

The generic interpreter does have a lot of branching compared to the bespoke implementations, but because the branch is always the same for long stretches the branch predictor should easily be able to handle it with minimal overhead (~1 cycle per branch IIRC).

This CL also elevates that we either want a NestedSet or an ImmutableCollection to the surface of the API, so consumers understand the cost when they call it with a non-immutable collection. Most of the changes in clients is due to this.

To cut down on CL churn, @Deprecated forwarding methods are added to CustomCommandLine. These will be removed in a separate CL using IDE inlining.

RELNOTES: None
PiperOrigin-RevId: 164725370

5 files changed, 34 insertions, 31 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/BundleSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/BundleSupport.java
index 2708a7b2aa..b353ad1d6b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/BundleSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/BundleSupport.java
@@ -21,6 +21,7 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.XCASSETS_DIR
 
 import com.google.common.base.Optional;
 import com.google.common.base.Verify;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -447,8 +448,8 @@ final class BundleSupport {
     }
 
     return commandLine
-        .add(PathFragment.safePathStrings(provider.get(XCASSETS_DIR)))
-        .add(extraActoolArgs)
+        .add(ImmutableList.copyOf(PathFragment.safePathStrings(provider.get(XCASSETS_DIR))))
+        .add(ImmutableList.copyOf(extraActoolArgs))
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
index 285fbd0fe5..4df8db9732 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -1161,7 +1161,7 @@ public abstract class CompilationSupport {
   }
 
   private static CommandLine symbolStripCommandLine(
-      Iterable<String> extraFlags, Artifact unstrippedArtifact, Artifact strippedArtifact) {
+      ImmutableList<String> extraFlags, Artifact unstrippedArtifact, Artifact strippedArtifact) {
     return CustomCommandLine.builder()
         .add(STRIP)
         .add(extraFlags)
@@ -1180,7 +1180,7 @@ public abstract class CompilationSupport {
    * subject to the given {@link StrippingType}.
    */
   protected void registerBinaryStripAction(Artifact binaryToLink, StrippingType strippingType) {
-    final Iterable<String> stripArgs;
+    final ImmutableList<String> stripArgs;
     if (isTestRule) {
       // For test targets, only debug symbols are stripped off, since /usr/bin/strip is not able
       // to strip off all symbols in XCTest bundle.
@@ -1394,8 +1394,7 @@ public abstract class CompilationSupport {
             .add("--");
     for (ObjcHeaderThinningInfo info : infos) {
       cmdLine.addJoinPaths(
-          ":",
-          Lists.newArrayList(info.sourceFile.getExecPath(), info.headersListFile.getExecPath()));
+          ":", ImmutableList.of(info.sourceFile.getExecPath(), info.headersListFile.getExecPath()));
       builder.addInput(info.sourceFile).addOutput(info.headersListFile);
     }
     ruleContext.registerAction(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
index 61e99ec6d9..1c17394a30 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java
@@ -461,7 +461,7 @@ public class J2ObjcAspect extends NativeAspectClass implements ConfiguredAspectF
         j2objcSourceJarTranslatedHeaderFiles(ruleContext));
   }
 
-  private static List<String> sourceJarFlags(RuleContext ruleContext) {
+  private static ImmutableList<String> sourceJarFlags(RuleContext ruleContext) {
     return ImmutableList.of(
         "--output_gen_source_dir",
         j2ObjcSourceJarTranslatedSourceFiles(ruleContext).getExecPathString(),
@@ -492,14 +492,14 @@ public class J2ObjcAspect extends NativeAspectClass implements ConfiguredAspectF
     ImmutableList.Builder<Artifact> sourceJarOutputFiles = ImmutableList.builder();
     if (!Iterables.isEmpty(sourceJars)) {
       sourceJarOutputFiles.addAll(sourceJarOutputs(ruleContext));
-      argBuilder.addJoinExecPaths("--src_jars", ",", sourceJars);
+      argBuilder.addJoinExecPaths("--src_jars", ",", ImmutableList.copyOf(sourceJars));
       argBuilder.add(sourceJarFlags(ruleContext));
     }
 
     Iterable<String> translationFlags = ruleContext
         .getFragment(J2ObjcConfiguration.class)
         .getTranslationFlags();
-    argBuilder.add(translationFlags);
+    argBuilder.add(ImmutableList.copyOf(translationFlags));
 
     NestedSet<Artifact> depsHeaderMappingFiles =
         depJ2ObjcMappingFileProvider.getHeaderMappingFiles();
@@ -541,7 +541,7 @@ public class J2ObjcAspect extends NativeAspectClass implements ConfiguredAspectF
       argBuilder.addJoinExecPaths("-classpath", ":", compileTimeJars);
     }
 
-    argBuilder.addExecPaths(sources);
+    argBuilder.addExecPaths(ImmutableList.copyOf(sources));
 
     Artifact paramFile = j2ObjcOutputParamFile(ruleContext);
     ruleContext.registerAction(new ParameterFileWriteAction(
@@ -584,10 +584,11 @@ public class J2ObjcAspect extends NativeAspectClass implements ConfiguredAspectF
     if (experimentalJ2ObjcHeaderMap) {
       CustomCommandLine.Builder headerMapCommandLine = CustomCommandLine.builder();
       if (!Iterables.isEmpty(sources)) {
-        headerMapCommandLine.addJoinExecPaths("--source_files", ",", sources);
+        headerMapCommandLine.addJoinExecPaths("--source_files", ",", ImmutableList.copyOf(sources));
       }
       if (!Iterables.isEmpty(sourceJars)) {
-        headerMapCommandLine.addJoinExecPaths("--source_jars", ",", sourceJars);
+        headerMapCommandLine.addJoinExecPaths(
+            "--source_jars", ",", ImmutableList.copyOf(sourceJars));
       }
       headerMapCommandLine.addExecPath("--output_mapping_file", outputHeaderMappingFile);
       ruleContext.registerAction(new SpawnAction.Builder()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
index 8f490626cb..0329afa5b3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
@@ -289,7 +289,7 @@ public class LegacyCompilationSupport extends CompilationSupport {
       commandLine.add("-g");
     }
 
-    List<String> coverageFlags = ImmutableList.of();
+    ImmutableList<String> coverageFlags = ImmutableList.of();
     if (collectCodeCoverage) {
       if (buildConfiguration.isLLVMCoverageMapFormatEnabled()) {
         coverageFlags = CLANG_LLVM_COVERAGE_FLAGS;
@@ -299,19 +299,19 @@ public class LegacyCompilationSupport extends CompilationSupport {
     }
 
     commandLine
-        .add(compileFlagsForClang(appleConfiguration))
+        .add(ImmutableList.copyOf(compileFlagsForClang(appleConfiguration)))
         .add(commonLinkAndCompileFlagsForClang(objcProvider, objcConfiguration, appleConfiguration))
         .add(objcConfiguration.getCoptsForCompilationMode())
         .addBeforeEachPath(
             "-iquote", ObjcCommon.userHeaderSearchPaths(objcProvider, buildConfiguration))
-        .addBeforeEachExecPath("-include", pchFile.asSet())
-        .addBeforeEachPath("-I", priorityHeaders)
+        .addBeforeEachExecPath("-include", ImmutableList.copyOf(pchFile.asSet()))
+        .addBeforeEachPath("-I", ImmutableList.copyOf(priorityHeaders))
         .addBeforeEachPath("-I", objcProvider.get(INCLUDE))
         .addBeforeEachPath("-isystem", objcProvider.get(INCLUDE_SYSTEM))
-        .add(otherFlags)
+        .add(ImmutableList.copyOf(otherFlags))
         .addFormatEach("-D%s", objcProvider.get(DEFINE))
         .add(coverageFlags)
-        .add(getCompileRuleCopts());
+        .add(ImmutableList.copyOf(getCompileRuleCopts()));
 
     // Add input source file arguments
     commandLine.add("-c");
@@ -529,10 +529,13 @@ public class LegacyCompilationSupport extends CompilationSupport {
             .setCommandLine(
                 new CustomCommandLine.Builder()
                     .add("-static")
-                    .add("-arch_only").add(appleConfiguration.getSingleArchitecture())
-                    .add("-syslibroot").add(AppleToolchain.sdkDir())
-                    .add("-o").add(outputArchive.getExecPathString())
-                    .addExecPaths(inputArtifacts)
+                    .add("-arch_only")
+                    .add(appleConfiguration.getSingleArchitecture())
+                    .add("-syslibroot")
+                    .add(AppleToolchain.sdkDir())
+                    .add("-o")
+                    .add(outputArchive.getExecPathString())
+                    .addExecPaths(ImmutableList.copyOf(inputArtifacts))
                     .build())
             .addInputs(inputArtifacts)
             .addOutput(outputArchive)
@@ -660,7 +663,7 @@ public class LegacyCompilationSupport extends CompilationSupport {
       Iterable<Artifact> bazelBuiltLibraries,
       Optional<Artifact> linkmap,
       Optional<Artifact> bitcodeSymbolMap) {
-    Iterable<String> libraryNames = libraryNames(objcProvider);
+    ImmutableList<String> libraryNames = libraryNames(objcProvider);
 
     CustomCommandLine.Builder commandLine = CustomCommandLine.builder()
             .addPath(xcrunwrapper(ruleContext).getExecutable().getExecPath());
@@ -724,12 +727,12 @@ public class LegacyCompilationSupport extends CompilationSupport {
         .add("@executable_path/Frameworks")
         .add("-fobjc-link-runtime")
         .add(DEFAULT_LINKER_FLAGS)
-        .addBeforeEach("-framework", frameworkNames(objcProvider))
+        .addBeforeEach("-framework", ImmutableList.copyOf(frameworkNames(objcProvider)))
         .addBeforeEach("-weak_framework", SdkFramework.names(objcProvider.get(WEAK_SDK_FRAMEWORK)))
         .addFormatEach("-l%s", libraryNames)
         .addExecPath("-o", linkedBinary)
         .addBeforeEachExecPath("-force_load", forceLinkArtifacts)
-        .add(extraLinkArgs)
+        .add(ImmutableList.copyOf(extraLinkArgs))
         .add(objcProvider.get(ObjcProvider.LINKOPT));
 
     if (buildConfiguration.isCodeCoverageEnabled()) {
@@ -819,8 +822,9 @@ public class LegacyCompilationSupport extends CompilationSupport {
   }
 
   /** Returns a list of clang flags used for all link and compile actions executed through clang. */
-  private List<String> commonLinkAndCompileFlagsForClang(
-      ObjcProvider provider, ObjcConfiguration objcConfiguration,
+  private ImmutableList<String> commonLinkAndCompileFlagsForClang(
+      ObjcProvider provider,
+      ObjcConfiguration objcConfiguration,
       AppleConfiguration appleConfiguration) {
     ImmutableList.Builder<String> builder = new ImmutableList.Builder<>();
     ApplePlatform platform = appleConfiguration.getSingleArchPlatform();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/SdkFramework.java b/src/main/java/com/google/devtools/build/lib/rules/objc/SdkFramework.java
index 8b33d5665c..337af77f74 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/SdkFramework.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/SdkFramework.java
@@ -34,10 +34,8 @@ final class SdkFramework extends Value<SdkFramework> {
     return name;
   }
 
-  /**
-   * Returns an iterable which contains the name of each given framework in the same order.
-   */
-  static Iterable<String> names(Iterable<SdkFramework> frameworks) {
+  /** Returns an iterable which contains the name of each given framework in the same order. */
+  static ImmutableList<String> names(Iterable<SdkFramework> frameworks) {
     ImmutableList.Builder<String> result = new ImmutableList.Builder<>();
     for (SdkFramework framework : frameworks) {
       result.add(framework.getName());