Add an experimental flag to enable the Skyframe-based loading phase runner.

Refactor the CommandEnvironment to no longer create a LoadingPhaseRunner
eagerly. Unfortunately, that means we can't reuse the TargetPatternEvaluator
either. Fortunately, the SkyframeTargetPatternEvaluator is a very lightweight
class, which only contains the offset as state, so we just create a new
instance whenever we need one.

On the plus side, the LoadingPhaseRunner API is now stateless, and the
QueryCommand no longer creates a unnecessary LoadingPhaseRunner instance.

--
MOS_MIGRATED_REVID=114519731

7 files changed, 36 insertions, 49 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index a37ff05691..4113fb3d6a 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -65,6 +65,7 @@ import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.lib.pkgcache.LoadingCallback;
 import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
+import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.profiler.ProfilePhase;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -398,10 +399,13 @@ public final class BuildTool {
       }
     };
 
-    LoadingResult result = env.getLoadingPhaseRunner().execute(getReporter(),
-        env.getEventBus(), request.getTargets(), request.getLoadingOptions(),
-        runtime.createBuildOptions(request).getAllLabels(), keepGoing,
-        isLoadingEnabled(request), request.shouldRunTests(), callback);
+    LoadingPhaseRunner loadingPhaseRunner = runtime.getSkyframeExecutor().getLoadingPhaseRunner(
+        runtime.getPackageFactory().getRuleClassNames(),
+        request.getLoadingOptions().useSkyframeTargetPatternEvaluator);
+    LoadingResult result = loadingPhaseRunner.execute(getReporter(),
+        env.getEventBus(), request.getTargets(), env.getRelativeWorkingDirectory(),
+        request.getLoadingOptions(), runtime.createBuildOptions(request).getAllLabels(),
+        keepGoing, isLoadingEnabled(request), request.shouldRunTests(), callback);
     env.throwPendingException();
     return result;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 7fda021118..6d68c47778 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -101,16 +101,6 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
     this.ruleNames = ruleNames;
   }
 
-  @Override
-  public TargetPatternEvaluator getTargetPatternEvaluator() {
-    return targetPatternEvaluator;
-  }
-
-  @Override
-  public void updatePatternEvaluator(PathFragment relativeWorkingDirectory) {
-    targetPatternEvaluator.updateOffset(relativeWorkingDirectory);
-  }
-
   /**
    * Performs target pattern evaluation, test suite expansion (if requested), and loads the
    * transitive closure of the resulting targets as well as of the targets needed to use the
@@ -118,7 +108,7 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
    */
   @Override
   public LoadingResult execute(EventHandler eventHandler, EventBus eventBus,
-      List<String> targetPatterns, LoadingOptions options,
+      List<String> targetPatterns, PathFragment relativeWorkingDirectory, LoadingOptions options,
       ListMultimap<String, Label> labelsToLoadUnconditionally, boolean keepGoing,
       boolean enableLoading, boolean determineTests, @Nullable LoadingCallback callback)
           throws TargetParsingException, LoadingFailedException, InterruptedException {
@@ -129,6 +119,7 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
           + "the --build_tests_only option or the 'bazel test' command ");
     }
 
+    targetPatternEvaluator.updateOffset(relativeWorkingDirectory);
     EventHandler parseFailureListener = new ParseFailureListenerImpl(eventHandler, eventBus);
     // Determine targets to build:
     ResolvedTargets<Target> targets = getTargetsToBuild(parseFailureListener,
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
index 6739388cad..2f40193fd0 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java
@@ -100,4 +100,16 @@ public class LoadingOptions extends OptionsBase {
              "This option affects --build_tests_only behavior and the test command."
       )
   public List<String> testLangFilterList;
+
+  // If this option is set, the value of experimental_interleave_loading_and_analysis is completely
+  // ignored. This enables a different LoadingPhaseRunner implementation which doesn't implement
+  // the loading phase at all, and therefore can't currently support the other flag. If we roll this
+  // out soonish, then we're never going to implement the legacy code path in the new
+  // implementation, making it a moot point.
+  @Option(name = "experimental_skyframe_target_pattern_evaluator",
+      defaultValue = "false",
+      category = "hidden",
+      help = "Use the Skyframe-based target pattern evaluator; implies "
+          + "--experimental_interleave_loading_and_analysis.")
+  public boolean useSkyframeTargetPatternEvaluator;
 }
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
index 1a9849098e..210b154ec5 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
@@ -54,18 +54,13 @@ import javax.annotation.Nullable;
  * maximize caching, it is vital that these change as rarely as possible.
  */
 public abstract class LoadingPhaseRunner {
-
-  public abstract TargetPatternEvaluator getTargetPatternEvaluator();
-
-  public abstract void updatePatternEvaluator(PathFragment relativeWorkingDirectory);
-
   /**
    * Performs target pattern evaluation, test suite expansion (if requested), and loads the
    * transitive closure of the resulting targets as well as of the targets needed to use the
    * given build configuration provider.
    */
   public abstract LoadingResult execute(EventHandler eventHandler, EventBus eventBus,
-      List<String> targetPatterns, LoadingOptions options,
+      List<String> targetPatterns, PathFragment relativeWorkingDirectory, LoadingOptions options,
       ListMultimap<String, Label> labelsToLoadUnconditionally, boolean keepGoing,
       boolean enableLoading, boolean determineTests, @Nullable LoadingCallback callback)
           throws TargetParsingException, LoadingFailedException, InterruptedException;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index a05d6f472f..952b71e082 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -36,7 +36,6 @@ import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.OutputService;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
@@ -49,6 +48,7 @@ import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.OptionPriority;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
@@ -70,8 +70,6 @@ import java.util.concurrent.atomic.AtomicReference;
  * command is done and all corresponding objects are garbage collected.
  */
 public final class CommandEnvironment {
-  private static final boolean USE_SKYFRAME_LOADING_PHASE = false;
-
   private final BlazeRuntime runtime;
 
   private final UUID commandId;  // Unique identifier for the command being run
@@ -80,9 +78,9 @@ public final class CommandEnvironment {
   private final BlazeModule.ModuleEnvironment blazeModuleEnvironment;
   private final Map<String, String> clientEnv = new HashMap<>();
 
-  private final LoadingPhaseRunner loadingPhaseRunner;
   private final BuildView view;
 
+  private PathFragment relativeWorkingDirectory = PathFragment.EMPTY_FRAGMENT;
   private long commandStartTime;
   private OutputService outputService;
   private String outputFileSystem;
@@ -113,8 +111,6 @@ public final class CommandEnvironment {
     this.eventBus = eventBus;
     this.blazeModuleEnvironment = new BlazeModuleEnvironment();
 
-    this.loadingPhaseRunner = runtime.getSkyframeExecutor().getLoadingPhaseRunner(
-        runtime.getPackageFactory().getRuleClassNames(), USE_SKYFRAME_LOADING_PHASE);
     this.view = new BuildView(runtime.getDirectories(), runtime.getRuleClassProvider(),
         runtime.getSkyframeExecutor(), runtime.getCoverageReportActionFactory());
 
@@ -169,15 +165,17 @@ public final class CommandEnvironment {
     return runtime.getPackageManager();
   }
 
-  public LoadingPhaseRunner getLoadingPhaseRunner() {
-    return loadingPhaseRunner;
+  public PathFragment getRelativeWorkingDirectory() {
+    return relativeWorkingDirectory;
   }
 
   /**
-   * Returns the target pattern parser.
+   * Creates and returns a new target pattern parser.
    */
-  public TargetPatternEvaluator getTargetPatternEvaluator() {
-    return loadingPhaseRunner.getTargetPatternEvaluator();
+  public TargetPatternEvaluator newTargetPatternEvaluator() {
+    TargetPatternEvaluator result = getPackageManager().newTargetPatternEvaluator();
+    result.updateOffset(relativeWorkingDirectory);
+    return result;
   }
 
   public BuildView getView() {
@@ -366,7 +364,7 @@ public final class CommandEnvironment {
       workspace = FileSystemUtils.getWorkingDirectory(runtime.getDirectories().getFileSystem());
       workingDirectory = workspace;
     }
-    loadingPhaseRunner.updatePatternEvaluator(workingDirectory.relativeTo(workspace));
+    this.relativeWorkingDirectory = workingDirectory.relativeTo(workspace);
     this.workingDirectory = workingDirectory;
 
     updateClientEnv(options.clientEnv, options.ignoreClientEnv);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
index b7769089fc..45a76e5ed9 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
@@ -253,7 +253,7 @@ public final class QueryCommand implements BlazeCommand {
         env.getPackageManager().newTransitiveLoader(),
         env.getSkyframeExecutor(),
         env.getPackageManager(),
-        env.getTargetPatternEvaluator(),
+        env.newTargetPatternEvaluator(),
         keepGoing, orderedResults, universeScope, loadingPhaseThreads, env.getReporter(),
         settings,
         functions.build(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 92c680424f..470e562902 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -100,7 +100,6 @@ import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
@@ -1729,33 +1728,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    * TargetPatternPhaseFunction}.
    */
   final class SkyframeLoadingPhaseRunner extends LoadingPhaseRunner {
-    private final TargetPatternEvaluator targetPatternEvaluator;
     private final Set<String> ruleClassNames;
 
     public SkyframeLoadingPhaseRunner(Set<String> ruleClassNames) {
-      this.targetPatternEvaluator = getPackageManager().newTargetPatternEvaluator();
       this.ruleClassNames = ruleClassNames;
     }
 
     @Override
-    public TargetPatternEvaluator getTargetPatternEvaluator() {
-      return targetPatternEvaluator;
-    }
-
-    @Override
-    public void updatePatternEvaluator(PathFragment relativeWorkingDirectory) {
-      targetPatternEvaluator.updateOffset(relativeWorkingDirectory);
-    }
-
-    @Override
     public LoadingResult execute(EventHandler eventHandler, EventBus eventBus,
-        List<String> targetPatterns, LoadingOptions options,
+        List<String> targetPatterns, PathFragment relativeWorkingDirectory, LoadingOptions options,
         ListMultimap<String, Label> labelsToLoadUnconditionally, boolean keepGoing,
         boolean enableLoading, boolean determineTests, @Nullable LoadingCallback callback)
         throws TargetParsingException, LoadingFailedException, InterruptedException {
       Stopwatch timer = Stopwatch.createStarted();
       SkyKey key = TargetPatternPhaseValue.key(ImmutableList.copyOf(targetPatterns),
-          targetPatternEvaluator.getOffset(), options.compileOneDependency,
+          relativeWorkingDirectory.getPathString(), options.compileOneDependency,
           options.buildTestsOnly, determineTests,
           TestFilter.forOptions(options, eventHandler, ruleClassNames));
       EvaluationResult<TargetPatternPhaseValue> evalResult =