From 6ddc6229812c9da7477a69399b1e61c3d176494e Mon Sep 17 00:00:00 2001
From: Googler <noreply@google.com>
Date: Wed, 24 Apr 2019 17:28:05 -0700
Subject: [PATCH] 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
---
 .../lib/analysis/config/BuildOptions.java     | 44 +++++++++++++----
 .../lib/analysis/config/BuildOptionsTest.java | 47 ++++++++++++-------
 2 files changed, 66 insertions(+), 25 deletions(-)

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 ba4e6ac525c8b7..a64215d530b8a4 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 @@ public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
     }
     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 @@ public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
       }
     }
     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 @@ public static OptionsDiffForReconstruction diffForReconstruction(
         second.computeChecksum(),
         diff.skylarkSecond,
         diff.extraStarlarkOptionsFirst,
-        diff.extraStarlarkOptionsSecond);
+        diff.extraStarlarkOptionsSecond,
+        second);
   }
 
   /**
@@ -827,7 +834,15 @@ public static final class OptionsDiffForReconstruction {
     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 @@ public OptionsDiffForReconstruction(
         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 @@ public OptionsDiffForReconstruction(
       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 @@ private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, Str
           checksum,
           ImmutableMap.of(),
           ImmutableList.of(),
-          ImmutableMap.of());
+          ImmutableMap.of(),
+          /*original=*/ null);
     }
 
     @Nullable
@@ -970,6 +988,15 @@ public static ConfigurationComparer.Result compareFragments(
       }
     }
 
+    /**
+     * 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 @@ public OptionsDiffForReconstruction deserialize(
                   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 326b2eb16050d8..2d97e617ff2d98 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 @@ public void optionsEquality() throws Exception {
             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 @@ public void optionsDiff() throws Exception {
 
   @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 @@ public void optionsDiff_differentBaseThrowException() throws Exception {
         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 optionsDiff_getEmptyAndApplyEmpty() throws Exception {
   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 @@ public void applyDiff_nativeOptions() throws Exception {
   }
 
   @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 @@ public void optionsDiff_sameStarlarkOptions() throws Exception {
   }
 
   @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 @@ public void optionsDiff_differentStarlarkOptions() throws Exception {
   }
 
   @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 @@ public void optionsDiff_extraStarlarkOptions() throws Exception {
   }
 
   @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 @@ public void applyDiff_sameStarlarkOptions() throws Exception {
   }
 
   @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 @@ public void repeatedCodec() throws Exception {
   }
 
   @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 @@ public void trim_nothingTrimmed_returnsSameInstance() throws Exception {
     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;
+  }
 }