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

5 files changed, 75 insertions, 15 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 48e4baf5d1..b6107f4915 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -1353,4 +1353,58 @@ public class BuildViewTest extends BuildViewTestBase {
       return super.defaultFlags().with(Flag.TRIMMED_CONFIGURATIONS);
     }
   }
+
+  /** Runs the same test with the Skyframe-based analysis prep. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithSkyframePrepareAnalysis extends BuildViewTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.SKYFRAME_PREPARE_ANALYSIS);
+    }
+  }
+
+  /** Runs the same test with the Skyframe-based analysis prep and trimmed configurations. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithSkyframePrepareAnalysisAndTrimmedConfigurations extends BuildViewTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags()
+          .with(Flag.SKYFRAME_PREPARE_ANALYSIS)
+          .with(Flag.TRIMMED_CONFIGURATIONS);
+    }
+
+    // We can't recover from dependency cycles in TransitiveTargetFunction, so we disable the tests
+    // for now. We will likely have to fix this before we can enable the new code by default.
+    @Override
+    @Test
+    public void testCircularDependency() {
+    }
+
+    @Override
+    @Test
+    public void testErrorBelowCycleKeepGoing() {
+    }
+
+    @Override
+    @Test
+    public void testCircularDependencyBelowTwoTargets() {
+    }
+
+    @Override
+    @Test
+    public void testCycleReporting_TargetCycleWhenPackageInError() {
+    }
+
+    @Override
+    @Test
+    public void testCircularDependencyWithKeepGoing() {
+    }
+
+    @Override
+    @Test
+    public void testErrorBelowCycle() {
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index c7d1becb1f..eb4b9cf571 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -101,7 +101,8 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
   public enum Flag {
     KEEP_GOING,
     // Configurations that only include the fragments a target needs to properly analyze.
-    TRIMMED_CONFIGURATIONS
+    TRIMMED_CONFIGURATIONS,
+    SKYFRAME_PREPARE_ANALYSIS
   }
 
   /** Helper class to make it easy to enable and disable flags. */
@@ -318,6 +319,7 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     AnalysisOptions viewOptions = optionsParser.getOptions(AnalysisOptions.class);
     // update --keep_going option if test requested it.
     boolean keepGoing = flags.contains(Flag.KEEP_GOING);
+    viewOptions.skyframePrepareAnalysis = flags.contains(Flag.SKYFRAME_PREPARE_ANALYSIS);
 
     PackageCacheOptions packageCacheOptions = optionsParser.getOptions(PackageCacheOptions.class);
     PathPackageLocator pathPackageLocator =
@@ -359,13 +361,13 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
 
     BuildRequestOptions requestOptions = optionsParser.getOptions(BuildRequestOptions.class);
     ImmutableSortedSet<String> multiCpu = ImmutableSortedSet.copyOf(requestOptions.multiCpus);
-    masterConfig = skyframeExecutor.createConfigurations(
-        reporter, ruleClassProvider.getConfigurationFragments(), buildOptions,
-        multiCpu, false);
+    skyframeExecutor.setConfigurationFragmentFactories(
+        ruleClassProvider.getConfigurationFragments());
     analysisResult =
         buildView.update(
             loadingResult,
-            masterConfig,
+            buildOptions,
+            multiCpu,
             aspects,
             viewOptions,
             keepGoing,
@@ -373,6 +375,7 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
             AnalysisTestUtil.TOP_LEVEL_ARTIFACT_CONTEXT,
             reporter,
             eventBus);
+    masterConfig = analysisResult.getConfigurationCollection();
     return analysisResult;
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index a0fb40f01c..083688d02b 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -140,7 +140,8 @@ public class BuildViewForTesting {
   @ThreadCompatible
   public AnalysisResult update(
       TargetPatternPhaseValue loadingResult,
-      BuildConfigurationCollection configurations,
+      BuildOptions targetOptions,
+      Set<String> multiCpu,
       List<String> aspects,
       AnalysisOptions viewOptions,
       boolean keepGoing,
@@ -148,10 +149,11 @@ public class BuildViewForTesting {
       TopLevelArtifactContext topLevelOptions,
       ExtendedEventHandler eventHandler,
       EventBus eventBus)
-      throws ViewCreationFailedException, InterruptedException {
+      throws ViewCreationFailedException, InterruptedException, InvalidConfigurationException {
     return buildView.update(
         loadingResult,
-        configurations,
+        targetOptions,
+        multiCpu,
         aspects,
         viewOptions,
         keepGoing,
@@ -186,8 +188,7 @@ public class BuildViewForTesting {
    */
   @VisibleForTesting
   public BuildConfiguration getConfigurationForTesting(
-      Target target, BuildConfiguration config, ExtendedEventHandler eventHandler)
-      throws InterruptedException {
+      Target target, BuildConfiguration config, ExtendedEventHandler eventHandler) {
     List<TargetAndConfiguration> node =
         ImmutableList.<TargetAndConfiguration>of(new TargetAndConfiguration(target, config));
     LinkedHashSet<TargetAndConfiguration> configs =
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 5fc77cbe63..c3bea4f72c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -351,9 +351,10 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
     BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser);
     skyframeExecutor.resetConfigurationCollectionForTesting();
+    skyframeExecutor.setConfigurationFragmentFactories(
+        ruleClassProvider.getConfigurationFragments());
     return skyframeExecutor.createConfigurations(
-        reporter, ruleClassProvider.getConfigurationFragments(), buildOptions,
-        ImmutableSet.<String>of(), false);
+        reporter, buildOptions, ImmutableSet.<String>of(), false);
   }
 
   protected Target getTarget(String label)
@@ -1755,7 +1756,8 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     }
     return view.update(
         loadingResult,
-        masterConfig,
+        targetConfig.getOptions(),
+        /* multiCpu= */ ImmutableSet.of(),
         aspects,
         viewOptions,
         keepGoing,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java
index 522b5fa232..b92739176b 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java
@@ -191,9 +191,9 @@ public abstract class ConfigurationTestCase extends FoundationTestCase {
         parser.getOptions(TestOptions.class).multiCpus);
 
     skyframeExecutor.handleDiffs(reporter);
+    skyframeExecutor.setConfigurationFragmentFactories(configurationFragmentFactories);
     BuildConfigurationCollection collection = skyframeExecutor.createConfigurations(
-        reporter, configurationFragmentFactories, BuildOptions.of(buildOptionClasses, parser),
-        multiCpu, false);
+        reporter, BuildOptions.of(buildOptionClasses, parser), multiCpu, false);
     return collection;
   }