Cache a soft reference to the reconstituted BuildOptions in OptionsDiffForReconstruction. This makes it so that applyDiff usually does no work and also reduces garbage creation. Although I do not think the results of applyDiff are currently being compared with equals/hashCode anywhere, this also prevents expensive re-initialization of fingerprint/hashCode if they ever are. PiperOrigin-RevId: 245148847
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 ba4e6ac..a64215d 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
@@ -50,6 +50,7 @@ import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.io.Serializable; +import java.lang.ref.SoftReference; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -307,7 +308,11 @@ } maybeInitializeFingerprintAndHashCode(); if (!Arrays.equals(fingerprint, optionsDiff.baseFingerprint)) { - throw new IllegalArgumentException("Can not reconstruct BuildOptions with a different base."); + throw new IllegalArgumentException("Cannot reconstruct BuildOptions with a different base."); + } + BuildOptions reconstructedOptions = optionsDiff.cachedReconstructed.get(); + if (reconstructedOptions != null) { + return reconstructedOptions; } Builder builder = builder(); for (FragmentOptions options : fragmentOptionsMap.values()) { @@ -332,8 +337,9 @@ } } skylarkOptions.putAll(optionsDiff.extraSecondStarlarkOptions); - builder.addStarlarkOptions(skylarkOptions); - return builder.build(); + reconstructedOptions = builder.addStarlarkOptions(skylarkOptions).build(); + optionsDiff.cachedReconstructed = new SoftReference<>(reconstructedOptions); + return reconstructedOptions; } /** @@ -673,7 +679,8 @@ second.computeChecksum(), diff.skylarkSecond, diff.extraStarlarkOptionsFirst, - diff.extraStarlarkOptionsSecond); + diff.extraStarlarkOptionsSecond, + second); } /** @@ -827,7 +834,15 @@ private final List<Label> extraFirstStarlarkOptions; private final Map<Label, Object> extraSecondStarlarkOptions; - @VisibleForTesting + /** + * A soft reference to the reconstructed build options to save work and garbage creation in + * {@link #applyDiff}. + * + * <p>Promotes reuse of a single {@code BuildOptions} instance to preserve reference equality + * and limit fingerprint/hashCode initialization. + */ + private SoftReference<BuildOptions> cachedReconstructed; + public OptionsDiffForReconstruction( Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions, ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses, @@ -836,7 +851,8 @@ String checksum, Map<Label, Object> differingStarlarkOptions, List<Label> extraFirstStarlarkOptions, - Map<Label, Object> extraSecondStarlarkOptions) { + Map<Label, Object> extraSecondStarlarkOptions, + @Nullable BuildOptions original) { this.differingOptions = differingOptions; this.extraFirstFragmentClasses = extraFirstFragmentClasses; this.extraSecondFragments = extraSecondFragments; @@ -845,6 +861,7 @@ this.differingStarlarkOptions = differingStarlarkOptions; this.extraFirstStarlarkOptions = extraFirstStarlarkOptions; this.extraSecondStarlarkOptions = extraSecondStarlarkOptions; + this.cachedReconstructed = new SoftReference<>(original); } private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, String checksum) { @@ -856,7 +873,8 @@ checksum, ImmutableMap.of(), ImmutableList.of(), - ImmutableMap.of()); + ImmutableMap.of(), + /*original=*/ null); } @Nullable @@ -970,6 +988,15 @@ } } + /** + * Clears {@link #cachedReconstructed} so that tests can cover the core logic of {@link + * #applyDiff}. + */ + @VisibleForTesting + void clearCachedReconstructedForTesting() { + cachedReconstructed = new SoftReference<>(null); + } + private boolean hasChangeToStarlarkOptionUnchangedIn(OptionsDiffForReconstruction that) { Set<Label> starlarkOptionsChangedOrRemovedInThat = Sets.union( @@ -1133,7 +1160,8 @@ checksum, differingStarlarkOptions, extraFirstStarlarkOptions, - extraSecondStarlarkOptions); + extraSecondStarlarkOptions, + /*original=*/ null); cache.putBytesFromOptionsDiff(diff, bytes); } return diff;
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 326b2eb..2d97e61 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
@@ -45,7 +45,7 @@ * skylark options, the format of this test class will need to accommodate that overlap. */ @RunWith(JUnit4.class) -public class BuildOptionsTest { +public final class BuildOptionsTest { private static final ImmutableList<Class<? extends FragmentOptions>> BUILD_CONFIG_OPTIONS = ImmutableList.of(BuildConfiguration.Options.class); @@ -87,8 +87,7 @@ BuildOptions.of(BUILD_CONFIG_OPTIONS, options1) .equals( BuildOptions.of( - ImmutableList.<Class<? extends FragmentOptions>>of( - BuildConfiguration.Options.class, CppOptions.class), + ImmutableList.of(BuildConfiguration.Options.class, CppOptions.class), options1))) .isFalse(); } @@ -112,8 +111,7 @@ @Test public void optionsDiff_differentFragments() throws Exception { - BuildOptions one = - BuildOptions.of(ImmutableList.<Class<? extends FragmentOptions>>of(CppOptions.class)); + BuildOptions one = BuildOptions.of(ImmutableList.of(CppOptions.class)); BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS); OptionsDiff diff = BuildOptions.diff(one, two); @@ -160,7 +158,7 @@ assertThrows(IllegalArgumentException.class, () -> three.applyDiff(diffForReconstruction)); assertThat(e) .hasMessageThat() - .contains("Can not reconstruct BuildOptions with a different base"); + .contains("Cannot reconstruct BuildOptions with a different base"); } @Test @@ -177,7 +175,7 @@ public void applyDiff_nativeOptions() throws Exception { BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8"); BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8"); - BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two)); + BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); assertThat(reconstructedTwo).isEqualTo(two); assertThat(reconstructedTwo).isNotSameAs(two); BuildOptions reconstructedOne = one.applyDiff(BuildOptions.diffForReconstruction(one, one)); @@ -190,7 +188,7 @@ } @Test - public void optionsDiff_sameStarlarkOptions() throws Exception { + public void optionsDiff_sameStarlarkOptions() { Label flagName = Label.parseAbsoluteUnchecked("//foo/flag"); String flagValue = "value"; BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValue)); @@ -200,7 +198,7 @@ } @Test - public void optionsDiff_differentStarlarkOptions() throws Exception { + public void optionsDiff_differentStarlarkOptions() { Label flagName = Label.parseAbsoluteUnchecked("//bar/flag"); String flagValueOne = "valueOne"; String flagValueTwo = "valueTwo"; @@ -218,7 +216,7 @@ } @Test - public void optionsDiff_extraStarlarkOptions() throws Exception { + public void optionsDiff_extraStarlarkOptions() { Label flagNameOne = Label.parseAbsoluteUnchecked("//extra/flag/one"); Label flagNameTwo = Label.parseAbsoluteUnchecked("//extra/flag/two"); String flagValue = "foo"; @@ -234,13 +232,13 @@ } @Test - public void applyDiff_sameStarlarkOptions() throws Exception { + public void applyDiff_sameStarlarkOptions() { Label flagName = Label.parseAbsoluteUnchecked("//foo/flag"); String flagValue = "value"; BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValue)); BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValue)); - BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two)); + BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); assertThat(reconstructedTwo).isEqualTo(two); assertThat(reconstructedTwo).isNotSameAs(two); @@ -251,33 +249,41 @@ } @Test - public void applyDiff_differentStarlarkOptions() throws Exception { + public void applyDiff_differentStarlarkOptions() { Label flagName = Label.parseAbsoluteUnchecked("//bar/flag"); String flagValueOne = "valueOne"; String flagValueTwo = "valueTwo"; BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValueOne)); BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValueTwo)); - BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two)); + BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); assertThat(reconstructedTwo).isEqualTo(two); assertThat(reconstructedTwo).isNotSameAs(two); } @Test - public void applyDiff_extraStarlarkOptions() throws Exception { + public void applyDiff_extraStarlarkOptions() { Label flagNameOne = Label.parseAbsoluteUnchecked("//extra/flag/one"); Label flagNameTwo = Label.parseAbsoluteUnchecked("//extra/flag/two"); String flagValue = "foo"; BuildOptions one = BuildOptions.of(ImmutableMap.of(flagNameOne, flagValue)); BuildOptions two = BuildOptions.of(ImmutableMap.of(flagNameTwo, flagValue)); - BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two)); + BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two)); assertThat(reconstructedTwo).isEqualTo(two); assertThat(reconstructedTwo).isNotSameAs(two); } + @Test + public void applyDiff_returnsOriginalOptionsInstanceIfStillExists() throws Exception { + BuildOptions base = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=a"); + BuildOptions original = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=b"); + BuildOptions reconstructed = base.applyDiff(BuildOptions.diffForReconstruction(base, original)); + assertThat(reconstructed).isSameAs(original); + } + private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() { return ImmutableList.<Class<? extends FragmentOptions>>builder() .addAll(BUILD_CONFIG_OPTIONS) @@ -334,7 +340,7 @@ } @Test - public void testMultiValueOptionImmutability() throws Exception { + public void testMultiValueOptionImmutability() { BuildOptions options = BuildOptions.of(BUILD_CONFIG_OPTIONS, OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS)); BuildConfiguration.Options coreOptions = options.get(Options.class); @@ -532,4 +538,11 @@ BuildOptions trimmed = original.trim(ImmutableSet.of(CppOptions.class, JavaOptions.class)); assertThat(trimmed).isSameAs(original); } + + private static OptionsDiffForReconstruction uncachedDiffForReconstruction( + BuildOptions first, BuildOptions second) { + OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(first, second); + diff.clearCachedReconstructedForTesting(); + return diff; + } }