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;
+ }
}
