[7.1.0] Improve performance of --reuse_sandbox_directories

src/main/java/com/google/devtools/build/lib/exec/TreeDeleter.java

@@ -27,6 +27,8 @@ public interface TreeDeleter {
    * not be reported even if they happen. For example, if deletions are asynchronous, there is no
    * way to capture their errors.
    *
+   * <p>For asynchronous deleters the directory is moved before being deleted.
+   *
    * @param path the tree to be deleted
    * @throws IOException if there are problems deleting the tree
    */

src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java

@@ -20,6 +20,8 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.exec.TreeDeleter;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -45,7 +47,7 @@ public abstract class AbstractContainerizingSandboxedSpawn implements SandboxedS
   final SandboxInputs inputs;
   final SandboxOutputs outputs;
   private final Set<Path> writableDirs;
-  private final TreeDeleter treeDeleter;
+  protected final TreeDeleter treeDeleter;
   @Nullable private final Path sandboxDebugPath;
   @Nullable private final Path statisticsPath;
   private final String mnemonic;
@@ -118,20 +120,28 @@ public void createFileSystem() throws IOException, InterruptedException {
             .filter(p -> p.startsWith(sandboxExecRoot))
             .map(p -> p.relativeTo(sandboxExecRoot))
             .collect(Collectors.toSet());
-    SandboxHelpers.populateInputsAndDirsToCreate(
-        writableSandboxDirs,
-        inputsToCreate,
-        dirsToCreate,
-        Iterables.concat(
-            ImmutableSet.of(), inputs.getFiles().keySet(), inputs.getSymlinks().keySet()),
-        outputs);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.populateInputsAndDirsToCreate")) {
+      SandboxHelpers.populateInputsAndDirsToCreate(
+          writableSandboxDirs,
+          inputsToCreate,
+          dirsToCreate,
+          Iterables.concat(
+              ImmutableSet.of(), inputs.getFiles().keySet(), inputs.getSymlinks().keySet()),
+          outputs);
+    }
 
-    // Allow subclasses to filter out inputs and dirs that don't need to be created.
-    filterInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.filterInputsAndDirsToCreate")) {
+      // Allow subclasses to filter out inputs and dirs that don't need to be created.
+      filterInputsAndDirsToCreate(inputsToCreate, dirsToCreate);
+    }
 
     // Finally create what needs creating.
-    SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict=*/ true);
-    createInputs(inputsToCreate, inputs);
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.createDirectories")) {
+      SandboxHelpers.createDirectories(dirsToCreate, sandboxExecRoot, /* strict= */ true);
+    }
+    try (SilentCloseable c = Profiler.instance().profile("sandbox.createInputs")) {
+      createInputs(inputsToCreate, inputs);
+    }
   }
 
   protected void filterInputsAndDirsToCreate(

src/main/java/com/google/devtools/build/lib/sandbox/AsynchronousTreeDeleter.java

@@ -26,6 +26,7 @@
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import javax.annotation.Nullable;
 
 /**
@@ -36,14 +37,21 @@
  * precious resources that could otherwise be used for the build itself. But when the build is
  * finished, this number should be raised to quickly go through any pending deletions.
  */
-class AsynchronousTreeDeleter implements TreeDeleter {
+public class AsynchronousTreeDeleter implements TreeDeleter {
+
+  public static final String MOVED_TRASH_DIR = "_moved_trash_dir";
+
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
+  private final AtomicInteger trashCount = new AtomicInteger(0);
+
   /** Thread pool used to execute asynchronous tree deletions; null in synchronous mode. */
   @Nullable private ThreadPoolExecutor service;
 
+  private final Path trashBase;
+
   /** Constructs a new asynchronous tree deleter backed by just one thread. */
-  AsynchronousTreeDeleter() {
+  public AsynchronousTreeDeleter(Path trashBase) {
     logger.atInfo().log("Starting async tree deletion pool with 1 thread");
 
     ThreadFactory threadFactory =
@@ -56,6 +64,8 @@ class AsynchronousTreeDeleter implements TreeDeleter {
     service =
         new ThreadPoolExecutor(
             1, 1, 0L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), threadFactory);
+
+    this.trashBase = trashBase;
   }
 
   /**
@@ -74,12 +84,17 @@ void setThreads(int threads) {
   }
 
   @Override
-  public void deleteTree(Path path) {
+  public void deleteTree(Path path) throws IOException {
+    if (!trashBase.exists()) {
+      trashBase.createDirectory();
+    }
+    Path trashPath = trashBase.getRelative(Integer.toString(trashCount.getAndIncrement()));
+    path.renameTo(trashPath);
     checkNotNull(service, "Cannot call deleteTree after shutdown")
         .execute(
             () -> {
               try {
-                path.deleteTree();
+                trashPath.deleteTree();
               } catch (IOException e) {
                 logger.atWarning().withCause(e).log(
                     "Failed to delete tree %s asynchronously", path);

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

@@ -20,13 +20,15 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/protobuf:failure_details_java_proto",
         "//third_party:auto_value",
         "//third_party:flogger",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -73,6 +75,7 @@ java_library(
         ":sandbox_helpers",
         ":sandbox_options",
         "//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/util:command",
         "//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtil.java

@@ -111,7 +111,7 @@ public static Path getInaccessibleHelperFile(Path sandboxBase) throws IOExceptio
   public static Path getInaccessibleHelperDir(Path sandboxBase) throws IOException {
     // The order of the permissions settings calls matters, see
     // https://github.com/bazelbuild/bazel/issues/16364
-    Path inaccessibleHelperDir = sandboxBase.getRelative("inaccessibleHelperDir");
+    Path inaccessibleHelperDir = sandboxBase.getRelative(SandboxHelpers.INACCESSIBLE_HELPER_DIR);
     inaccessibleHelperDir.createDirectory();
     inaccessibleHelperDir.setExecutable(false);
     inaccessibleHelperDir.setWritable(false);

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.Sandbox;
 import com.google.devtools.build.lib.server.FailureDetails.Sandbox.Code;
@@ -64,18 +65,23 @@
 import java.util.TreeMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
 
 /**
  * Helper methods that are shared by the different sandboxing strategies.
  *
  * <p>All sandboxed strategies within a build should share the same instance of this object.
  */
 public final class SandboxHelpers {
+
+  public static final String INACCESSIBLE_HELPER_DIR = "inaccessibleHelperDir";
+
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   private static final AtomicBoolean warnedAboutMovesBeingCopies = new AtomicBoolean(false);
 
   private static final AtomicInteger tempFileUniquifierForVirtualInputWrites = new AtomicInteger();
+
   /**
    * Moves all given outputs from a root to another.
    *
@@ -135,6 +141,27 @@ public static void cleanExisting(
       Set<PathFragment> dirsToCreate,
       Path workDir)
       throws IOException, InterruptedException {
+    cleanExisting(root, inputs, inputsToCreate, dirsToCreate, workDir, /* treeDeleter= */ null);
+  }
+
+  public static void cleanExisting(
+      Path root,
+      SandboxInputs inputs,
+      Set<PathFragment> inputsToCreate,
+      Set<PathFragment> dirsToCreate,
+      Path workDir,
+      @Nullable TreeDeleter treeDeleter)
+      throws IOException, InterruptedException {
+    Path inaccessibleHelperDir = workDir.getRelative(INACCESSIBLE_HELPER_DIR);
+    // Setting the permissions is necessary when we are using an asynchronous tree deleter in order
+    // to move the directory first. This is not necessary for a synchronous tree deleter because the
+    // permissions are only needed in the parent directory in that case.
+    if (inaccessibleHelperDir.exists()) {
+      inaccessibleHelperDir.setExecutable(true);
+      inaccessibleHelperDir.setWritable(true);
+      inaccessibleHelperDir.setReadable(true);
+    }
+
     // To avoid excessive scanning of dirsToCreate for prefix dirs, we prepopulate this set of
     // prefixes.
     Set<PathFragment> prefixDirs = new HashSet<>();
@@ -148,7 +175,7 @@ public static void cleanExisting(
         parent = parent.getParentDirectory();
       }
     }
-    cleanRecursively(root, inputs, inputsToCreate, dirsToCreate, workDir, prefixDirs);
+    cleanRecursively(root, inputs, inputsToCreate, dirsToCreate, workDir, prefixDirs, treeDeleter);
   }
 
   /**
@@ -161,7 +188,8 @@ private static void cleanRecursively(
       Set<PathFragment> inputsToCreate,
       Set<PathFragment> dirsToCreate,
       Path workDir,
-      Set<PathFragment> prefixDirs)
+      Set<PathFragment> prefixDirs,
+      @Nullable TreeDeleter treeDeleter)
       throws IOException, InterruptedException {
     Path execroot = workDir.getParentDirectory();
     for (Dirent dirent : root.readdir(Symlinks.NOFOLLOW)) {
@@ -188,17 +216,28 @@ private static void cleanRecursively(
             && absPath.readSymbolicLink().equals(destination.get())) {
           inputsToCreate.remove(pathRelativeToWorkDir);
         } else if (absPath.isDirectory()) {
-          absPath.deleteTree();
+          if (treeDeleter == null) {
+            // TODO(bazel-team): Use async tree deleter for workers too
+            absPath.deleteTree();
+          } else {
+            treeDeleter.deleteTree(absPath);
+          }
         } else {
           absPath.delete();
         }
       } else if (DIRECTORY.equals(dirent.getType())) {
         if (dirsToCreate.contains(pathRelativeToWorkDir)
             || prefixDirs.contains(pathRelativeToWorkDir)) {
-          cleanRecursively(absPath, inputs, inputsToCreate, dirsToCreate, workDir, prefixDirs);
+          cleanRecursively(
+              absPath, inputs, inputsToCreate, dirsToCreate, workDir, prefixDirs, treeDeleter);
           dirsToCreate.remove(pathRelativeToWorkDir);
         } else {
-          absPath.deleteTree();
+          if (treeDeleter == null) {
+            // TODO(bazel-team): Use async tree deleter for workers too
+            absPath.deleteTree();
+          } else {
+            treeDeleter.deleteTree(absPath);
+          }
         }
       } else if (!inputsToCreate.contains(pathRelativeToWorkDir)) {
         absPath.delete();