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

6 files changed, 74 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
index 26803f8294..1bed59f362 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
@@ -89,4 +89,13 @@ public class AnalysisOptions extends OptionsBase {
     help = "No-op."
   )
   public boolean interleaveLoadingAndAnalysis;
+
+  @Option(
+    name = "experimental_skyframe_prepare_analysis",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+    effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+    help = "Switches analysis preparation to a new code path based on Skyframe."
+  )
+  public boolean skyframePrepareAnalysis;
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
index 1fc7048aa5..fa7984d1d7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
@@ -21,7 +21,6 @@ import com.google.devtools.build.lib.actions.PackageRoots;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.skyframe.AspectValue;
 import java.util.Collection;
-import java.util.List;
 import javax.annotation.Nullable;
 
 /**
@@ -41,7 +40,7 @@ public final class AnalysisResult {
   private final ImmutableSet<AspectValue> aspects;
   private final PackageRoots packageRoots;
   private final String workspaceName;
-  private final List<TargetAndConfiguration> topLevelTargetsWithConfigs;
+  private final Collection<TargetAndConfiguration> topLevelTargetsWithConfigs;
 
   AnalysisResult(
       BuildConfigurationCollection configurations,
@@ -57,7 +56,7 @@ public final class AnalysisResult {
       TopLevelArtifactContext topLevelContext,
       PackageRoots packageRoots,
       String workspaceName,
-      List<TargetAndConfiguration> topLevelTargetsWithConfigs) {
+      Collection<TargetAndConfiguration> topLevelTargetsWithConfigs) {
     this.configurations = configurations;
     this.targetsToBuild = ImmutableSet.copyOf(targetsToBuild);
     this.aspects = aspects;
@@ -157,7 +156,7 @@ public final class AnalysisResult {
     return workspaceName;
   }
 
-  public List<TargetAndConfiguration> getTopLevelTargetsWithConfigs() {
+  public Collection<TargetAndConfiguration> getTopLevelTargetsWithConfigs() {
     return topLevelTargetsWithConfigs;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
index 32fa07f03f..b7d8e14281 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -196,13 +196,13 @@ public final class AnalysisUtils {
    *
    * <p>Preserves the original input ordering.
    */
+  // Keep this in sync with PrepareAnalysisPhaseFunction.
   public static List<TargetAndConfiguration> getTargetsWithConfigs(
       BuildConfigurationCollection configurations,
       Collection<Target> targets,
       ExtendedEventHandler eventHandler,
       ConfiguredRuleClassProvider ruleClassProvider,
-      SkyframeExecutor skyframeExecutor)
-      throws InterruptedException {
+      SkyframeExecutor skyframeExecutor) {
     // We use a hash set here to remove duplicate nodes; this can happen for input files and package
     // groups.
     LinkedHashSet<TargetAndConfiguration> nodes = new LinkedHashSet<>(targets.size());
@@ -225,7 +225,7 @@ public final class AnalysisUtils {
 
   @VisibleForTesting
   public static Multimap<BuildConfiguration, Dependency> targetsToDeps(
-      LinkedHashSet<TargetAndConfiguration> nodes, ConfiguredRuleClassProvider ruleClassProvider) {
+      Collection<TargetAndConfiguration> nodes, ConfiguredRuleClassProvider ruleClassProvider) {
     Multimap<BuildConfiguration, Dependency> asDeps =
         ArrayListMultimap.<BuildConfiguration, Dependency>create();
     for (TargetAndConfiguration targetAndConfig : nodes) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 9995de0c73..e93d119eb1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -15,13 +15,11 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
 import com.google.common.eventbus.EventBus;
@@ -34,6 +32,8 @@ import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.PackageRoots;
 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.analysis.constraints.TopLevelConstraintSemantics;
 import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory;
 import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
@@ -55,11 +55,14 @@ import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.pkgcache.PackageManager.PackageManagerStatistics;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.skyframe.CoverageReportValue;
+import com.google.devtools.build.lib.skyframe.PrepareAnalysisPhaseValue;
 import com.google.devtools.build.lib.skyframe.SkyframeAnalysisResult;
 import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -182,7 +185,8 @@ public class BuildView {
   @ThreadCompatible
   public AnalysisResult update(
       TargetPatternPhaseValue loadingResult,
-      BuildConfigurationCollection configurations,
+      BuildOptions targetOptions,
+      Set<String> multiCpu,
       List<String> aspects,
       AnalysisOptions viewOptions,
       boolean keepGoing,
@@ -190,7 +194,7 @@ public class BuildView {
       TopLevelArtifactContext topLevelOptions,
       ExtendedEventHandler eventHandler,
       EventBus eventBus)
-      throws ViewCreationFailedException, InterruptedException {
+      throws ViewCreationFailedException, InvalidConfigurationException, InterruptedException {
     logger.info("Starting analysis");
     pollInterruptedStatus();
 
@@ -202,12 +206,49 @@ public class BuildView {
         loadingResult.getTargets(eventHandler, skyframeExecutor.getPackageManager());
     eventBus.post(new AnalysisPhaseStartedEvent(targets));
 
+    // Prepare the analysis phase
+    BuildConfigurationCollection configurations;
+    Collection<TargetAndConfiguration> topLevelTargetsWithConfigs;
+    if (viewOptions.skyframePrepareAnalysis) {
+      PrepareAnalysisPhaseValue prepareAnalysisPhaseValue;
+      try (SilentCloseable c = Profiler.instance().profile("Prepare analysis phase")) {
+        prepareAnalysisPhaseValue = skyframeExecutor.prepareAnalysisPhase(
+            eventHandler, targetOptions, multiCpu, loadingResult.getTargetLabels());
+  
+        // Determine the configurations
+        configurations =
+            prepareAnalysisPhaseValue.getConfigurations(eventHandler, skyframeExecutor);
+        topLevelTargetsWithConfigs =
+            prepareAnalysisPhaseValue.getTopLevelCts(eventHandler, skyframeExecutor);
+      }
+    } else {
+      // 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.
+      try (SilentCloseable c = Profiler.instance().profile("createConfigurations")) {
+        configurations =
+            skyframeExecutor
+                .createConfigurations(
+                    eventHandler,
+                    targetOptions,
+                    multiCpu,
+                    keepGoing);
+      }
+      try (SilentCloseable c = Profiler.instance().profile("AnalysisUtils.getTargetsWithConfigs")) {
+        topLevelTargetsWithConfigs =
+            AnalysisUtils.getTargetsWithConfigs(
+                configurations, targets, eventHandler, ruleClassProvider, skyframeExecutor);
+      }
+    }
+
     skyframeBuildView.setConfigurations(eventHandler, configurations);
 
-    // Determine the configurations.
-    List<TargetAndConfiguration> topLevelTargetsWithConfigs =
-        AnalysisUtils.getTargetsWithConfigs(
-            configurations, targets, eventHandler, ruleClassProvider, skyframeExecutor);
+    if (configurations.getTargetConfigurations().size() == 1) {
+      eventBus
+          .post(
+              new MakeEnvironmentEvent(
+                  configurations.getTargetConfigurations().get(0).getMakeEnvironment()));
+    }
 
     // Report the generated association of targets to configurations
     Multimap<Label, BuildConfiguration> byLabel =
@@ -220,14 +261,10 @@ public class BuildView {
     }
 
     List<ConfiguredTargetKey> topLevelCtKeys =
-        Lists.transform(
-            topLevelTargetsWithConfigs,
-            new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
-              @Override
-              public ConfiguredTargetKey apply(TargetAndConfiguration node) {
-                return ConfiguredTargetKey.of(node.getLabel(), node.getConfiguration());
-              }
-            });
+        topLevelTargetsWithConfigs
+            .stream()
+            .map(node -> ConfiguredTargetKey.of(node.getLabel(), node.getConfiguration()))
+            .collect(Collectors.toList());
 
     Multimap<Pair<Label, String>, BuildConfiguration> aspectConfigurations =
         ArrayListMultimap.create();
@@ -362,7 +399,7 @@ public class BuildView {
       AnalysisOptions viewOptions,
       SkyframeAnalysisResult skyframeAnalysisResult,
       Set<ConfiguredTarget> targetsToSkip,
-      List<TargetAndConfiguration> topLevelTargetsWithConfigs)
+      Collection<TargetAndConfiguration> topLevelTargetsWithConfigs)
       throws InterruptedException {
     Set<Label> testsToRun = loadingResult.getTestsToRunLabels();
     Set<ConfiguredTarget> configuredTargets =
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
index fb9a66a8c4..20459337d4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
@@ -17,8 +17,8 @@ package com.google.devtools.build.lib.analysis.config;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import java.util.Collection;
 import java.util.HashMap;
-import java.util.List;
 
 /**
  * Convenience container for top-level target and host configurations.
@@ -34,9 +34,10 @@ public final class BuildConfigurationCollection {
   private final ImmutableList<BuildConfiguration> targetConfigurations;
   private final BuildConfiguration hostConfiguration;
 
-  public BuildConfigurationCollection(List<BuildConfiguration> targetConfigurations,
+  public BuildConfigurationCollection(
+      Collection<BuildConfiguration> targetConfigurations,
       BuildConfiguration hostConfiguration)
-      throws InvalidConfigurationException {
+          throws InvalidConfigurationException {
     this.targetConfigurations = ImmutableList.copyOf(targetConfigurations);
     this.hostConfiguration = hostConfiguration;
 
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 405d272649..6dde833cdc 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
@@ -558,6 +558,7 @@ public final class ConfigurationResolver {
   //   should never make it through analysis (and especially not seed ConfiguredTargetValues)
   // TODO(gregce): merge this more with resolveConfigurations? One crucial difference is
   //   resolveConfigurations can null-return on missing deps since it executes inside Skyfunctions.
+  // Keep this in sync with {@link PrepareAnalysisPhaseFunction#resolveConfigurations}.
   public static LinkedHashSet<TargetAndConfiguration> getConfigurationsFromExecutor(
       Iterable<TargetAndConfiguration> defaultContext,
       Multimap<BuildConfiguration, Dependency> targetsToEvaluate,