Track the entire OutputService instead of just the BatchStatter.

--
MOS_MIGRATED_REVID=107800790

7 files changed, 67 insertions, 22 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
index bc9269b4b8..ef5a23c8ee 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
@@ -268,6 +268,14 @@ public class BuildRequest implements OptionsClassProvider {
                 + "this flag to false to see the effect on incremental build times.")
     public boolean checkOutputFiles;
 
+    @Option(name = "experimental_output_tree_tracking",
+            defaultValue = "false",
+            category = "undocumented",
+            help = "If set, communicate with objsfd to track when files in the output tree have "
+                + "been modified externally (not by Blaze). This should improve incremental build "
+                + "speed.")
+    public boolean finalizeActions;
+
     @Option(
       name = "aspects",
       converter = Converters.CommaSeparatedOptionListConverter.class,
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
index 3a95cbd702..26feb53ddb 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -84,6 +84,7 @@ import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -357,15 +358,17 @@ public class ExecutionTool {
     }
 
     OutputService outputService = env.getOutputService();
+    ModifiedFileSet modifiedOutputFiles = ModifiedFileSet.EVERYTHING_MODIFIED;
     if (outputService != null) {
-      outputService.startBuild(buildId);
+      modifiedOutputFiles = outputService.startBuild(buildId);
     } else {
       startLocalOutputBuild(); // TODO(bazel-team): this could be just another OutputService
     }
 
     ActionCache actionCache = getActionCache();
     SkyframeExecutor skyframeExecutor = env.getSkyframeExecutor();
-    Builder builder = createBuilder(request, executor, actionCache, skyframeExecutor);
+    Builder builder =
+            createBuilder(request, executor, actionCache, skyframeExecutor, modifiedOutputFiles);
 
     //
     // Execution proper.  All statements below are logically nested in
@@ -662,7 +665,8 @@ public class ExecutionTool {
   private Builder createBuilder(BuildRequest request,
       Executor executor,
       ActionCache actionCache,
-      SkyframeExecutor skyframeExecutor) {
+      SkyframeExecutor skyframeExecutor,
+      ModifiedFileSet modifiedOutputFiles) {
     BuildRequest.BuildRequestOptions options = request.getBuildOptions();
     boolean verboseExplanations = options.verboseExplanations;
     boolean keepGoing = request.getViewOptions().keepGoing;
@@ -682,8 +686,9 @@ public class ExecutionTool {
     return new SkyframeBuilder(skyframeExecutor,
         new ActionCacheChecker(actionCache, env.getView().getArtifactFactory(), executionFilter,
             verboseExplanations),
-        keepGoing, actualJobs, options.checkOutputFiles, fileCache,
-        request.getBuildOptions().progressReportInterval);
+        keepGoing, actualJobs,
+        options.checkOutputFiles ? modifiedOutputFiles : ModifiedFileSet.NOTHING_MODIFIED,
+        options.finalizeActions, fileCache, request.getBuildOptions().progressReportInterval);
   }
 
   private void configureResourceManager(BuildRequest request) {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
index c7142e39dd..8bd0da31a4 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -51,6 +51,7 @@ import com.google.devtools.build.lib.skyframe.TargetCompletionValue;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.BlazeClock;
 import com.google.devtools.build.lib.util.LoggingUtil;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.ErrorInfo;
 import com.google.devtools.build.skyframe.EvaluationProgressReceiver;
@@ -79,20 +80,23 @@ public class SkyframeBuilder implements Builder {
   private final SkyframeExecutor skyframeExecutor;
   private final boolean keepGoing;
   private final int numJobs;
-  private final boolean checkOutputFiles;
+  private final boolean finalizeActionsToOutputService;
+  private final ModifiedFileSet modifiedOutputFiles;
   private final ActionInputFileCache fileCache;
   private final ActionCacheChecker actionCacheChecker;
   private final int progressReportInterval;
 
   @VisibleForTesting
   public SkyframeBuilder(SkyframeExecutor skyframeExecutor, ActionCacheChecker actionCacheChecker,
-      boolean keepGoing, int numJobs, boolean checkOutputFiles,
-      ActionInputFileCache fileCache, int progressReportInterval) {
+      boolean keepGoing, int numJobs, ModifiedFileSet modifiedOutputFiles,
+      boolean finalizeActionsToOutputService, ActionInputFileCache fileCache,
+      int progressReportInterval) {
     this.skyframeExecutor = skyframeExecutor;
     this.actionCacheChecker = actionCacheChecker;
     this.keepGoing = keepGoing;
     this.numJobs = numJobs;
-    this.checkOutputFiles = checkOutputFiles;
+    this.finalizeActionsToOutputService = finalizeActionsToOutputService;
+    this.modifiedOutputFiles = modifiedOutputFiles;
     this.fileCache = fileCache;
     this.progressReportInterval = progressReportInterval;
   }
@@ -110,7 +114,7 @@ public class SkyframeBuilder implements Builder {
       boolean explain,
       @Nullable Range<Long> lastExecutionTimeRange)
       throws BuildFailedException, AbruptExitException, TestExecException, InterruptedException {
-    skyframeExecutor.prepareExecution(checkOutputFiles, lastExecutionTimeRange);
+    skyframeExecutor.prepareExecution(modifiedOutputFiles, lastExecutionTimeRange);
     skyframeExecutor.setFileCache(fileCache);
     // Note that executionProgressReceiver accesses builtTargets concurrently (after wrapping in a
     // synchronized collection), so unsynchronized access to this variable is unsafe while it runs.
@@ -147,6 +151,7 @@ public class SkyframeBuilder implements Builder {
               /*exclusiveTesting=*/ false,
               keepGoing,
               explain,
+              finalizeActionsToOutputService,
               numJobs,
               actionCacheChecker,
               executionProgressReceiver);
@@ -181,6 +186,7 @@ public class SkyframeBuilder implements Builder {
                 true,
                 keepGoing,
                 explain,
+                finalizeActionsToOutputService,
                 numJobs,
                 actionCacheChecker,
                 null);
diff --git a/src/main/java/com/google/devtools/build/lib/exec/OutputService.java b/src/main/java/com/google/devtools/build/lib/exec/OutputService.java
index 2e3d883083..2a48b5cb74 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/OutputService.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/OutputService.java
@@ -14,11 +14,15 @@
 
 package com.google.devtools.build.lib.exec;
 
+import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.BuildFailedException;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.vfs.BatchStat;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -53,10 +57,11 @@ public interface OutputService {
    * Start the build.
    *
    * @param buildId the UUID build identifier
+   * @return a ModifiedFileSet of changed output files.
    * @throws BuildFailedException if build preparation failed
    * @throws InterruptedException
    */
-  void startBuild(UUID buildId)
+  ModifiedFileSet startBuild(UUID buildId)
       throws BuildFailedException, AbruptExitException, InterruptedException;
 
   /**
@@ -67,6 +72,10 @@ public interface OutputService {
    */
   void finalizeBuild(boolean buildSuccessful) throws BuildFailedException, AbruptExitException;
 
+  /** Notify the output service of a completed action. */
+  void finalizeAction(Action action, MetadataHandler metadataHandler)
+      throws IOException, EnvironmentalExecException;
+
   /**
    * Stages the given tool from the package path, possibly copying it to local disk.
    *
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 2401940881..665c1b3944 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
@@ -353,9 +353,7 @@ public final class CommandEnvironment {
     }
 
     SkyframeExecutor skyframeExecutor = getSkyframeExecutor();
-    skyframeExecutor.setBatchStatter(outputService == null
-        ? null
-        : outputService.getBatchStatter());
+    skyframeExecutor.setOutputService(outputService);
 
     this.outputFileSystem = determineOutputFileSystem();
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 2415474fb1..e6bf2f5738 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -42,6 +42,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.MiddlemanExpander;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
 import com.google.devtools.build.lib.actions.CachedActionEvent;
+import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.MapBasedActionGraph;
 import com.google.devtools.build.lib.actions.MutableActionGraph;
@@ -59,6 +60,7 @@ import com.google.devtools.build.lib.concurrent.Sharder;
 import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.exec.OutputService;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.util.Pair;
@@ -130,6 +132,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
   private ProgressSupplier progressSupplier;
   private ActionCompletedReceiver completionReceiver;
   private final AtomicReference<ActionExecutionStatusReporter> statusReporterRef;
+  private OutputService outputService;
 
   SkyframeActionExecutor(ResourceManager resourceManager,
       AtomicReference<EventBus> eventBus,
@@ -331,7 +334,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
   }
 
   void prepareForExecution(Reporter reporter, Executor executor, boolean keepGoing,
-      boolean explain, ActionCacheChecker actionCacheChecker) {
+      boolean explain, ActionCacheChecker actionCacheChecker, OutputService outputService) {
     this.reporter = Preconditions.checkNotNull(reporter);
     this.executorEngine = Preconditions.checkNotNull(executor);
 
@@ -342,6 +345,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     this.actionCacheChecker = Preconditions.checkNotNull(actionCacheChecker);
     // Don't cache possibly stale data from the last build.
     this.explain = explain;
+    this.outputService = outputService;
   }
 
   public void setActionLogBufferPathGenerator(
@@ -354,6 +358,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     // This transitively holds a bunch of heavy objects, so it's important to clear it at the
     // end of a build.
     this.executorEngine = null;
+    this.outputService = null;
   }
 
   boolean probeActionExecution(Action action) {
@@ -810,6 +815,15 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
       } finally {
         profiler.completeTask(ProfilerTask.ACTION_COMPLETE);
       }
+
+      if (outputService != null) {
+        try {
+          outputService.finalizeAction(action, metadataHandler);
+        } catch (EnvironmentalExecException | IOException e) {
+          reportError("unable to finalize action: " + e.getMessage(), e, action, fileOutErr);
+        }
+      }
+
       reportActionExecution(action, null, fileOutErr);
     } catch (ActionExecutionException actionException) {
       // Success in execution but failure in completion.
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 b3e5b8e6f7..8b78370090 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
@@ -76,6 +76,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.exec.OutputService;
 import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
@@ -173,7 +174,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   private final WorkspaceStatusAction.Factory workspaceStatusActionFactory;
   private final BlazeDirectories directories;
   @Nullable
-  private BatchStat batchStatter;
+  private OutputService outputService;
 
   // TODO(bazel-team): Figure out how to handle value builders that block internally. Blocking
   // operations may need to be handled in another (bigger?) thread pool. Also, we should detect
@@ -447,8 +448,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   public abstract void dumpPackages(PrintStream out);
 
-  public void setBatchStatter(@Nullable BatchStat batchStatter) {
-    this.batchStatter = batchStatter;
+  public void setOutputService(OutputService outputService) {
+    this.outputService = outputService;
   }
 
   /**
@@ -1043,6 +1044,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       boolean exclusiveTesting,
       boolean keepGoing,
       boolean explain,
+      boolean finalizeActionsToOutputService,
       int numJobs,
       ActionCacheChecker actionCacheChecker,
       @Nullable EvaluationProgressReceiver executionProgressReceiver)
@@ -1051,7 +1053,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     Preconditions.checkState(actionLogBufferPathGenerator != null);
 
     skyframeActionExecutor.prepareForExecution(
-        reporter, executor, keepGoing, explain, actionCacheChecker);
+        reporter, executor, keepGoing, explain, actionCacheChecker,
+        finalizeActionsToOutputService ? outputService : null);
 
     resourceManager.resetResourceUsage();
     try {
@@ -1077,7 +1080,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   @VisibleForTesting
   public void prepareBuildingForTestingOnly(Reporter reporter, Executor executor, boolean keepGoing,
       boolean explain, ActionCacheChecker checker) {
-    skyframeActionExecutor.prepareForExecution(reporter, executor, keepGoing, explain, checker);
+    skyframeActionExecutor.prepareForExecution(reporter, executor, keepGoing, explain, checker,
+        outputService);
   }
 
   EvaluationResult<TargetPatternValue> targetPatterns(Iterable<SkyKey> patternSkyKeys,
@@ -1637,14 +1641,15 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     this.statusReporterRef.set(statusReporter);
   }
 
-  public void prepareExecution(boolean checkOutputFiles,
+  public void prepareExecution(ModifiedFileSet modifiedOutputFiles,
       @Nullable Range<Long> lastExecutionTimeRange) throws AbruptExitException,
       InterruptedException {
     maybeInjectEmbeddedArtifacts();
 
-    if (checkOutputFiles) {
+    if (modifiedOutputFiles != ModifiedFileSet.NOTHING_MODIFIED) {
       // Detect external modifications in the output tree.
       FilesystemValueChecker fsvc = new FilesystemValueChecker(tsgm, lastExecutionTimeRange);
+      BatchStat batchStatter = outputService == null ? null : outputService.getBatchStatter();
       invalidateDirtyActions(fsvc.getDirtyActionValues(memoizingEvaluator.getValues(),
           batchStatter));
       modifiedFiles += fsvc.getNumberOfModifiedOutputFiles();