Add a flag to evaluate the top level transitions in Skyframe

This adds a new PrepareAnalysisPhaseFunction, which started out as a copy of
some existing code from SkyframeExecutor, BuildView, AnalysisPhaseRunner,
AnalysisUtils, and ConfigurationResolver, which was then modified to work
inside Skyframe.

Most of our tests already work with the new code, except for some of the tests
related to configuration trimming in combination with dependency cycles. The
reason for this is that we can only recover from dependency cycles at the end
of a Skyframe invocation, but never inside a Skyframe invocation. The new code
therefore cannot return partial results like the old code.

This seems to make null builds a bit faster. In my testing, I saw null build
times for a single test target go from ~50ms to ~40ms. This is probably due to
slightly better caching - it seems that computing the configuration transitions
and top-level targets is non-negligible, even if there's only a single
top-level configuration for a single top-level target.

This might be an even bigger win if there are a lot of top-level targets and
configurations.

PiperOrigin-RevId: 207083192

2 files changed, 15 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
index be441c15cc..97e2a67d51 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -22,11 +22,9 @@ import com.google.devtools.build.lib.analysis.BuildView;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.LicensesProvider;
 import com.google.devtools.build.lib.analysis.LicensesProvider.TargetLicense;
-import com.google.devtools.build.lib.analysis.MakeEnvironmentEvent;
 import com.google.devtools.build.lib.analysis.StaticallyLinkedMarkerProvider;
 import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.buildeventstream.AbortedEvent;
@@ -112,45 +110,22 @@ public final class AnalysisPhaseRunner {
     // Exit if there are any pending exceptions from modules.
     env.throwPendingException();
 
-    // Configuration creation.
-    // TODO(gregce): Consider dropping this phase and passing on-the-fly target / host configs as
-    // needed. This requires cleaning up the invalidation in SkyframeBuildView.setConfigurations.
-    BuildConfigurationCollection configurations;
-    try (SilentCloseable c = Profiler.instance().profile("createConfigurations")) {
-      configurations =
-          env.getSkyframeExecutor()
-              .createConfigurations(
-                  env.getReporter(),
-                  env.getRuntime().getConfigurationFragmentFactories(),
-                  buildOptions,
-                  request.getMultiCpus(),
-                  request.getKeepGoing());
-    }
-
-    env.throwPendingException();
-    if (configurations.getTargetConfigurations().size() == 1) {
-      // TODO(bazel-team): This is not optimal - we retain backwards compatibility in the case
-      // where there's only a single configuration, but we don't send an event in the multi-config
-      // case. Can we do better? [multi-config]
-      env.getEventBus()
-          .post(
-              new MakeEnvironmentEvent(
-                  configurations.getTargetConfigurations().get(0).getMakeEnvironment()));
-    }
-    logger.info("Configurations created");
+    env.getSkyframeExecutor().setConfigurationFragmentFactories(
+        env.getRuntime().getConfigurationFragmentFactories());
 
     AnalysisResult analysisResult = null;
     if (request.getBuildOptions().performAnalysisPhase) {
       Profiler.instance().markPhase(ProfilePhase.ANALYZE);
       try (SilentCloseable c = Profiler.instance().profile("runAnalysisPhase")) {
-        analysisResult = runAnalysisPhase(request, loadingResult, configurations);
+        analysisResult =
+            runAnalysisPhase(request, loadingResult, buildOptions, request.getMultiCpus());
       }
 
       // Check licenses.
       // We check licenses if the first target configuration has license checking enabled. Right
       // now, it is not possible to have multiple target configurations with different settings
       // for this flag, which allows us to take this short cut.
-      boolean checkLicenses = configurations.getTargetConfigurations().get(0).checkLicenses();
+      boolean checkLicenses = buildOptions.get(BuildConfiguration.Options.class).checkLicenses;
       if (checkLicenses) {
         Profiler.instance().markPhase(ProfilePhase.LICENSE);
         try (SilentCloseable c = Profiler.instance().profile("validateLicensingForTargets")) {
@@ -221,8 +196,9 @@ public final class AnalysisPhaseRunner {
   private AnalysisResult runAnalysisPhase(
       BuildRequest request,
       TargetPatternPhaseValue loadingResult,
-      BuildConfigurationCollection configurations)
-      throws InterruptedException, ViewCreationFailedException {
+      BuildOptions targetOptions,
+      Set<String> multiCpu)
+      throws InterruptedException, InvalidConfigurationException, ViewCreationFailedException {
     Stopwatch timer = Stopwatch.createStarted();
     env.getReporter().handle(Event.progress("Loading complete.  Analyzing..."));
 
@@ -235,7 +211,8 @@ public final class AnalysisPhaseRunner {
     AnalysisResult analysisResult =
         view.update(
             loadingResult,
-            configurations,
+            targetOptions,
+            multiCpu,
             request.getAspects(),
             request.getViewOptions(),
             request.getKeepGoing(),
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java
index b2018a23fa..101529d950 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutorWrappingWalkableGraph;
 import com.google.devtools.build.skyframe.WalkableGraph;
 import java.io.IOException;
+import java.util.Collection;
 import java.util.List;
 import java.util.stream.Collectors;
 
@@ -83,8 +84,9 @@ public abstract class PostAnalysisQueryBuildTool<T> extends BuildTool {
       WalkableGraph walkableGraph);
 
   private BuildConfiguration getBuildConfiguration(
-      List<TargetAndConfiguration> topLevelTargetsWithConfigs, QueryExpression queryExpression)
-      throws QueryException {
+      Collection<TargetAndConfiguration> topLevelTargetsWithConfigs,
+      QueryExpression queryExpression)
+          throws QueryException {
     // Currently, CTQE assumes that all top level targets take on the same default config and we
     // don't have the ability to map multiple configs to multiple top level targets.
     // So for now, we only allow multiple targets when they all carry the same config.
@@ -115,7 +117,7 @@ public abstract class PostAnalysisQueryBuildTool<T> extends BuildTool {
   private void doPostAnalysisQuery(
       BuildRequest request,
       BuildConfiguration hostConfiguration,
-      List<TargetAndConfiguration> topLevelTargetsWithConfigs,
+      Collection<TargetAndConfiguration> topLevelTargetsWithConfigs,
       QueryExpression queryExpression)
       throws InterruptedException, QueryException, IOException {
     BuildConfiguration targetConfig =