diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java index f2e6ed013eda22..54ed909d7177e5 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawn.java +++ b/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 mappings) throws IOException { + PathFragment path, Path symlink, Map 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 symlinks = null; + Map symlinks = null; for (Map.Entry 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 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 finalSymlinks = symlinks; + process.createSandbox( + sandboxName, + (mapper) -> { + mapper.map(rootFragment, scratchDir.asFragment(), true); + + for (Map.Entry 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 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