From 187f3e499060a467db65087ef9dd89b172c8aa40 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 22 Mar 2023 13:34:36 -0700 Subject: [PATCH] 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 --- .../lib/analysis/actions/SpawnAction.java | 4 +- .../rules/java/AndroidLintActionBuilder.java | 5 +- .../devtools/build/lib/rules/java/BUILD | 1 + .../rules/java/JavaCompileActionBuilder.java | 13 ++-- .../java/JavaHeaderCompileActionBuilder.java | 34 ++++++---- .../lib/rules/java/JavaToolchainTool.java | 66 +++++++++++++------ .../lib/analysis/actions/SpawnActionTest.java | 12 ++++ 7 files changed, 91 insertions(+), 44 deletions(-) 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 d85fa4d1d40d60..c6cf7f5162e74d 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 @@ -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 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) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/AndroidLintActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/AndroidLintActionBuilder.java index f24ef5e713e855..abc19aed79a220 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/AndroidLintActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/AndroidLintActionBuilder.java @@ -116,10 +116,13 @@ static Artifact create( cmd.addExecPath("--xml", result); NestedSetBuilder 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) diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD index 2d8d0866ef2129..08d2620a3e7a33 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 3f065a26d1f41d..4410e46f421ad7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -203,9 +203,7 @@ public JavaCompileAction build() { } NestedSetBuilder 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, 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 d40331439962b4..492db60c121c38 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 @@ -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 `, 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 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>> 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,7 +478,8 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti } NestedSet mandatoryInputs = mandatoryInputsBuilder.build(); - boolean stripOutputPaths = + + boolean pathStrippingEnabled = JavaCompilationHelper.stripOutputPaths( NestedSetBuilder.stableOrder() .addTransitive(mandatoryInputs) @@ -484,11 +487,14 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti .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, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java index 48353556053a80..5976ad9e8a2aa2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java +++ b/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 jvmOpts(); + /** + * Cache for the {@link CustomCommandLine} for a given tool. + * + *

Practically, every {@link JavaToolchainTool} is used with only 1 {@link + * JavaToolchainProvider} hence the initial capacity of 2 (path stripping on/off). + * + *

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, CustomCommandLine> + commandLineCache = + Caffeine.newBuilder() + .initialCapacity(2) + .softValues() + .build(key -> extractCommandLine(key.first, key.second)); + @Nullable static JavaToolchainTool fromRuleContext( RuleContext ruleContext, @@ -92,26 +112,27 @@ 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. * *

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 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()) @@ -119,17 +140,22 @@ void buildCommandLine( .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 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 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()); + } } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java index bb67aec9bf6187..b0b3c6e941ea0d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java +++ b/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");