Cache BuildOptions.diffForReconstruction.
PiperOrigin-RevId: 245323196
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 a64215d..ad6bf3c 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
@@ -17,6 +17,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -63,6 +65,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
@@ -629,11 +632,40 @@
}
/**
+ * Cache for {@link OptionsDiffForReconstruction}, which is expensive to compute.
+ *
+ * <p>The reason for using {@linkplain CacheBuilder#weakKeys weak keys} is twofold: we want
+ * objects in the cache to be garbage collected, and we also want to use reference equality to
+ * avoid the expensive initialization in {@link #maybeInitializeFingerprintAndHashCode}.
+ */
+ private static final Cache<BuildOptions, OptionsDiffForReconstruction>
+ diffForReconstructionCache = CacheBuilder.newBuilder().weakKeys().build();
+
+ /**
* Returns a {@link OptionsDiffForReconstruction} object that can be applied to {@code first} via
* {@link #applyDiff} to get a {@link BuildOptions} object equal to {@code second}.
*/
public static OptionsDiffForReconstruction diffForReconstruction(
BuildOptions first, BuildOptions second) {
+ OptionsDiffForReconstruction diff;
+ try {
+ diff =
+ diffForReconstructionCache.get(second, () -> createDiffForReconstruction(first, second));
+ } catch (ExecutionException e) {
+ throw new IllegalStateException(e);
+ }
+
+ // We need to ensure that the possibly cached diff was computed against the same base options.
+ // In practice this should always be the case, since callers pass in a "default" options
+ // instance as "first". To be safe however, we create an uncached diff if there is a mismatch.
+ // Note that this check should be fast because the fingerprints should be reference-equal.
+ return Arrays.equals(first.fingerprint, diff.baseFingerprint)
+ ? diff
+ : createDiffForReconstruction(first, second);
+ }
+
+ private static OptionsDiffForReconstruction createDiffForReconstruction(
+ BuildOptions first, BuildOptions second) {
OptionsDiff diff = diff(first, second);
if (diff.areSame()) {
first.maybeInitializeFingerprintAndHashCode();
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 2d97e61..0b7d95f 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
@@ -284,6 +284,31 @@
assertThat(reconstructed).isSameAs(original);
}
+ @Test
+ public void diffForReconstruction_calledTwiceWithSameArgs_returnsSameInstance() throws Exception {
+ BuildOptions one = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=one");
+ BuildOptions two = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=two");
+
+ OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(one, two);
+ OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(one, two);
+
+ assertThat(diff1).isSameAs(diff2);
+ }
+
+ @Test
+ public void diffForReconstruction_againstDifferentBase() throws Exception {
+ BuildOptions base1 = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=base1");
+ BuildOptions base2 = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=base2");
+ BuildOptions other = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=other");
+
+ OptionsDiffForReconstruction diff1 = BuildOptions.diffForReconstruction(base1, other);
+ OptionsDiffForReconstruction diff2 = BuildOptions.diffForReconstruction(base2, other);
+
+ assertThat(diff1).isNotEqualTo(diff2);
+ assertThat(base1.applyDiff(diff1)).isEqualTo(other);
+ assertThat(base2.applyDiff(diff2)).isEqualTo(other);
+ }
+
private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() {
return ImmutableList.<Class<? extends FragmentOptions>>builder()
.addAll(BUILD_CONFIG_OPTIONS)
