Remove outdated trimming logic that was harming efficiency.
This arose after observing excessive calls to BuildOptions.trim in https://github.com/bazelbuild/bazel/commit/ab9f2dbc078fc59b1784c2f03da027a75d72b688. This is because ConfigurationResolver.applyTransition was called with "trim fragments mode" on from a *lot* of callers even though this is an outdated interpretation of "trimming" that yields no benefits.
This was originally added in https://github.com/bazelbuild/bazel/commit/b0de919d8657d5809d9ab8315d4665926087d0b4 when the distinction between dynamic configurations and trimmed configurations wasn't yet explicit.
PiperOrigin-RevId: 245235812
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index b7bfe66..04a584d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -19,7 +19,6 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.base.VerifyException;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
@@ -264,7 +263,6 @@
transition,
depFragments,
ruleClassProvider,
- !sameFragments,
defaultBuildSettingValues,
buildSettingPackages);
StarlarkTransition.replayEvents(env.getListener(), transition);
@@ -530,9 +528,7 @@
/**
* Applies a configuration transition over a set of build options.
*
- * @return the build options for the transitioned configuration. If trimResults is true, only
- * options needed by the required fragments are included. Else the same options as the
- * original input are included (with different possible values, of course).
+ * @return the build options for the transitioned configuration.
*/
@VisibleForTesting
public static List<BuildOptions> applyTransition(
@@ -540,7 +536,6 @@
ConfigurationTransition transition,
Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments,
RuleClassProvider ruleClassProvider,
- boolean trimResults,
ImmutableMap<Label, Object> buildSettingDefaults,
Map<SkyKey, SkyValue> buildSettingPackages)
throws TransitionException {
@@ -548,14 +543,6 @@
addDefaultStarlarkOptions(fromOptions, buildSettingDefaults);
// TODO(bazel-team): safety-check that this never mutates fromOptions.
List<BuildOptions> result = transition.apply(fromOptionsWithDefaults);
- if (trimResults) {
- ImmutableList.Builder<BuildOptions> trimmedOptions = ImmutableList.builder();
- for (BuildOptions toOptions : result) {
- trimmedOptions.add(toOptions.trim(
- BuildConfiguration.getOptionsClasses(requiredFragments, ruleClassProvider)));
- }
- result = trimmedOptions.build();
- }
// Post-process transitions on starlark build settings
return StarlarkTransition.validate(transition, buildSettingPackages, result);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
index 06fed8a..2c2b89c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
@@ -357,7 +357,6 @@
transition,
depFragments,
ruleClassProvider,
- true,
defaultBuildSettingValues,
buildSettingOutputPackages);
StarlarkTransition.replayEvents(env.getListener(), transition);
@@ -404,7 +403,6 @@
transition,
depFragments,
ruleClassProvider,
- true,
defaultBuildSettingValues,
buildSettingOutputPackages);
for (BuildOptions toOption : toOptions) {
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 fe10629..c537f10 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
@@ -2010,7 +2010,6 @@
transition,
depFragments,
ruleClassProvider,
- true,
defaultInputValues,
collectBuildSettingValues(transition, eventHandler, Settings.OUTPUTS));
StarlarkTransition.replayEvents(eventHandler, transition);
@@ -2048,7 +2047,6 @@
key.getTransition(),
depFragments,
ruleClassProvider,
- true,
defaultInputValues,
collectBuildSettingValues(key.getTransition(), eventHandler, Settings.OUTPUTS));
} catch (TransitionException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionTest.java
index ab7de92..3c86097 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionTest.java
@@ -649,73 +649,6 @@
}
@Test
- public void flagOnDynamicConfigsTrimHostDeps_AreNotAnalyzedAnyExtraTimes() throws Exception {
- scratch.file(
- "test/BUILD",
- "load(':test.bzl', 'skylark_test')",
- "load(':lib.bzl', 'skylark_lib')",
- "native_test(",
- " name = 'native_outer_test',",
- " deps = [':native_test', ':skylark_test'],",
- " host_deps = [':native_test', ':skylark_test'],",
- ")",
- "skylark_test(",
- " name = 'skylark_outer_test',",
- " deps = [':native_test', ':skylark_test'],",
- " host_deps = [':native_test', ':skylark_test'],",
- ")",
- "native_test(",
- " name = 'native_test',",
- " deps = [':native_dep', ':skylark_dep'],",
- " host_deps = [':native_dep', ':skylark_dep'],",
- ")",
- "skylark_test(",
- " name = 'skylark_test',",
- " deps = [':native_dep', ':skylark_dep'],",
- " host_deps = [':native_dep', ':skylark_dep'],",
- ")",
- "native_lib(",
- " name = 'native_dep',",
- " deps = [':native_shared_dep', 'skylark_shared_dep'],",
- " host_deps = [':native_shared_dep', 'skylark_shared_dep'],",
- ")",
- "skylark_lib(",
- " name = 'skylark_dep',",
- " deps = [':native_shared_dep', 'skylark_shared_dep'],",
- " host_deps = [':native_shared_dep', 'skylark_shared_dep'],",
- ")",
- "native_lib(",
- " name = 'native_shared_dep',",
- ")",
- "skylark_lib(",
- " name = 'skylark_shared_dep',",
- ")");
- useConfiguration("--trim_test_configuration", "--experimental_dynamic_configs=on");
- update(
- "//test:native_outer_test",
- "//test:skylark_outer_test",
- "//test:native_test",
- "//test:skylark_test",
- "//test:native_dep",
- "//test:skylark_dep",
- "//test:native_shared_dep",
- "//test:skylark_shared_dep");
- LinkedHashSet<SkyKey> visitedTargets = new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
- assertNumberOfConfigurationsOfTargets(
- visitedTargets,
- new ImmutableMap.Builder<String, Integer>()
- // each target should be analyzed in two and only two configurations: target and host
- // there should not be a "host trimmed" and "host untrimmed" version
- .put("//test:native_test", 2)
- .put("//test:skylark_test", 2)
- .put("//test:native_dep", 2)
- .put("//test:skylark_dep", 2)
- .put("//test:native_shared_dep", 2)
- .put("//test:skylark_shared_dep", 2)
- .build());
- }
-
- @Test
public void flagOffConfigSetting_CanInspectTestOptions() throws Exception {
scratch.file(
"test/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java
index 3db94c7..ee0228c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsWithTrimmedConfigurationsTest.java
@@ -589,7 +589,6 @@
transition,
ruleClassProvider.getAllFragments(),
ruleClassProvider,
- false,
ImmutableMap.of(),
ImmutableMap.of())) {
outValues.add(toOptions.get(TestConfiguration.TestOptions.class).testFilter);
