When trimming BuildOptions, return the same instance if there are no changes.
On a typical build, thousands of value-equal BuildOptions instances are being created. In addition to consuming memory, this is causing BuildOptions to go through its (relatively expensive) hashCode/fingerprint initialization many times.
An added bonus comes from not calling toString on the FragmentOptions instance, which requires reflectively iterating over all of its options fields.
PiperOrigin-RevId: 244853750
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
index 1838331..a8ac70f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
import com.google.common.collect.MapDifference;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
@@ -91,19 +92,29 @@
}
/**
- * Returns an equivalent instance to this one with only options from the given {@link
- * FragmentOptions} classes.
+ * Returns {@code BuildOptions} that are otherwise identical to this one, but contain only options
+ * from the given {@link FragmentOptions} classes (plus build configuration options).
+ *
+ * <p>If nothing needs to be trimmed, this instance is returned.
*/
public BuildOptions trim(Set<Class<? extends FragmentOptions>> optionsClasses) {
- Builder builder = builder();
+ List<FragmentOptions> retainedOptions =
+ Lists.newArrayListWithExpectedSize(optionsClasses.size() + 1);
for (FragmentOptions options : fragmentOptionsMap.values()) {
if (optionsClasses.contains(options.getClass())
// TODO(bazel-team): make this non-hacky while not requiring BuildConfiguration access
// to BuildOptions.
- || options.toString().contains("BuildConfiguration$Options")) {
- builder.addFragmentOptions(options);
+ || options.getClass().getName().endsWith("BuildConfiguration$Options")) {
+ retainedOptions.add(options);
}
}
+ if (retainedOptions.size() == fragmentOptionsMap.size()) {
+ return this; // Nothing to trim.
+ }
+ Builder builder = builder();
+ for (FragmentOptions options : retainedOptions) {
+ builder.addFragmentOptions(options);
+ }
return builder.addStarlarkOptions(skylarkOptionsMap).build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
index db47f96..326b2eb 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options;
import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
@@ -507,4 +508,28 @@
assertThat(original.matches(parser)).isFalse();
}
+
+ @Test
+ public void trim() throws Exception {
+ BuildOptions original = BuildOptions.of(ImmutableList.of(CppOptions.class, JavaOptions.class));
+ BuildOptions trimmed = original.trim(ImmutableSet.of(CppOptions.class));
+ assertThat(trimmed.getFragmentClasses()).containsExactly(CppOptions.class);
+ }
+
+ @Test
+ public void trim_retainsBuildConfigurationOptions() throws Exception {
+ BuildOptions original =
+ BuildOptions.of(
+ ImmutableList.of(
+ CppOptions.class, JavaOptions.class, BuildConfiguration.Options.class));
+ BuildOptions trimmed = original.trim(ImmutableSet.of());
+ assertThat(trimmed.getFragmentClasses()).containsExactly(BuildConfiguration.Options.class);
+ }
+
+ @Test
+ public void trim_nothingTrimmed_returnsSameInstance() throws Exception {
+ BuildOptions original = BuildOptions.of(ImmutableList.of(CppOptions.class, JavaOptions.class));
+ BuildOptions trimmed = original.trim(ImmutableSet.of(CppOptions.class, JavaOptions.class));
+ assertThat(trimmed).isSameAs(original);
+ }
}
