Remove LoadingResult

Instead, refactor the code to use TargetPatternPhaseValue exclusively. This
removes the need to convert from TargetPatternPhaseValue to LoadingResult, and
prepares for interleaving.

It also reduces the number of Skyframe calls which may speed up null builds a
bit, as a followup for https://github.com/bazelbuild/bazel/commit/1067310e18cb9ac203110726de0be53bdc403cea.

PiperOrigin-RevId: 205989338

1 files changed, 23 insertions, 17 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 2f3e9b2c5f..0cc40fe8a5 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
@@ -46,12 +46,12 @@ import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
-import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.profiler.ProfilePhase;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
+import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.RegexFilter;
 import com.google.devtools.common.options.OptionsParsingException;
@@ -81,7 +81,7 @@ public final class AnalysisPhaseRunner {
           InvalidConfigurationException {
 
     // Target pattern evaluation.
-    LoadingResult loadingResult;
+    TargetPatternPhaseValue loadingResult;
     Profiler.instance().markPhase(ProfilePhase.LOAD);
     try (SilentCloseable c = Profiler.instance().profile("evaluateTargetPatterns")) {
       loadingResult = evaluateTargetPatterns(request, validator);
@@ -90,17 +90,22 @@ public final class AnalysisPhaseRunner {
 
     // Compute the heuristic instrumentation filter if needed.
     if (request.needsInstrumentationFilter()) {
-      String instrumentationFilter =
-          InstrumentationFilterSupport.computeInstrumentationFilter(
-              env.getReporter(), loadingResult.getTestsToRun());
-      try {
-        // We're modifying the buildOptions in place, which is not ideal, but we also don't want
-        // to pay the price for making a copy. Maybe reconsider later if this turns out to be a
-        // problem (and the performance loss may not be a big deal).
-        buildOptions.get(BuildConfiguration.Options.class).instrumentationFilter =
-            new RegexFilter.RegexFilterConverter().convert(instrumentationFilter);
-      } catch (OptionsParsingException e) {
-        throw new InvalidConfigurationException(e);
+      try (SilentCloseable c = Profiler.instance().profile("Compute instrumentation filter")) {
+        String instrumentationFilter =
+            InstrumentationFilterSupport.computeInstrumentationFilter(
+                env.getReporter(),
+                // TODO(ulfjack): Expensive. Make this part of the TargetPatternPhaseValue or write
+                // a new SkyFunction to compute it?
+                loadingResult.getTestsToRun(env.getReporter(), env.getPackageManager()));
+        try {
+          // We're modifying the buildOptions in place, which is not ideal, but we also don't want
+          // to pay the price for making a copy. Maybe reconsider later if this turns out to be a
+          // problem (and the performance loss may not be a big deal).
+          buildOptions.get(BuildConfiguration.Options.class).instrumentationFilter =
+              new RegexFilter.RegexFilterConverter().convert(instrumentationFilter);
+        } catch (OptionsParsingException e) {
+          throw new InvalidConfigurationException(e);
+        }
       }
     }
 
@@ -181,11 +186,11 @@ public final class AnalysisPhaseRunner {
     return analysisResult;
   }
 
-  private final LoadingResult evaluateTargetPatterns(
+  private final TargetPatternPhaseValue evaluateTargetPatterns(
       final BuildRequest request, final TargetValidator validator)
       throws LoadingFailedException, TargetParsingException, InterruptedException {
     boolean keepGoing = request.getKeepGoing();
-    LoadingResult result =
+    TargetPatternPhaseValue result =
         env.getSkyframeExecutor()
             .loadTargetPatterns(
                 env.getReporter(),
@@ -195,7 +200,8 @@ public final class AnalysisPhaseRunner {
                 keepGoing,
                 request.shouldRunTests());
     if (validator != null) {
-      Collection<Target> targets = result.getTargets();
+      Collection<Target> targets =
+          result.getTargets(env.getReporter(), env.getSkyframeExecutor().getPackageManager());
       validator.validateTargets(targets, keepGoing);
     }
     return result;
@@ -213,7 +219,7 @@ public final class AnalysisPhaseRunner {
    */
   private AnalysisResult runAnalysisPhase(
       BuildRequest request,
-      LoadingResult loadingResult,
+      TargetPatternPhaseValue loadingResult,
       BuildConfigurationCollection configurations)
       throws InterruptedException, ViewCreationFailedException {
     Stopwatch timer = Stopwatch.createStarted();