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

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",

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<C> {
   private PlatformMappingValue platformMappingValue;
   private BuildOptionsScopeValue buildOptionsScopeValue;
   private BuildOptions postPlatformProcessedOptions;
+  private BuildOptions baselineConfiguration;
 
   BuildConfigurationKeyProducer(
       ResultSink<C> 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<SkyValue>) this);
-    return this::finishConfigurationKeyProcessing;
+    return this::possiblyApplyScopes;
   }
 
   /**
@@ -258,49 +260,42 @@ 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 =
         buildOptionsScopeValue.getFullyResolvedScopes().values().stream()
             .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;

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

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<Label> scopedFlags;
   LinkedHashMap<Label, Scope> fullyResolvedScopes;
 
@@ -95,20 +93,16 @@ public int hashCode() {
   public static BuildOptionsScopeValue create(
       BuildOptions inputBuildOptions,
       // BuildOptions buildOptionsWithScopes,
-      BuildOptions baselineConfiguration,
       List<Label> scopedFlags,
       LinkedHashMap<Label, Scope> fullyResolvedScopes) {
-    return new BuildOptionsScopeValue(
-        inputBuildOptions, baselineConfiguration, scopedFlags, fullyResolvedScopes);
+    return new BuildOptionsScopeValue(inputBuildOptions, scopedFlags, fullyResolvedScopes);
   }
 
   public BuildOptionsScopeValue(
       BuildOptions resolvedBuildOptionsWithScopeTypes,
-      BuildOptions baselineConfiguration,
       List<Label> scopedFlags,
       LinkedHashMap<Label, Scope> fullyResolvedScopes) {
     this.resolvedBuildOptionsWithScopeTypes = resolvedBuildOptionsWithScopeTypes;
-    this.baselineConfiguration = baselineConfiguration;
     this.scopedFlags = scopedFlags;
     this.fullyResolvedScopes = fullyResolvedScopes;
   }
@@ -128,8 +122,4 @@ public BuildOptions getResolvedBuildOptionsWithScopeTypes() {
   public LinkedHashMap<Label, Scope> getFullyResolvedScopes() {
     return fullyResolvedScopes;
   }
-
-  public BuildOptions getBaselineConfiguration() {
-    return baselineConfiguration;
-  }
 }

src/test/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunctionTest.java

@@ -119,7 +119,6 @@ public void buildOptionsScopesFunction_returnsCorrectScope() throws Exception {
     BuildOptionsScopeValue buildOptionsScopeValue = executeFunction(key);
 
     // verify that the Scope is fully resolved for //test_flags:foo and //test_flags:bar
-    assertThat(buildOptionsScopeValue.getBaselineConfiguration()).isNotNull();
     var unused =
         assertThat(
             buildOptionsScopeValue
@@ -175,7 +174,6 @@ public void buildOptionsScopesFunction_doesNotErrorOut_whenNoProjectFile() throw
         BuildOptionsScopeValue.Key.create(buildOptionsWithoutScopes, scopedFlags);
 
     BuildOptionsScopeValue buildOptionsScopeValue = executeFunction(key);
-    assertThat(buildOptionsScopeValue.getBaselineConfiguration()).isNotNull();
     var unused =
         assertThat(
             buildOptionsScopeValue