Avoid file operations in the sandbox creation critical path.

This shuffles the code to perform computations that require disk access
outside of the code snippet that holds the sandboxfs log.

Additionally, this comes with another minor optimization to check the
value of the sandboxfsMapSymlinkTargets boolean flag first before doing
disk accesses.

RELNOTES: None.
PiperOrigin-RevId: 298911970

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

@@ -15,12 +15,12 @@
 package com.google.devtools.build.lib.sandbox;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.base.Joiner;
 import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs;
-import com.google.devtools.build.lib.sandbox.SandboxfsProcess.SandboxMapper;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -183,9 +183,7 @@ public void createFileSystem() throws IOException {
       sandboxScratchDir.getRelative(dir).createDirectoryAndParents();
     }
 
-    process.createSandbox(
-        sandboxName,
-        (mapper) -> createMappings(mapper, sandboxScratchDir, inputs, mapSymlinkTargets));
+    createSandbox(process, sandboxName, sandboxScratchDir, inputs, mapSymlinkTargets);
     sandboxIsMapped = true;
   }
 
@@ -253,7 +251,8 @@ public void delete() {
    * @throws IOException if we fail to resolve symbolic links
    */
   private static void computeSymlinkMappings(
-      PathFragment path, Path symlink, Map<PathFragment, Path> mappings) throws IOException {
+      PathFragment path, Path symlink, Map<PathFragment, PathFragment> mappings)
+      throws IOException {
     for (; ; ) {
       PathFragment symlinkTarget = symlink.readSymbolicLinkUnchecked();
       if (!symlinkTarget.isAbsolute()) {
@@ -268,7 +267,7 @@ private static void computeSymlinkMappings(
           throw new IOException("Cannot resolve " + symlinkTarget + " relative to " + symlink);
         }
         Path value = valueParent.getRelative(symlinkTarget);
-        mappings.put(key, value);
+        mappings.put(key, value.asFragment());
 
         if (value.isSymbolicLink()) {
           path = key;
@@ -283,6 +282,8 @@ private static void computeSymlinkMappings(
   /**
    * Creates a new set of mappings to sandbox the given inputs.
    *
+   * @param process the sandboxfs instance on which to create the sandbox
+   * @param sandboxName the name of the sandbox to pass to sandboxfs
    * @param scratchDir writable used as the target for all writable mappings
    * @param inputs collection of paths to expose within the sandbox as read-only mappings, given as
    *     a map of mapped path to target path. The target path may be null, in which case an empty
@@ -291,57 +292,72 @@ private static void computeSymlinkMappings(
    * @return the collection of mappings to use for reconfiguration
    * @throws IOException if we fail to resolve symbolic links
    */
-  // TODO(jmmv): This function runs holding the sandboxfs lock (note that we are passed in a
-  // "mapper" object, so it should not do any I/O. But it does a lot. Should factor it out and
-  // measure the impact.
-  private static void createMappings(
-      SandboxMapper mapper,
+  private static void createSandbox(
+      SandboxfsProcess process,
+      String sandboxName,
       Path scratchDir,
       SandboxInputs inputs,
       boolean sandboxfsMapSymlinkTargets)
       throws IOException {
-    mapper.map(rootFragment, scratchDir.asFragment(), true);
-
     // Path to the empty file used as the target of mappings that don't provide one.  This is
     // lazily created and initialized only when we need such a mapping.  It's safe to share the
     // same empty file across all such mappings because this file is exposed as read-only.
     //
     // We cannot use /dev/null, as we used to do in the past, because exposing devices via a
     // FUSE file system (which sandboxfs is) requires root privileges.
-    Path emptyFile = null;
+    PathFragment emptyFile = null;
 
     // Collection of extra mappings needed to represent the targets of relative symlinks. Lazily
     // created once we encounter the first symlink in the list of inputs.
-    Map<PathFragment, Path> symlinks = null;
+    Map<PathFragment, PathFragment> symlinks = null;
 
     for (Map.Entry<PathFragment, Path> entry : inputs.getFiles().entrySet()) {
-      PathFragment target;
       if (entry.getValue() == null) {
         if (emptyFile == null) {
-          emptyFile = scratchDir.getRelative("empty");
-          FileSystemUtils.createEmptyFile(emptyFile);
+          Path emptyFilePath = scratchDir.getRelative("empty");
+          FileSystemUtils.createEmptyFile(emptyFilePath);
+          emptyFile = emptyFilePath.asFragment();
         }
-        target = emptyFile.asFragment();
       } else {
-        if (entry.getValue().isSymbolicLink() && sandboxfsMapSymlinkTargets) {
+        if (sandboxfsMapSymlinkTargets && entry.getValue().isSymbolicLink()) {
           if (symlinks == null) {
             symlinks = new HashMap<>();
           }
           computeSymlinkMappings(entry.getKey(), entry.getValue(), symlinks);
         }
-        target = entry.getValue().asFragment();
       }
-      mapper.map(rootFragment.getRelative(entry.getKey()), target, false);
     }
 
-    if (symlinks != null) {
-      for (Map.Entry<PathFragment, Path> entry : symlinks.entrySet()) {
-        if (!inputs.getFiles().containsKey(entry.getKey())) {
-          mapper.map(
-              rootFragment.getRelative(entry.getKey()), entry.getValue().asFragment(), false);
-        }
-      }
-    }
+    // IMPORTANT: Keep the code in the lambda passed to createSandbox() free from any operations
+    // that may block. This includes doing any kind of I/O. We used to include the loop above in
+    // this call and doing so cost 2-3% of the total build time measured on an iOS build with many
+    // actions that have thousands of inputs each.
+    @Nullable final PathFragment finalEmptyFile = emptyFile;
+    @Nullable final Map<PathFragment, PathFragment> finalSymlinks = symlinks;
+    process.createSandbox(
+        sandboxName,
+        (mapper) -> {
+          mapper.map(rootFragment, scratchDir.asFragment(), true);
+
+          for (Map.Entry<PathFragment, Path> entry : inputs.getFiles().entrySet()) {
+            PathFragment target;
+            if (entry.getValue() == null) {
+              checkNotNull(finalEmptyFile, "Must have been initialized above by matching logic");
+              target = finalEmptyFile;
+            } else {
+              target = entry.getValue().asFragment();
+            }
+            mapper.map(rootFragment.getRelative(entry.getKey()), target, false);
+          }
+
+          if (finalSymlinks != null) {
+            for (Map.Entry<PathFragment, PathFragment> entry : finalSymlinks.entrySet()) {
+              if (!inputs.getFiles().containsKey(entry.getKey())) {
+                mapper.map(rootFragment.getRelative(entry.getKey()), entry.getValue(), false);
+              }
+            }
+          }
+        });
 
     // sandboxfs probably doesn't support symlinks.
     // TODO(jmmv): This claim is simply not true. Figure out why this code snippet was added and