From 60d6f787eabea8f689e64c78723736dd47aa460d Mon Sep 17 00:00:00 2001 From: djasper Date: Mon, 25 Jan 2021 05:33:54 -0800 Subject: [PATCH] Use PerBuildSyscallCache when getting the file size to decide on remote include extraction. Especially in clean builds, we'll already have stat'ed all these files in FileStateFunction. Also change PerBuildSyscallCache to share results independent of whether following symlinks (FOLLOW) or not following them (NOFOLLOW). The vast majority of files are not symlinks and so both functions return the same result. RELNOTES: None. PiperOrigin-RevId: 353627146 --- .../includescanning/IncludeScanningModule.java | 3 ++- .../includescanning/SpawnIncludeScanner.java | 17 +++++++++++++---- .../lib/skyframe/PerBuildSyscallCache.java | 11 ++++++++++- .../build/lib/skyframe/SkyframeExecutor.java | 4 ++++ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java index 14a9122fa9c3d6..97a30124fe89a4 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java @@ -241,7 +241,8 @@ public IncludeScannerLifecycleManager( spawnScannerSupplier.set( new SpawnIncludeScanner( env.getExecRoot(), - options.experimentalRemoteExtractionThreshold)); + options.experimentalRemoteExtractionThreshold, + env.getSkyframeExecutor().getSyscalls())); this.spawnScannerSupplier = spawnScannerSupplier; env.getEventBus().register(this); } diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java index 605ecfd69cae36..bcde047143966f 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SpawnIncludeScanner.java @@ -51,14 +51,18 @@ import com.google.devtools.build.lib.includescanning.IncludeParser.GrepIncludesFileType; import com.google.devtools.build.lib.includescanning.IncludeParser.Inclusion; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.IORuntimeException; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls; import java.io.IOException; import java.io.InputStream; import java.util.Collection; import java.util.List; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; /** @@ -73,11 +77,14 @@ public class SpawnIncludeScanner { private final Path execRoot; private boolean inMemoryOutput; private final int remoteExtractionThreshold; + private final AtomicReference syscallCache; /** Constructs a new SpawnIncludeScanner. */ - public SpawnIncludeScanner(Path execRoot, int remoteExtractionThreshold) { + public SpawnIncludeScanner( + Path execRoot, int remoteExtractionThreshold, AtomicReference syscallCache) { this.execRoot = execRoot; this.remoteExtractionThreshold = remoteExtractionThreshold; + this.syscallCache = syscallCache; } public void setInMemoryOutput(boolean inMemoryOutput) { @@ -116,9 +123,11 @@ public boolean shouldParseRemotely(Artifact file, ActionExecutionContext ctx) th // Output files are generally not locally available should be scanned remotely to avoid the // bandwidth and disk space penalty of bringing them across. Also, enable include scanning // remotely when the file size exceeds a certain size. - return remoteExtractionThreshold == 0 - || !file.isSourceArtifact() - || ctx.getPathResolver().toPath(file).getFileSize() > remoteExtractionThreshold; + if (remoteExtractionThreshold == 0 || !file.isSourceArtifact()) { + return true; + } + FileStatus status = syscallCache.get().statIfFound(file.getPath(), Symlinks.FOLLOW); + return status == null || status.getSize() > remoteExtractionThreshold; } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java index 7afb6520353de0..55504e7ea200f5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java @@ -106,11 +106,20 @@ public Collection readdir(Path path) throws IOException { @Override public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException { - Object result = statCache.getUnchecked(Pair.of(path, symlinks)); + // Try to load a Symlinks.NOFOLLOW result first. Symlinks are rare and this enables sharing the + // cache for all non-symlink paths. + Object result = statCache.getUnchecked(Pair.of(path, Symlinks.NOFOLLOW)); if (result instanceof IOException) { throw (IOException) result; } FileStatus status = (FileStatus) result; + if (status != NO_STATUS && symlinks == Symlinks.FOLLOW && status.isSymbolicLink()) { + result = statCache.getUnchecked(Pair.of(path, Symlinks.FOLLOW)); + if (result instanceof IOException) { + throw (IOException) result; + } + status = (FileStatus) result; + } return (status == NO_STATUS) ? null : status; } 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 0b76d94fc11d0b..f4b2bf0331ee05 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 @@ -672,6 +672,10 @@ protected final PerBuildSyscallCache getPerBuildSyscallCache(int concurrencyLeve return perBuildSyscallCache; } + public AtomicReference getSyscalls() { + return syscalls; + } + @ThreadCompatible public void setActive(boolean active) { this.active = active;