Fix performance regression with manual trimming.
When --enforce_transitive_configs_for_config_feature_flag=true
(which means manual trimming is enabled), the manual trimming
transition applies to every configured target in the build, even
though it's a no-op for most of them. But it unconditionally creates
new BuildOptions instances, each of which has its hash key computed
later in the build. This is expensive enough to severely harm
analysis time.
With this change, the transition returns the original instance when no
trimming is necessary.
Example results on a trivial android_binary depending on an android_library
with trimming:
Before:
############################################
# Original Blaze, manual trimming disabled:
b/blazedev_orig build --nobuild //testapp:binary --enforce_transitive_configs_for_config_feature_flag=false | sort | uniq -c
137 CALLING BuildOptions.maybeInitializeFingerprintAndHashCode
# Original Blaze, manual trimming enabled:
b/blazedev_orig build --nobuild //testapp:binary --enforce_transitive_configs_for_config_feature_flag=true | sort | uniq -c
809 CALLING BuildOptions.maybeInitializeFingerprintAndHashCode
573 TRIMMING TRANSITION RETURNED NEW BUILDOPTIONS INSTANCE
############################################
After:
############################################
# Updated Blaze, manual trimming disabled:
b/blazedev_new build --nobuild //testapp:binary --enforce_transitive_configs_for_config_feature_flag=false | sort | uniq -c
138 CALLING BuildOptions.maybeInitializeFingerprintAndHashCode
# Updated Blaze, manual trimming enabled:
b/blazedev_new build --nobuild //testapp:binary --enforce_transitive_configs_for_config_feature_flag=true | sort | uniq -c
475 CALLING BuildOptions.maybeInitializeFingerprintAndHashCode
209 TRIMMING TRANSITION RETURNED NEW BUILDOPTIONS INSTANCE
############################################
We still create more instances when manual trimming is on, but this is expected. Note that rules which don't use trimming (don't have the transitive_configs attribute) don't create *any* more instances.
PiperOrigin-RevId: 251520074
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/FeatureFlagValue.java b/src/main/java/com/google/devtools/build/lib/rules/config/FeatureFlagValue.java
index f9a759a..ff5bcd2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/FeatureFlagValue.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/FeatureFlagValue.java
@@ -17,12 +17,13 @@
import com.google.auto.value.AutoValue;
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.Sets;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.cmdline.Label;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
@@ -102,25 +103,42 @@
/** Returns a new BuildOptions with the feature flag values trimmed down to the given flags. */
static BuildOptions trimFlagValues(BuildOptions original, Set<Label> availableFlags) {
- BuildOptions.Builder result = original.toBuilder();
- ImmutableSet.Builder<Label> seenFlagsBuilder = new ImmutableSet.Builder<>();
- for (Map.Entry<Label, Object> entry : original.getStarlarkOptions().entrySet()) {
- if (entry.getValue() instanceof FeatureFlagValue) {
- seenFlagsBuilder.add(entry.getKey());
- }
- }
- ImmutableSet<Label> seenFlags = seenFlagsBuilder.build();
- for (Label trimmedFlag : Sets.difference(seenFlags, availableFlags)) {
- result.removeStarlarkOption(trimmedFlag);
- }
+ // An important performance property of this method is that we don't create a new BuildOptions
+ // instance unless we really need one. This particularly saves the expensive cost of
+ // BuildOptions.hashCode(). Since this method is called unconditionally over every configured
+ // target, this has real observable effect on build analysis time.
+ Set<Label> seenFlags = new LinkedHashSet<>();
+ Set<Label> flagsToTrim = new LinkedHashSet<>();
+ Map<Label, Object> unknownFlagsToAdd = new LinkedHashMap<>();
+ boolean changeAllValuesPresentOption = false;
+
+ // What do we need to change?
+ original.getStarlarkOptions().entrySet().stream()
+ .filter(entry -> entry.getValue() instanceof FeatureFlagValue)
+ .forEach(featureFlagEntry -> seenFlags.add(featureFlagEntry.getKey()));
+ flagsToTrim.addAll(Sets.difference(seenFlags, availableFlags));
FeatureFlagValue unknownFlagValue =
(original.contains(ConfigFeatureFlagOptions.class)
&& original.get(ConfigFeatureFlagOptions.class).allFeatureFlagValuesArePresent)
? DefaultValue.INSTANCE
: UnknownValue.INSTANCE;
for (Label unknownFlag : Sets.difference(availableFlags, seenFlags)) {
- result.addStarlarkOption(unknownFlag, unknownFlagValue);
+ unknownFlagsToAdd.put(unknownFlag, unknownFlagValue);
}
+ if (original.contains(ConfigFeatureFlagOptions.class)) {
+ changeAllValuesPresentOption =
+ original.get(ConfigFeatureFlagOptions.class).allFeatureFlagValuesArePresent;
+ }
+
+ // Nothing changed? Return the original BuildOptions.
+ if (flagsToTrim.isEmpty() && unknownFlagsToAdd.isEmpty() && !changeAllValuesPresentOption) {
+ return original;
+ }
+
+ // Else construct a new one. This should not be the common case.
+ BuildOptions.Builder result = original.toBuilder();
+ flagsToTrim.forEach(trimmedFlag -> result.removeStarlarkOption(trimmedFlag));
+ unknownFlagsToAdd.forEach((flag, value) -> result.addStarlarkOption(flag, value));
BuildOptions builtResult = result.build();
if (builtResult.contains(ConfigFeatureFlagOptions.class)) {
builtResult.get(ConfigFeatureFlagOptions.class).allFeatureFlagValuesArePresent = false;
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/FeatureFlagManualTrimmingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/FeatureFlagManualTrimmingTest.java
index 0f90113..6865436 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/FeatureFlagManualTrimmingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/FeatureFlagManualTrimmingTest.java
@@ -24,13 +24,16 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
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.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import java.util.Map;
@@ -849,4 +852,40 @@
Label usedFlag = Label.parseAbsolute("//test:used_flag", ImmutableMap.of());
assertThat(getFlagValuesFromOutputFile(targetFlags)).containsEntry(usedFlag, "configured");
}
+
+ @Test
+ public void trimmingTransitionReturnsOriginalOptionsWhenNothingIsTrimmed() throws Exception {
+ // This is a performance regression test. The trimming transition applies over every configured
+ // target in a build. Since BuildOptions.hashCode is expensive, if that produced a unique
+ // BuildOptions instance for every configured target
+ scratch.file(
+ "test/BUILD",
+ "load(':read_flags.bzl', 'read_flags')",
+ "feature_flag_setter(",
+ " name = 'toplevel_target',",
+ " deps = [':dep'],",
+ " flag_values = {",
+ " ':used_flag': 'configured',",
+ " },",
+ " transitive_configs = [':used_flag'],",
+ ")",
+ "read_flags(",
+ " name = 'dep',",
+ " flags = [':used_flag'],",
+ " transitive_configs = [':used_flag'],",
+ ")",
+ "config_feature_flag(",
+ " name = 'used_flag',",
+ " allowed_values = ['default', 'configured', 'other'],",
+ " default_value = 'default',",
+ ")");
+
+ BuildOptions topLevelOptions =
+ getConfiguration(getConfiguredTarget("//test:toplevel_target")).getOptions();
+ BuildOptions depOptions =
+ new ConfigFeatureFlagTaggedTrimmingTransitionFactory(BaseRuleClasses.TAGGED_TRIMMING_ATTR)
+ .create((Rule) getTarget("//test:dep"))
+ .patch(topLevelOptions);
+ assertThat(depOptions).isSameInstanceAs(topLevelOptions);
+ }
}
