Trim the BuildOptions input of ConfigurationFragment.key to only those

options actually needed by the fragment. This protects against, e.g.,
unnecessarily duplicating CppConfiguration instances when only Java flags
change.

This is a recommit of ca1b21ac6d8a58041db822725b42de151b163dee which was
rolled back because it broke LIPO.

This change is particularly important for dynamic configurations, which may
mix and match fragments arbitrarily throughout a build. This not only has
performance implications, but also correctness implications: code that
expects two configured targets to have the same fragment (value) shouldn't
break just because the second CT's configuration is a trimmed version of the
first's.

The original change breaks FDO/LIPO because CppConfiguration can't be
shared across configurations. That's because it mutates state when
prepareHook() is called, and each configuration calls prepareHook. We
should ultimately solve this by refactoring the FDO/LIPO implementation
but don't want to block dynamic configuration progress on that. So this
change only enables trimming for dynamic configurations.

--
MOS_MIGRATED_REVID=113570250

7 files changed, 82 insertions, 29 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
index 15498ce490..a25c3ac201 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
@@ -16,14 +16,12 @@ package com.google.devtools.build.lib.skyframe;
 import static com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 
 import com.google.common.collect.ClassToInstanceMap;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.MutableClassToInstanceMap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfigurationCollectionFactory;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -90,13 +88,8 @@ public class BuildConfigurationFunction implements SkyFunction {
     // Get SkyKeys for the fragments we need to load.
     Set<SkyKey> fragmentKeys = new LinkedHashSet<>();
     for (Class<? extends BuildConfiguration.Fragment> fragmentClass : key.getFragments()) {
-      // We don't want to invalidate the fragment Skyframe key due to existence of absence of
-      // options the fragment doesn't use.
-      BuildOptions optionsUsedByFragment = key.getBuildOptions().trim(
-          BuildConfiguration.getOptionsClasses(
-              ImmutableList.<Class<? extends BuildConfiguration.Fragment>>of(fragmentClass),
-              ruleClassProvider));
-      fragmentKeys.add(ConfigurationFragmentValue.key(optionsUsedByFragment, fragmentClass));
+      fragmentKeys.add(
+          ConfigurationFragmentValue.key(key.getBuildOptions(), fragmentClass, ruleClassProvider));
     }
 
     // Load them as Skyframe deps.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
index aea49f20c7..a5061b25f9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigura
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.skyframe.ConfigurationCollectionValue.ConfigurationCollectionKey;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -44,9 +45,12 @@ import javax.annotation.Nullable;
 public class ConfigurationCollectionFunction implements SkyFunction {
 
   private final Supplier<ConfigurationFactory> configurationFactory;
+  private final RuleClassProvider ruleClassProvider;
 
-  public ConfigurationCollectionFunction(Supplier<ConfigurationFactory> configurationFactory) {
+  public ConfigurationCollectionFunction(Supplier<ConfigurationFactory> configurationFactory,
+      RuleClassProvider ruleClassProvider) {
     this.configurationFactory = configurationFactory;
+    this.ruleClassProvider = ruleClassProvider;
   }
 
   @Override
@@ -54,9 +58,9 @@ public class ConfigurationCollectionFunction implements SkyFunction {
       ConfigurationCollectionFunctionException {
     ConfigurationCollectionKey collectionKey = (ConfigurationCollectionKey) skyKey.argument();
     try {
-      BuildConfigurationCollection result =
-          getConfigurations(env, new SkyframePackageLoaderWithValueEnvironment(env),
-              collectionKey.getBuildOptions(), collectionKey.getMultiCpu());
+      BuildConfigurationCollection result = getConfigurations(env,
+          new SkyframePackageLoaderWithValueEnvironment(env, ruleClassProvider),
+          collectionKey.getBuildOptions(), collectionKey.getMultiCpu());
 
       // BuildConfigurationCollection can be created, but dependencies to some files might be
       // missing. In that case we need to build configurationCollection again.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
index 8da3a9cc3a..ac541411cd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.ConfigurationFragmentValue.ConfigurationFragmentKey;
 import com.google.devtools.build.lib.vfs.Path;
@@ -43,10 +44,13 @@ import java.io.IOException;
 public class ConfigurationFragmentFunction implements SkyFunction {
 
   private final Supplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments;
+  private final RuleClassProvider ruleClassProvider;
 
   public ConfigurationFragmentFunction(
-      Supplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments) {
+      Supplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments,
+      RuleClassProvider ruleClassProvider) {
     this.configurationFragments = configurationFragments;
+    this.ruleClassProvider = ruleClassProvider;
   }
 
   @Override
@@ -58,7 +62,7 @@ public class ConfigurationFragmentFunction implements SkyFunction {
     ConfigurationFragmentFactory factory = getFactory(configurationFragmentKey.getFragmentType());
     try {
       PackageProviderForConfigurations packageProvider = 
-          new SkyframePackageLoaderWithValueEnvironment(env);
+          new SkyframePackageLoaderWithValueEnvironment(env, ruleClassProvider);
       ConfigurationEnvironment confEnv = new ConfigurationBuilderEnvironment(packageProvider);
       Fragment fragment = factory.create(confEnv, buildOptions);
       
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentValue.java
index bebc76a100..1f72d00501 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentValue.java
@@ -14,11 +14,13 @@
 package com.google.devtools.build.lib.skyframe;
 
 
+import com.google.common.collect.ImmutableList;
 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.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -34,7 +36,7 @@ import javax.annotation.Nullable;
 @Immutable
 @ThreadSafe
 public class ConfigurationFragmentValue implements SkyValue {
-  
+
   @Nullable
   private final BuildConfiguration.Fragment fragment;
 
@@ -45,31 +47,47 @@ public class ConfigurationFragmentValue implements SkyValue {
   public BuildConfiguration.Fragment getFragment() {
     return fragment;
   }
-  
+
   @ThreadSafe
-  public static SkyKey key(BuildOptions buildOptions, Class<? extends Fragment> fragmentType) {
+  public static SkyKey key(BuildOptions buildOptions, Class<? extends Fragment> fragmentType,
+      RuleClassProvider ruleClassProvider) {
+    // For dynamic configurations, trim the options down to just those used by this fragment.
+    // This ensures we don't end up with different Skyframe keys due to trimming of unrelated
+    // options.
+    //
+    // We don't trim for static configurations because static configurations need to support
+    // LIPO and trimming breaks LIPO: it results in CppConfiguration instances being shared
+    // across configurations. That isn't safe because CppConfiguration mutates its state
+    // in prepareHook() for each configuration.
+    BuildOptions optionsKey =
+        buildOptions.get(BuildConfiguration.Options.class).useDynamicConfigurations
+        ? buildOptions.trim(
+              BuildConfiguration.getOptionsClasses(
+                  ImmutableList.<Class<? extends BuildConfiguration.Fragment>>of(fragmentType),
+                  ruleClassProvider))
+        : buildOptions;
     return new SkyKey(SkyFunctions.CONFIGURATION_FRAGMENT,
-        new ConfigurationFragmentKey(buildOptions, fragmentType));
+        new ConfigurationFragmentKey(optionsKey, fragmentType));
   }
-  
+
   static final class ConfigurationFragmentKey implements Serializable {
     private final BuildOptions buildOptions;
     private final Class<? extends Fragment> fragmentType;
-    
+
     public ConfigurationFragmentKey(BuildOptions buildOptions,
         Class<? extends Fragment> fragmentType) {
       this.buildOptions = Preconditions.checkNotNull(buildOptions);
-      this.fragmentType = Preconditions.checkNotNull(fragmentType);      
+      this.fragmentType = Preconditions.checkNotNull(fragmentType);
     }
-    
+
     public BuildOptions getBuildOptions() {
       return buildOptions;
     }
-    
+
     public Class<? extends Fragment> getFragmentType() {
       return fragmentType;
     }
-    
+
     @Override
     public boolean equals(Object o) {
       if (this == o) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index f60b333036..e62b452255 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -376,9 +376,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     map.put(SkyFunctions.BUILD_CONFIGURATION,
         new BuildConfigurationFunction(directories, ruleClassProvider));
     map.put(SkyFunctions.CONFIGURATION_COLLECTION, new ConfigurationCollectionFunction(
-        configurationFactory));
+        configurationFactory, ruleClassProvider));
     map.put(SkyFunctions.CONFIGURATION_FRAGMENT, new ConfigurationFragmentFunction(
-        configurationFragments));
+        configurationFragments, ruleClassProvider));
     map.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider));
     map.put(
         SkyFunctions.WORKSPACE_FILE,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
index 95bd400795..05dc2b3332 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor.SkyframePackageLoader;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -40,9 +41,12 @@ import java.io.IOException;
  */
 class SkyframePackageLoaderWithValueEnvironment implements PackageProviderForConfigurations {
   private final SkyFunction.Environment env;
+  private final RuleClassProvider ruleClassProvider;
 
-  public SkyframePackageLoaderWithValueEnvironment(SkyFunction.Environment env) {
+  public SkyframePackageLoaderWithValueEnvironment(SkyFunction.Environment env,
+      RuleClassProvider ruleClassProvider) {
     this.env = env;
+    this.ruleClassProvider = ruleClassProvider;
   }
 
   private Package getPackage(final PackageIdentifier pkgIdentifier)
@@ -76,7 +80,7 @@ class SkyframePackageLoaderWithValueEnvironment implements PackageProviderForCon
   public <T extends Fragment> T getFragment(BuildOptions buildOptions, Class<T> fragmentType)
       throws InvalidConfigurationException {
     ConfigurationFragmentValue fragmentNode = (ConfigurationFragmentValue) env.getValueOrThrow(
-        ConfigurationFragmentValue.key(buildOptions, fragmentType),
+        ConfigurationFragmentValue.key(buildOptions, fragmentType, ruleClassProvider),
         InvalidConfigurationException.class);
     if (fragmentNode == null) {
       return null;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
index 7c8c5bca5a..48bb5cfadd 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppOptions;
+import com.google.devtools.build.lib.rules.java.J2ObjcConfiguration;
 import com.google.devtools.build.lib.rules.java.JavaConfiguration;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -319,4 +320,33 @@ public class BuildConfigurationTest extends ConfigurationTestCase {
     assertFalse(config.equalsOrIsSupersetOf(hostConfig));
     assertFalse(trimmedConfig.equalsOrIsSupersetOf(config));
   }
+
+  @Test
+  public void testDynamicConfigFragmentsAreShareableAcrossConfigurations() throws Exception {
+    // Note we can't use any fragments that load files (e.g. CROSSTOOL) because those get
+    // Skyframe-invalidated between create() calls.
+    BuildConfiguration config1 = create("--experimental_dynamic_configs", "--javacopt=foo");
+    BuildConfiguration config2 = create("--experimental_dynamic_configs", "--javacopt=bar");
+    BuildConfiguration config3 =
+        create("--experimental_dynamic_configs", "--j2objc_translation_flags=baz");
+    // Shared because all j2objc options are the same:
+    assertThat(config1.getFragment(J2ObjcConfiguration.class))
+        .isSameAs(config2.getFragment(J2ObjcConfiguration.class));
+    // Distinct because the j2objc options differ:
+    assertThat(config1.getFragment(J2ObjcConfiguration.class))
+        .isNotSameAs(config3.getFragment(J2ObjcConfiguration.class));
+  }
+
+  @Test
+  public void testStaticConfigFragmentsDistinctAcrossConfigurations() throws Exception {
+    BuildConfiguration config1 = create("--javacopt=foo");
+    BuildConfiguration config2 = create("--javacopt=foo");
+    BuildConfiguration config3 = create("--javacopt=bar");
+    // Shared because global build options are identical:
+    assertThat(config1.getFragment(J2ObjcConfiguration.class))
+        .isSameAs(config2.getFragment(J2ObjcConfiguration.class));
+    // Distinct because global build options differ (even though j2objc options are the same).
+    assertThat(config1.getFragment(J2ObjcConfiguration.class))
+        .isNotSameAs(config3.getFragment(J2ObjcConfiguration.class));
+  }
 }