Add a cache for command lines of tools in Java toolchain.

Java compile actions all have a common command line component of the tool itself (e.g. `JavaBuilder`). Those are currently copied for each of the actions. Add a cache which allows the actions to share these command line components, including both `toolchain` and `stripOutputPaths`.

Allow building a `SpawnAction` with only composite command lines.

PiperOrigin-RevId: 518659540
Change-Id: I0417e962539c011cadd3ee23d89078c3ace99cb5

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

@@ -673,7 +673,7 @@ public static class Builder {
     private boolean useDefaultShellEnvironment = false;
     protected boolean executeUnconditionally;
     private Object executableArg;
-    private CustomCommandLine.Builder executableArgs;
+    @Nullable private CustomCommandLine.Builder executableArgs;
     private List<CommandLineAndParamFileInfo> commandLines = new ArrayList<>();
 
     private CharSequence progressMessage;
@@ -737,7 +737,7 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio
       CommandLines.Builder result = CommandLines.builder();
       if (executableArg != null) {
         result.addSingleArgument(executableArg);
-      } else {
+      } else if (executableArgs != null) {
         result.addCommandLine(executableArgs.build());
       }
       for (CommandLineAndParamFileInfo pair : this.commandLines) {

src/main/java/com/google/devtools/build/lib/rules/java/AndroidLintActionBuilder.java

@@ -116,10 +116,13 @@ static Artifact create(
     cmd.addExecPath("--xml", result);
 
     NestedSetBuilder<Artifact> toolInputs = NestedSetBuilder.stableOrder();
-    androidLint.tool().buildCommandLine(spawnAction.executableArguments(), toolchain, toolInputs);
+    androidLint.tool().addInputs(toolchain, toolInputs);
+
     semantics.setLintProgressMessage(spawnAction);
     ruleContext.registerAction(
         spawnAction
+            .addCommandLine(
+                androidLint.tool().getCommandLine(toolchain, /* stripOutputPath= */ null))
             .addCommandLine(cmd.build(), PARAM_FILE_INFO)
             .addInputs(attributes.getSourceFiles())
             .addInputs(allSrcJars)

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

@@ -200,6 +200,7 @@ java_library(
         "//src/main/protobuf:extra_actions_base_java_proto",
         "//src/main/protobuf:failure_details_java_proto",
         "//third_party:auto_value",
+        "//third_party:caffeine",
         "//third_party:flogger",
         "//third_party:guava",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java

@@ -203,9 +203,7 @@ public JavaCompileAction build() {
     }
 
     NestedSetBuilder<Artifact> toolsBuilder = NestedSetBuilder.compileOrder();
-
-    CustomCommandLine.Builder executableLine =
-        javaBuilder.buildCommandLine(toolchain, toolsBuilder);
+    javaBuilder.addInputs(toolchain, toolsBuilder);
     toolsBuilder.addTransitive(toolsJars);
 
     ActionEnvironment actionEnvironment =
@@ -270,9 +268,10 @@ public JavaCompileAction build() {
             JavaCompileAction.allInputs(
                 mandatoryInputs, classpathEntries, compileTimeDependencyArtifacts),
             ruleContext.getConfiguration());
-    if (stripOutputPaths) {
-      executableLine.stripOutputPaths(JavaCompilationHelper.outputBase(outputs.output()));
-    }
+    CustomCommandLine executableLine =
+        javaBuilder.getCommandLine(
+            toolchain,
+            stripOutputPaths ? JavaCompilationHelper.outputBase(outputs.output()) : null);
 
     return new JavaCompileAction(
         /* compilationType= */ JavaCompileAction.CompilationType.JAVAC,
@@ -292,7 +291,7 @@ public JavaCompileAction build() {
         /* outputs= */ allOutputs(),
         /* executionInfo= */ executionInfo,
         /* extraActionInfoSupplier= */ extraActionInfoSupplier,
-        /* executableLine= */ executableLine.build(),
+        /* executableLine= */ executableLine,
         /* flagLine= */ buildParamFileContents(internedJcopts, stripOutputPaths),
         /* configuration= */ ruleContext.getConfiguration(),
         /* dependencyArtifacts= */ compileTimeDependencyArtifacts,

src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java

@@ -355,8 +355,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
             : javaToolchain.getHeaderCompiler();
     // The header compiler is either a jar file that needs to be executed using
     // `java -jar <path>`, or an executable that can be run directly.
-    CustomCommandLine.Builder executableLine =
-        headerCompiler.buildCommandLine(javaToolchain, mandatoryInputsBuilder);
+    headerCompiler.addInputs(javaToolchain, mandatoryInputsBuilder);
     CustomCommandLine.Builder commandLine =
         CustomCommandLine.builder()
             .addExecPath("--output", outputJar)
@@ -409,6 +408,11 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
       NestedSet<Artifact> allInputs = mandatoryInputsBuilder.build();
       boolean stripOutputPaths =
           JavaCompilationHelper.stripOutputPaths(allInputs, ruleContext.getConfiguration());
+      @Nullable
+      PathFragment strippedOutputBase =
+          stripOutputPaths ? JavaCompilationHelper.outputBase(outputJar) : null;
+      CustomCommandLine executableLine =
+          headerCompiler.getCommandLine(javaToolchain, strippedOutputBase);
       Consumer<Pair<ActionExecutionContext, List<SpawnResult>>> resultConsumer =
           createResultConsumer(
               outputDepsProto,
@@ -419,10 +423,8 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
               /*insertDependencies=*/ classpathMode == JavaClasspathMode.BAZEL,
               stripOutputPaths);
 
-      if (stripOutputPaths) {
-        PathFragment outputBase = JavaCompilationHelper.outputBase(outputJar);
-        commandLine.stripOutputPaths(outputBase);
-        executableLine.stripOutputPaths(outputBase);
+      if (strippedOutputBase != null) {
+        commandLine.stripOutputPaths(strippedOutputBase);
       }
 
       ruleContext.registerAction(
@@ -434,7 +436,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
               /* primaryOutput= */ outputJar,
               /* resourceSetOrBuilder= */ AbstractAction.DEFAULT_RESOURCE_SET,
               /* commandLines= */ CommandLines.builder()
-                  .addCommandLine(executableLine.build())
+                  .addCommandLine(executableLine)
                   .addCommandLine(commandLine.build(), PARAM_FILE_INFO)
                   .build(),
               /* commandLineLimits= */ ruleContext.getConfiguration().getCommandLineLimits(),
@@ -449,7 +451,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
               /* executeUnconditionally= */ false,
               /* extraActionInfoSupplier= */ null,
               /* resultConsumer= */ resultConsumer,
-              /*stripOutputPaths= */ stripOutputPaths));
+              /* stripOutputPaths= */ stripOutputPaths));
       return;
     }
 
@@ -476,19 +478,23 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
     }
 
     NestedSet<Artifact> mandatoryInputs = mandatoryInputsBuilder.build();
-    boolean stripOutputPaths =
+
+    boolean pathStrippingEnabled =
         JavaCompilationHelper.stripOutputPaths(
             NestedSetBuilder.<Artifact>stableOrder()
                 .addTransitive(mandatoryInputs)
                 .addTransitive(classpathEntries)
                 .addTransitive(compileTimeDependencyArtifacts)
                 .build(),
             ruleContext.getConfiguration());
-    if (stripOutputPaths) {
-      PathFragment outputBase = JavaCompilationHelper.outputBase(outputJar);
-      commandLine.stripOutputPaths(outputBase);
-      executableLine.stripOutputPaths(outputBase);
+
+    PathFragment strippedOutputBase =
+        pathStrippingEnabled ? JavaCompilationHelper.outputBase(outputJar) : null;
+    if (strippedOutputBase != null) {
+      commandLine.stripOutputPaths(strippedOutputBase);
     }
+    CustomCommandLine executableLine =
+        headerCompiler.getCommandLine(javaToolchain, strippedOutputBase);
 
     ruleContext.registerAction(
         new JavaCompileAction(
@@ -504,7 +510,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti
             /* outputs= */ outputs.build(),
             /* executionInfo= */ executionInfo,
             /* extraActionInfoSupplier= */ null,
-            /* executableLine= */ executableLine.build(),
+            /* executableLine= */ executableLine,
             /* flagLine= */ commandLine.build(),
             /* configuration= */ ruleContext.getConfiguration(),
             /* dependencyArtifacts= */ compileTimeDependencyArtifacts,

src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java

@@ -16,6 +16,8 @@
 
 import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
@@ -30,6 +32,8 @@
 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.util.Pair;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
 
 /** An executable tool that is part of {@code java_toolchain}. */
@@ -48,6 +52,22 @@ public abstract class JavaToolchainTool {
    */
   public abstract NestedSet<String> jvmOpts();
 
+  /**
+   * Cache for the {@link CustomCommandLine} for a given tool.
+   *
+   * <p>Practically, every {@link JavaToolchainTool} is used with only 1 {@link
+   * JavaToolchainProvider} hence the initial capacity of 2 (path stripping on/off).
+   *
+   * <p>Using soft values since the main benefit is to share the command line between different
+   * actions, in which case the {@link CustomCommandLine} object remains strongly reachable anyway.
+   */
+  private final transient LoadingCache<Pair<JavaToolchainProvider, PathFragment>, CustomCommandLine>
+      commandLineCache =
+          Caffeine.newBuilder()
+              .initialCapacity(2)
+              .softValues()
+              .build(key -> extractCommandLine(key.first, key.second));
+
   @Nullable
   static JavaToolchainTool fromRuleContext(
       RuleContext ruleContext,
@@ -92,44 +112,50 @@ private static JavaToolchainTool create(
   }
 
   /**
-   * Builds the executable command line for the tool and adds its inputs to the given input builder.
+   * Returns the executable command line for the tool.
    *
    * <p>For a Java command, the executable command line will include {@code java -jar deploy.jar} as
    * well as any JVM flags.
    *
-   * @param command the executable command line builder for the tool
    * @param toolchain {@code java_toolchain} for the action being constructed
-   * @param inputs inputs for the action being constructed
+   * @param stripOutputPath output tree root fragment to use for path stripping or null if disabled.
    */
-  void buildCommandLine(
-      CustomCommandLine.Builder command,
-      JavaToolchainProvider toolchain,
-      NestedSetBuilder<Artifact> inputs) {
-    inputs.addTransitive(data());
+  CustomCommandLine getCommandLine(
+      JavaToolchainProvider toolchain, @Nullable PathFragment stripOutputPath) {
+    return commandLineCache.get(Pair.of(toolchain, stripOutputPath));
+  }
+
+  private CustomCommandLine extractCommandLine(
+      JavaToolchainProvider toolchain, @Nullable PathFragment stripOutputPath) {
+    CustomCommandLine.Builder command = CustomCommandLine.builder();
+
     Artifact executable = tool().getExecutable();
     if (!executable.getExtension().equals("jar")) {
       command.addExecPath(executable);
-      inputs.addTransitive(tool().getFilesToRun());
     } else {
-      inputs.add(executable).addTransitive(toolchain.getJavaRuntime().javaBaseInputs());
       command
           .addPath(toolchain.getJavaRuntime().javaBinaryExecPathFragment())
           .addAll(toolchain.getJvmOptions())
           .addAll(jvmOpts())
           .add("-jar")
           .addPath(executable.getExecPath());
     }
+
+    if (stripOutputPath != null) {
+      command.stripOutputPaths(stripOutputPath);
+    }
+
+    return command.build();
   }
 
-  /**
-   * Builds the executable command line for the tool and adds its inputs to the given builder, see
-   * also {@link #buildCommandLine(CustomCommandLine.Builder, JavaToolchainProvider,
-   * NestedSetBuilder)}.
-   */
-  CustomCommandLine.Builder buildCommandLine(
-      JavaToolchainProvider toolchain, NestedSetBuilder<Artifact> inputs) {
-    CustomCommandLine.Builder command = CustomCommandLine.builder();
-    buildCommandLine(command, toolchain, inputs);
-    return command;
+  /** Adds its inputs for the tool to provided input builder. */
+  void addInputs(JavaToolchainProvider toolchain, NestedSetBuilder<Artifact> inputs) {
+    inputs.addTransitive(data());
+    Artifact executable = tool().getExecutable();
+    if (!executable.getExtension().equals("jar")) {
+      inputs.addTransitive(tool().getFilesToRun());
+    } else {
+      inputs.add(executable).addTransitive(toolchain.getJavaRuntime().javaBaseInputs());
+    }
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java

@@ -313,6 +313,18 @@ public void testBuilderWithExtraExecutableArguments() throws Exception {
             "arg1");
   }
 
+  @Test
+  public void testBuilderWithNoExecutableCommand_buildsActionWithCorrectArgs() throws Exception {
+    SpawnAction action =
+        builder()
+            .addOutput(getBinArtifactWithNoOwner("output"))
+            .addCommandLine(CommandLine.of(ImmutableList.of("arg1", "arg2")))
+            .addCommandLine(CommandLine.of(ImmutableList.of("arg3")))
+            .build(ActionsTestUtil.NULL_ACTION_OWNER, targetConfig);
+
+    assertThat(action.getArguments()).containsExactly("arg1", "arg2", "arg3").inOrder();
+  }
+
   @Test
   public void testMultipleCommandLines() throws Exception {
     Artifact input = getSourceArtifact("input");