Make incremental build with Skymeld and BwoB faster.

Previously, a incremental build with Skymeld and BwoB has to re-evaluate action node in order to correctly staging toplevel artifacts. The reason is BwoB requires full analysis result before execution phrase to correctly invalidate action nodes for toplevel artifacts and with Skymeld, the full analysis result is not available yet when execution phrase starts.

This CL changes how BwoB invalidate action nodes before entering execution phrase: instead of depending on full analysis result, it now uses `remoteOutputChecker` from previous build -- if `lastRemoteOutputChecker` said Bazel should download one toplevel artifact but it's missing locally, we invalidate the action node. For clean build, `lastRemoteOutputChecker` is initialized to `null` because there is no action nodes to invalidate.

If an artifact is promoted from intermediate output to toplevel in an incremental build, no action node is invalidated. Instead, we rely on `CompletionFunction` to download them. There is one caveat though: sometimes we need to invalidate `CompletionFunction`, e.g. when `outputsMode` is changed.

Working towards #22367.

PiperOrigin-RevId: 667562015
Change-Id: Ida86cf04d99fcdec998970eb3cabd63b2318246f

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

@@ -323,15 +323,15 @@ void prepareForExecution(Stopwatch executionTimer)
     }
 
     skyframeExecutor.drainChangedFiles();
-    // With Skymeld, we don't have enough information at this stage to consider remote files.
-    // This allows BwoB to function (in the sense that it's now compatible with Skymeld), but
-    // there's a performance penalty for incremental build: all action nodes will be dirtied.
-    // We'll be relying on the other forms of caching (local action cache or remote cache).
-    // TODO(b/281655526): Improve this.
+
+    var remoteArtifactChecker =
+        env.getOutputService() != null
+            ? env.getOutputService().getRemoteArtifactChecker()
+            : RemoteArtifactChecker.IGNORE_ALL;
     skyframeExecutor.detectModifiedOutputFiles(
         modifiedOutputFiles,
         env.getBlazeWorkspace().getLastExecutionTimeRange(),
-        RemoteArtifactChecker.IGNORE_ALL,
+        remoteArtifactChecker,
         buildRequestOptions.fsvcThreads);
     try (SilentCloseable c = Profiler.instance().profile("configureActionExecutor")) {
       skyframeExecutor.configureActionExecutor(

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -215,7 +215,9 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/options",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/skyframe:coverage_report_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/skyframe",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -137,6 +137,7 @@ public final class RemoteModule extends BlazeModule {
   @Nullable private TempPathGenerator tempPathGenerator;
   @Nullable private BlockWaitingModule blockWaitingModule;
   @Nullable private RemoteOutputChecker remoteOutputChecker;
+  @Nullable private RemoteOutputChecker lastRemoteOutputChecker;
   @Nullable private String lastBuildId;
 
   private ChannelFactory channelFactory =
@@ -372,7 +373,9 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
             new JavaClock(),
             env.getCommandName(),
             remoteOptions.remoteOutputsMode,
-            patternsToDownloadBuilder.build());
+            patternsToDownloadBuilder.build(),
+            lastRemoteOutputChecker);
+    remoteOutputChecker.maybeInvalidateSkyframeValues(env.getSkyframeExecutor().getEvaluator());
 
     env.getEventBus().register(this);
     String invocationId = env.getCommandId().toString();
@@ -922,6 +925,7 @@ public void afterCommand() {
           () -> afterCommandTask(actionContextProviderRef, tempPathGeneratorRef, rpcLogFileRef));
     }
 
+    lastRemoteOutputChecker = remoteOutputChecker;
     lastBuildId = Preconditions.checkNotNull(env).getCommandId().toString();
 
     buildEventArtifactUploaderFactoryDelegate.reset();

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java

@@ -36,7 +36,9 @@
 import com.google.devtools.build.lib.clock.Clock;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.remote.util.ConcurrentPathTrie;
+import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.skyframe.MemoizingEvaluator;
 import java.util.function.Supplier;
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
@@ -55,13 +57,24 @@ private enum CommandMode {
   private final CommandMode commandMode;
   private final RemoteOutputsMode outputsMode;
   private final ImmutableList<Pattern> patternsToDownload;
+  @Nullable private final RemoteOutputChecker lastRemoteOutputChecker;
+
   private final ConcurrentPathTrie pathsToDownload = new ConcurrentPathTrie();
 
   public RemoteOutputChecker(
       Clock clock,
       String commandName,
       RemoteOutputsMode outputsMode,
       ImmutableList<Pattern> patternsToDownload) {
+    this(clock, commandName, outputsMode, patternsToDownload, /* lastRemoteOutputChecker= */ null);
+  }
+
+  public RemoteOutputChecker(
+      Clock clock,
+      String commandName,
+      RemoteOutputsMode outputsMode,
+      ImmutableList<Pattern> patternsToDownload,
+      RemoteOutputChecker lastRemoteOutputChecker) {
     this.clock = clock;
     this.commandMode =
         switch (commandName) {
@@ -73,6 +86,7 @@ public RemoteOutputChecker(
         };
     this.outputsMode = outputsMode;
     this.patternsToDownload = patternsToDownload;
+    this.lastRemoteOutputChecker = lastRemoteOutputChecker;
   }
 
   // Skymeld-only.
@@ -264,13 +278,40 @@ public boolean shouldDownloadOutput(PathFragment execPath) {
 
   @Override
   public boolean shouldTrustRemoteArtifact(ActionInput file, RemoteFileArtifactValue metadata) {
+    // If Bazel should download this file, but it does not exist locally, returns false to rerun
+    // the generating action to trigger the download (just like in the normal build, when local
+    // outputs are missing).
+
+    if (lastRemoteOutputChecker != null) {
+      // This is an incremental build. If the file was downloaded by previous build and is now
+      // missing, invalidate the action.
+      if (lastRemoteOutputChecker.shouldDownloadOutput(file)) {
+        return false;
+      }
+    }
+
     if (shouldDownloadOutput(file)) {
-      // If Bazel should download this file, but it does not exist locally, returns false to rerun
-      // the generating action to trigger the download (just like in the normal build, when local
-      // outputs are missing).
       return false;
     }
 
     return metadata.isAlive(clock.now());
   }
+
+  public void maybeInvalidateSkyframeValues(MemoizingEvaluator memoizingEvaluator) {
+    if (lastRemoteOutputChecker == null) {
+      return;
+    }
+
+    // If the outputsMode or commandMode is changed, we invalidate completion functions. Otherwise,
+    // some requested outputs might not be correctly downloaded.
+    if (lastRemoteOutputChecker.outputsMode != outputsMode
+        || lastRemoteOutputChecker.commandMode != commandMode) {
+      memoizingEvaluator.delete(
+          k -> {
+            var functionName = k.functionName();
+            return functionName.equals(SkyFunctions.TARGET_COMPLETION)
+                || functionName.equals(SkyFunctions.ASPECT_COMPLETION);
+          });
+    }
+  }
 }

src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java

@@ -511,14 +511,14 @@ public void remoteCacheEvictBlobs_whenUploadingInputTree_incrementalBuildCanCont
     restartServer();
 
     // Clean build, foo.out isn't downloaded
+    setDownloadToplevel();
     buildTarget("//a:bar");
     assertOutputDoesNotExist("a/foo.out/file-inside");
 
     // Evict blobs from remote cache
     evictAllBlobs();
 
     // trigger build error
-    setDownloadToplevel();
     write("a/bar.in", "updated bar");
     // Build failed because of remote cache eviction
     assertThrows(BuildFailedException.class, () -> buildTarget("//a:bar"));

src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java

@@ -66,11 +66,6 @@ protected void waitDownloads() throws Exception {
     runtimeWrapper.newCommand();
   }
 
-  // TODO(b/281655526) incompatible with Skymeld.
-  protected void setIncompatibleWithSkymeld() {
-    addOptions("--noexperimental_merged_skyframe_analysis_execution");
-  }
-
   @Test
   public void outputsAreNotDownloaded() throws Exception {
     write(
@@ -891,6 +886,40 @@ public void downloadToplevel_incrementalBuild_multipleToplevelTargets() throws E
     //     .isTrue();
   }
 
+  @Test
+  public void downloadToplevel_incrementalBuild_anotherTarget() throws Exception {
+    write(
+        "BUILD",
+        "genrule(",
+        "  name = 'foo1',",
+        "  srcs = [':foo3'],",
+        "  outs = ['out/foo1.txt'],",
+        "  cmd = 'echo foo1 > $@',",
+        ")",
+        "genrule(",
+        "  name = 'foo2',",
+        "  srcs = [],",
+        "  outs = ['out/foo2.txt'],",
+        "  cmd = 'echo foo2 > $@',",
+        ")",
+        "genrule(",
+        "  name = 'foo3',",
+        "  srcs = [],",
+        "  outs = ['out/foo3.txt'],",
+        "  cmd = 'echo foo3 > $@',",
+        ")");
+    setDownloadToplevel();
+    buildTarget("//:foo1");
+
+    assertValidOutputFile("out/foo1.txt", "foo1\n");
+    assertOutputsDoNotExist("//:foo2");
+    assertOutputsDoNotExist("//:foo3");
+
+    buildTarget("//:foo3");
+    assertOutputsDoNotExist("//:foo2");
+    assertValidOutputFile("out/foo3.txt", "foo3\n");
+  }
+
   @Test
   public void downloadToplevel_symlinkToGeneratedFile() throws Exception {
     setDownloadToplevel();
@@ -1240,7 +1269,6 @@ public void incrementalBuild_sourceModified_rerunActions() throws Exception {
 
   @Test
   public void incrementalBuild_intermediateOutputDeleted_nothingIsReEvaluated() throws Exception {
-    setIncompatibleWithSkymeld();
     // Arrange: Prepare workspace and run a clean build
     write(
         "BUILD",
@@ -1394,7 +1422,6 @@ public void incrementalBuild_intermediateOutputModified_rerunGeneratingActions()
   @Test
   public void remoteCacheEvictBlobs_whenPrefetchingInputFile_incrementalBuildCanContinue()
       throws Exception {
-    setIncompatibleWithSkymeld();
     // Arrange: Prepare workspace and populate remote cache
     write(
         "a/BUILD",
@@ -1425,7 +1452,6 @@ public void remoteCacheEvictBlobs_whenPrefetchingInputFile_incrementalBuildCanCo
     getOutputPath("a/bar.out").delete();
     getOutputBase().getRelative("action_cache").deleteTreesBelow();
     restartServer();
-    setIncompatibleWithSkymeld();
 
     // Clean build, foo.out isn't downloaded
     buildTarget("//a:bar");
@@ -1450,7 +1476,6 @@ public void remoteCacheEvictBlobs_whenPrefetchingInputFile_incrementalBuildCanCo
   @Test
   public void remoteCacheEvictBlobs_whenPrefetchingInputTree_incrementalBuildCanContinue()
       throws Exception {
-    setIncompatibleWithSkymeld();
     // Arrange: Prepare workspace and populate remote cache
     write("BUILD");
     writeOutputDirRule();
@@ -1482,7 +1507,6 @@ public void remoteCacheEvictBlobs_whenPrefetchingInputTree_incrementalBuildCanCo
     getOutputPath("a/bar.out").delete();
     getOutputBase().getRelative("action_cache").deleteTreesBelow();
     restartServer();
-    setIncompatibleWithSkymeld();
 
     // Clean build, foo.out isn't downloaded
     buildTarget("//a:bar");

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -375,8 +375,8 @@ EOF
     --remote_download_toplevel \
     //a:foo >& $TEST_log || fail "Failed to build //a:foobar"
 
-  # Output of foo is missing, the generating action is re-executed
-  expect_log "2 processes: 1 remote cache hit, 1 internal."
+  # action "foo" is not re-executed.
+  expect_log "1 process: 1 internal."
 
   [[ -f bazel-bin/a/foo.txt ]] \
     || fail "Expected toplevel output bazel-bin/a/foo.txt to be downloaded"