Utilize `CustomCommandLine#addObject` to add a `Collection` without copying over its elements.
This is more memory efficient if the `Collection` is retained anyway, since we spend a single array slot on a pointer instead of an array slot for each element. It works straightaway because `CustomCommandLine` already has handling for lazily "expanding" `Iterable`.
This change only affects Java compilation actions, as they are the only case I found which is currently wastefully copying lists. To maximize the benefit, structure the `JavaHeaderCompileAction` builder to accept an existing `ImmutableList<String>` of Javac options instead of creating its own.
PiperOrigin-RevId: 535304666
Change-Id: Id41af250916e95e87fd69bcdc1bfda1d3bc1421f
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index a415729..f1d9729 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -780,8 +780,15 @@
/**
* Adds a single argument to the command line, which is lazily converted to string.
*
- * <p>If the value is null, neither the arg nor the value is added.
+ * <p>If the value is null, this method is a no-op.
+ *
+ * <p>Passing a {@link Collection} containing multiple elements to this method instead of {@link
+ * #addAll(Collection)} and similar is preferable if the caller knows that the given instance
+ * will be retained elsewhere. This method spends a single array slot on the {@link Collection}
+ * instead of copying over all of its elements, potentially saving memory if it is retained
+ * elsewhere.
*/
+ @CanIgnoreReturnValue
public Builder addObject(@Nullable Object value) {
return addObjectInternal(value);
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
index e068205..c23d308b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
@@ -554,8 +554,8 @@
.processorClasses()
.toList()
.contains("dagger.internal.codegen.ComponentProcessor")) {
- // see b/31371210
- builder.addJavacOpt("-Aexperimental_turbine_hjar");
+ // See b/31371210 and b/142059842.
+ builder.addTurbineHjarJavacOpt();
}
builder.enableDirectClasspath(enableDirectClasspath);
builder.build(javaToolchain);
@@ -566,7 +566,7 @@
private JavaHeaderCompileAction.Builder getJavaHeaderCompileActionBuilder() {
JavaTargetAttributes attributes = getAttributes();
- JavaHeaderCompileAction.Builder builder = JavaHeaderCompileAction.newBuilder(getRuleContext());
+ JavaHeaderCompileAction.Builder builder = JavaHeaderCompileAction.newBuilder(ruleContext);
builder.setSourceFiles(attributes.getSourceFiles());
builder.setSourceJars(attributes.getSourceJars());
builder.setClasspathEntries(attributes.getCompileTimeClassPath());
@@ -574,7 +574,7 @@
// Exclude any per-package configured data (see JavaCommon.computePerPackageData).
// It is used to allow Error Prone checks to load additional data,
// and Error Prone doesn't run during header compilation.
- builder.addAllJavacOpts(getJavacOpts());
+ builder.setJavacOpts(customJavacOpts);
builder.setStrictJavaDeps(attributes.getStrictJavaDeps());
builder.setCompileTimeDependencyArtifacts(attributes.getCompileTimeDependencyArtifacts());
builder.setDirectJars(attributes.getDirectJars());
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 f2ee559f..e79a0ab 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
@@ -322,7 +322,7 @@
result.addAll(
"--builtin_processors",
Sets.intersection(plugins.processorClasses().toSet(), builtinProcessorNames));
- result.addExecPaths("--source_jars", ImmutableList.copyOf(sourceJars));
+ result.addExecPaths("--source_jars", sourceJars);
result.addExecPaths("--sources", sourceFiles);
if (!javacOpts.isEmpty()) {
if (stripOutputPaths) {
@@ -338,7 +338,7 @@
"--javacopts",
javacOpts.stream().map(coptStripper::strip).collect(Collectors.toList()));
} else {
- result.addAll("--javacopts", javacOpts);
+ result.add("--javacopts").addObject(javacOpts);
}
// terminate --javacopts with `--` to support javac flags that start with `--`
result.add("--");
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
index 950d2e1..5bf8e62 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
@@ -155,7 +155,8 @@
private NestedSet<Artifact> directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
private NestedSet<Artifact> compileTimeDependencyArtifacts =
NestedSetBuilder.emptySet(Order.STABLE_ORDER);
- private final ImmutableList.Builder<String> javacOptsBuilder = ImmutableList.builder();
+ private ImmutableList<String> javacOpts = ImmutableList.of();
+ private boolean addTurbineHjarJavacOpt = false;
private JavaPluginData plugins = JavaPluginData.empty();
private ImmutableList<Artifact> additionalInputs = ImmutableList.of();
@@ -210,17 +211,20 @@
return this;
}
- /** Adds Java compiler flags. */
+ /** Sets Java compiler flags. */
@CanIgnoreReturnValue
- public Builder addAllJavacOpts(Iterable<String> javacOpts) {
- this.javacOptsBuilder.addAll(javacOpts);
+ public Builder setJavacOpts(ImmutableList<String> javacOpts) {
+ this.javacOpts = checkNotNull(javacOpts);
return this;
}
- /** Adds a Java compiler flag. */
+ /**
+ * Adds {@code -Aexperimental_turbine_hjar} to Java compiler flags without creating an entirely
+ * new list.
+ */
@CanIgnoreReturnValue
- public Builder addJavacOpt(String javacOpt) {
- this.javacOptsBuilder.add(javacOpt);
+ public Builder addTurbineHjarJavacOpt() {
+ this.addTurbineHjarJavacOpt = true;
return this;
}
@@ -342,8 +346,6 @@
checkNotNull(
compileTimeDependencyArtifacts, "compileTimeDependencyArtifacts must not be null");
- ImmutableList<String> javacOpts = javacOptsBuilder.build();
-
// Invariant: if strictJavaDeps is OFF, then directJars and
// dependencyArtifacts are ignored
if (strictJavaDeps == StrictDepsMode.OFF) {
@@ -424,8 +426,14 @@
.addExecPaths("--source_jars", sourceJars)
.add("--injecting_rule_kind", injectingRuleKind);
- if (!javacOpts.isEmpty()) {
- commandLine.addAll("--javacopts", javacOpts);
+ if (!javacOpts.isEmpty() || addTurbineHjarJavacOpt) {
+ commandLine.add("--javacopts");
+ if (!javacOpts.isEmpty()) {
+ commandLine.addObject(javacOpts);
+ }
+ if (addTurbineHjarJavacOpt) {
+ commandLine.add("-Aexperimental_turbine_hjar");
+ }
// terminate --javacopts with `--` to support javac flags that start with `--`
commandLine.add("--");
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/SingleJarActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/SingleJarActionBuilder.java
index df63e9c..15289fd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/SingleJarActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/SingleJarActionBuilder.java
@@ -146,7 +146,7 @@
NestedSet<Artifact> resourceJars) {
CustomCommandLine.Builder args = CustomCommandLine.builder();
args.addExecPath("--output", outputJar);
- args.addAll(SOURCE_JAR_COMMAND_LINE_ARGS);
+ args.addObject(SOURCE_JAR_COMMAND_LINE_ARGS);
args.addExecPaths("--sources", resourceJars);
if (!resources.isEmpty()) {
args.add("--resources");
