From cf701ebeab2565e910453cad8b1b950553596d83 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 17 Jan 2025 13:42:39 -0800 Subject: [PATCH] Fix performance regression on builds that change a test configuration flag. Instead of `BuildOptionsScopeFunction` requesting `PrecomputedValue.BASELINE_CONFIGURATION`, request it in `BuildConfigurationKeyProducer` only after we know that we truly need it. This way the dependency is only requested if scopes are being applied. The performance concern still exists if starlark flag scoping is enabled. PiperOrigin-RevId: 716785874 Change-Id: Iabd7c11440c70c909f111f1dcafafa3b98610be9 --- .../build/lib/analysis/producers/BUILD | 1 + .../BuildConfigurationKeyProducer.java | 68 +++++++++---------- .../skyframe/BuildOptionsScopeFunction.java | 3 - .../lib/skyframe/BuildOptionsScopeValue.java | 12 +--- .../BuildOptionsScopeFunctionTest.java | 2 - 5 files changed, 34 insertions(+), 52 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD index 3360fa203caafa..375c3943365fba 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD @@ -115,6 +115,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe:build_options_scope_function", "//src/main/java/com/google/devtools/build/lib/skyframe:build_options_scope_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe/config", "//src/main/java/com/google/devtools/build/lib/skyframe/config:exceptions", "//src/main/java/com/google/devtools/build/lib/skyframe/toolchains:platform_lookup_util", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java index 0f1deb65696386..b21b8ccc07ccce 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.BuildOptionsScopeFunction.BuildOptionsScopeFunctionException; import com.google.devtools.build.lib.skyframe.BuildOptionsScopeValue; +import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey; import com.google.devtools.build.lib.skyframe.config.ParsedFlagsValue; import com.google.devtools.build.lib.skyframe.config.PlatformMappingException; @@ -105,6 +106,7 @@ public interface ResultSink { private PlatformMappingValue platformMappingValue; private BuildOptionsScopeValue buildOptionsScopeValue; private BuildOptions postPlatformProcessedOptions; + private BuildOptions baselineConfiguration; BuildConfigurationKeyProducer( ResultSink sink, StateMachine runAfter, C context, BuildOptions options, Label label) { @@ -164,7 +166,7 @@ private StateMachine findBuildOptionsScopes(Tasks tasks) { Preconditions.checkNotNull(this.postPlatformProcessedOptions); // including platform-based flags in skykey for scopes lookUp if (postPlatformProcessedOptions.getStarlarkOptions().isEmpty()) { - return this::finishConfigurationKeyProcessing; + return this::possiblyApplyScopes; } // the list of flags that are either project scoped or their scopes are not yet resolved. @@ -184,14 +186,14 @@ private StateMachine findBuildOptionsScopes(Tasks tasks) { // if flagsWithIncompleteScopeInfo is empty, we do not need to do any further lookUp for the // ScopeType and ScopeDefinition if (flagsWithIncompleteScopeInfo.isEmpty()) { - return this::finishConfigurationKeyProcessing; + return this::possiblyApplyScopes; } BuildOptionsScopeValue.Key buildOptionsScopeValueKey = BuildOptionsScopeValue.Key.create( this.postPlatformProcessedOptions, flagsWithIncompleteScopeInfo); tasks.lookUp(buildOptionsScopeValueKey, (Consumer) this); - return this::finishConfigurationKeyProcessing; + return this::possiblyApplyScopes; } /** @@ -258,28 +260,12 @@ public void accept(SkyValue value) { this.buildOptionsScopeValue = (BuildOptionsScopeValue) value; } - private StateMachine finishConfigurationKeyProcessing(Tasks tasks) { - if (this.postPlatformProcessedOptions.getStarlarkOptions().isEmpty()) { - sink.acceptTransitionedConfiguration( - this.context, BuildConfigurationKey.create(this.postPlatformProcessedOptions)); - return this.runAfter; - } - - BuildConfigurationKey finalBuildConfigurationKey = - possiblyApplyScopes( - this.buildOptionsScopeValue, this.label, this.postPlatformProcessedOptions); - sink.acceptTransitionedConfiguration(this.context, finalBuildConfigurationKey); - return this.runAfter; - } - - private BuildConfigurationKey possiblyApplyScopes( - @Nullable BuildOptionsScopeValue buildOptionsScopeValue, - Label label, - BuildOptions postPlatformBasedFlagsOptions) { + private StateMachine possiblyApplyScopes(Tasks tasks) { // This is not the same as null associated with Skyframe lookUp. This happens when scoping logic // is not enabled. This means the lookup via BuildOptionsScopesFunction was not performed. - if (buildOptionsScopeValue == null) { - return BuildConfigurationKey.create(postPlatformBasedFlagsOptions); + if (buildOptionsScopeValue == null + || postPlatformProcessedOptions.getStarlarkOptions().isEmpty()) { + return finishConfigurationKeyProcessing(postPlatformProcessedOptions); } boolean shouldApplyScopes = @@ -287,20 +273,29 @@ private BuildConfigurationKey possiblyApplyScopes( .anyMatch(scope -> scope.getScopeType() == Scope.ScopeType.PROJECT); if (!shouldApplyScopes) { - return BuildConfigurationKey.create( - this.buildOptionsScopeValue.getResolvedBuildOptionsWithScopeTypes()); + return finishConfigurationKeyProcessing( + buildOptionsScopeValue.getResolvedBuildOptionsWithScopeTypes()); } - if (!buildOptionsScopeValue - .getBaselineConfiguration() - .getStarlarkOptions() - .equals( - buildOptionsScopeValue.getResolvedBuildOptionsWithScopeTypes().getStarlarkOptions())) { - return BuildConfigurationKey.create(resetFlags(buildOptionsScopeValue, label)); - } + // TODO: b/390669368 - The same performance issue still exists if we reach this point. + tasks.lookUp( + PrecomputedValue.BASELINE_CONFIGURATION.getKey(), + val -> this.baselineConfiguration = (BuildOptions) ((PrecomputedValue) val).get()); + return this::applyScopes; + } + + private StateMachine applyScopes(Tasks tasks) { + BuildOptions resolved = buildOptionsScopeValue.getResolvedBuildOptionsWithScopeTypes(); + BuildOptions finalBuildOptions = + baselineConfiguration.getStarlarkOptions().equals(resolved.getStarlarkOptions()) + ? resolved + : resetFlags(buildOptionsScopeValue, baselineConfiguration, label); + return finishConfigurationKeyProcessing(finalBuildOptions); + } - return BuildConfigurationKey.create( - buildOptionsScopeValue.getResolvedBuildOptionsWithScopeTypes()); + private StateMachine finishConfigurationKeyProcessing(BuildOptions finalBuildOptions) { + sink.acceptTransitionedConfiguration(context, BuildConfigurationKey.create(finalBuildOptions)); + return runAfter; } /** @@ -318,7 +313,9 @@ private BuildConfigurationKey possiblyApplyScopes( * has the {@link Scope.ScopeType} information for all starlark flags. */ private static BuildOptions resetFlags( - BuildOptionsScopeValue buildOptionsScopeValue, Label label) { + BuildOptionsScopeValue buildOptionsScopeValue, + BuildOptions baselineConfiguration, + Label label) { Preconditions.checkNotNull(buildOptionsScopeValue); Preconditions.checkNotNull(label); @@ -329,7 +326,6 @@ private static BuildOptions resetFlags( return transitionedOptionsWithScopeType; } - BuildOptions baselineConfiguration = buildOptionsScopeValue.getBaselineConfiguration(); Preconditions.checkNotNull(baselineConfiguration); boolean flagsRemoved = false; boolean flagsResetToBaseline = false; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java index 1f1b7ef1809956..d1a1ecdbb0864d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java @@ -120,11 +120,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) : new Scope.ScopeDefinition(projectValue.getDefaultActiveDirectory()))); } - BuildOptions baseline = PrecomputedValue.BASELINE_CONFIGURATION.get(env); - return BuildOptionsScopeValue.create( fullyResolvedBuildOptionsBuilder.build(), - baseline, Lists.newArrayList(projectValueSkyKeysMap.keySet()), scopes); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeValue.java index adc613fbc0352d..bc4e44dffc5f76 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeValue.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyKey.SkyKeyInterner; import com.google.devtools.build.skyframe.SkyValue; import java.util.LinkedHashMap; import java.util.List; @@ -30,7 +29,6 @@ public final class BuildOptionsScopeValue implements SkyValue { BuildOptions resolvedBuildOptionsWithScopeTypes; - BuildOptions baselineConfiguration; List