Intern the BuildConfigurationValue.Key instances that we create. Currently we don't create too many of them, but it'll be nice to have this interning for future work in which there are many more of them.

When comparing BuildConfiguration instances, compare their full BuildOptions fields, which can compare quickly due to fingerprint caching, as opposed to their raw options, which is slow.

Also intern the map of Fragments that we create as part of a BuildConfiguration. There aren't too many of them either, but it means that equal Fragment sets can be compared using reference equality downstream.

PiperOrigin-RevId: 180289334

2 files changed, 45 insertions, 26 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index f41ff41836..6cc9d11f78 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Interners;
 import com.google.common.collect.Iterables;
@@ -46,6 +47,7 @@ import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
@@ -109,6 +111,16 @@ import javax.annotation.Nullable;
         + "depend on it and not targets that it depends on.")
 public class BuildConfiguration implements BuildEvent {
   /**
+   * Sorts fragments by class name. This produces a stable order which, e.g., facilitates consistent
+   * output from buildMnemonic.
+   */
+  public static final Comparator<Class<? extends Fragment>> lexicalFragmentSorter =
+      Comparator.comparing(Class::getName);
+
+  private static final Interner<ImmutableSortedMap<Class<? extends Fragment>, Fragment>>
+      fragmentsInterner = BlazeInterners.newWeakInterner();
+
+  /**
    * An interface for language-specific configurations.
    *
    * <p>All implementations must be immutable and communicate this as clearly as possible (e.g.
@@ -1084,7 +1096,8 @@ public class BuildConfiguration implements BuildEvent {
 
   private final String checksum;
 
-  private final ImmutableMap<Class<? extends Fragment>, Fragment> fragments;
+  private final ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments;
+
   private final ImmutableMap<String, Class<? extends Fragment>> skylarkVisibleFragments;
   private final RepositoryName mainRepositoryName;
   private final ImmutableSet<String> reservedActionMnemonics;
@@ -1259,7 +1272,7 @@ public class BuildConfiguration implements BuildEvent {
     BuildConfiguration otherConfig = (BuildConfiguration) other;
     return actionsEnabled == otherConfig.actionsEnabled
         && fragments.values().equals(otherConfig.fragments.values())
-        && buildOptions.getOptions().equals(otherConfig.buildOptions.getOptions());
+        && buildOptions.equals(otherConfig.buildOptions);
   }
 
   private int computeHashCode() {
@@ -1347,17 +1360,10 @@ public class BuildConfiguration implements BuildEvent {
     return ActionEnvironment.split(testEnv);
   }
 
-  /**
-   * Sorts fragments by class name. This produces a stable order which, e.g., facilitates
-   * consistent output from buildMneumonic.
-   */
-  private static final Comparator lexicalFragmentSorter =
-      new Comparator<Class<? extends Fragment>>() {
-        @Override
-        public int compare(Class<? extends Fragment> o1, Class<? extends Fragment> o2) {
-          return o1.getName().compareTo(o2.getName());
-        }
-      };
+  private static ImmutableSortedMap<Class<? extends Fragment>, Fragment> makeFragmentsMap(
+      Map<Class<? extends Fragment>, Fragment> fragmentsMap) {
+    return fragmentsInterner.intern(ImmutableSortedMap.copyOf(fragmentsMap, lexicalFragmentSorter));
+  }
 
   /**
    * Constructs a new BuildConfiguration instance.
@@ -1367,7 +1373,7 @@ public class BuildConfiguration implements BuildEvent {
       BuildOptions buildOptions,
       String repositoryName) {
     this.directories = directories;
-    this.fragments = ImmutableSortedMap.copyOf(fragmentsMap, lexicalFragmentSorter);
+    this.fragments = makeFragmentsMap(fragmentsMap);
 
     this.skylarkVisibleFragments = buildIndexOfSkylarkVisibleFragments();
 
@@ -1836,10 +1842,8 @@ public class BuildConfiguration implements BuildEvent {
     return true;
   }
 
-  /**
-   * Which fragments does this configuration contain?
-   */
-  public Set<Class<? extends Fragment>> fragmentClasses() {
+  /** Which fragments does this configuration contain? */
+  public ImmutableSortedSet<Class<? extends Fragment>> fragmentClasses() {
     return fragments.keySet();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java
index f7057d8e25..8389422fa6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationValue.java
@@ -14,8 +14,12 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Interner;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -31,6 +35,7 @@ import java.util.Set;
 // @Immutable
 @ThreadSafe
 public class BuildConfigurationValue implements SkyValue {
+  private static final Interner<Key> keyInterner = BlazeInterners.newWeakInterner();
 
   private final BuildConfiguration configuration;
 
@@ -51,16 +56,20 @@ public class BuildConfigurationValue implements SkyValue {
   @ThreadSafe
   public static SkyKey key(Set<Class<? extends BuildConfiguration.Fragment>> fragments,
       BuildOptions buildOptions) {
-    return new Key(fragments, buildOptions);
+    return keyInterner.intern(
+        new Key(
+            ImmutableSortedSet.copyOf(BuildConfiguration.lexicalFragmentSorter, fragments),
+            buildOptions));
   }
 
   static final class Key implements SkyKey, Serializable {
-    private final Set<Class<? extends BuildConfiguration.Fragment>> fragments;
+    private final ImmutableSortedSet<Class<? extends BuildConfiguration.Fragment>> fragments;
     private final BuildOptions buildOptions;
     private final boolean enableActions;
+    // If hashCode really is -1, we'll recompute it from scratch each time. Oh well.
+    private volatile int hashCode = -1;
 
-    Key(Set<Class<? extends BuildConfiguration.Fragment>> fragments,
-        BuildOptions buildOptions) {
+    Key(ImmutableSortedSet<Class<? extends Fragment>> fragments, BuildOptions buildOptions) {
       this.fragments = fragments;
       this.buildOptions = Preconditions.checkNotNull(buildOptions);
       // Cache this value for quicker access on .equals() / .hashCode(). We don't cache it inside
@@ -69,7 +78,7 @@ public class BuildConfigurationValue implements SkyValue {
       this.enableActions = buildOptions.enableActions();
     }
 
-    Set<Class<? extends BuildConfiguration.Fragment>> getFragments() {
+    ImmutableSortedSet<Class<? extends BuildConfiguration.Fragment>> getFragments() {
       return fragments;
     }
 
@@ -84,18 +93,24 @@ public class BuildConfigurationValue implements SkyValue {
 
     @Override
     public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
       if (!(o instanceof Key)) {
         return false;
       }
       Key otherConfig = (Key) o;
-      return Objects.equals(fragments, otherConfig.fragments)
-          && Objects.equals(buildOptions, otherConfig.buildOptions)
+      return buildOptions.equals(otherConfig.buildOptions)
+          && Objects.equals(fragments, otherConfig.fragments)
           && enableActions == otherConfig.enableActions;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(fragments, buildOptions, enableActions);
+      if (hashCode == -1) {
+        hashCode = Objects.hash(fragments, buildOptions, enableActions);
+      }
+      return hashCode;
     }
   }
 }