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

1 files changed, 2 insertions, 9 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.