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, 30 insertions, 0 deletions

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));
+  }
 }