Replace UnixGlob's package glob prefetching functionality with an optional method in FileSystem. Custom FileSystem implementations can use this to provide their own implementation of glob prefetching. -- MOS_MIGRATED_REVID=140736304
diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java index 5259385..56df73c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java +++ b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java
@@ -138,16 +138,8 @@ Future<List<Path>> cached = globCache.get(Pair.of(pattern, excludeDirs)); if (cached == null) { if (maxDirectoriesToEagerlyVisit > -1 - && !globalStarted.getAndSet(true) - && !pattern.startsWith("**")) { - UnixGlob.forPath(packageDirectory) - .setMaxDirectoriesToEagerlyVisit(maxDirectoriesToEagerlyVisit) - .addPattern("**") - .setExcludeDirectories(true) - .setDirectoryFilter(childDirectoryPredicate) - .setThreadPool(globExecutor) - .setFilesystemCalls(syscalls) - .globAsync(true); + && !globalStarted.getAndSet(true)) { + packageDirectory.prefetchPackageAsync(maxDirectoriesToEagerlyVisit); } cached = safeGlobUnsorted(pattern, excludeDirs); setGlobPaths(pattern, excludeDirs, cached);
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index 4d93804..de1fe986 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
@@ -827,4 +827,12 @@ */ protected abstract void createFSDependentHardLink(Path linkPath, Path originalPath) throws IOException; + + /** + * Prefetch all directories and symlinks within the package + * rooted at "path". Enter at most "maxDirs" total directories. + * Specializations for high-latency remote filesystems may wish to + * implement this in order to warm the filesystem's internal caches. + */ + protected void prefetchPackageAsync(Path path, int maxDirs) { } }
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java index e039efe..cdc0a0a 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
@@ -1227,6 +1227,10 @@ fileSystem.chmod(this, mode); } + public void prefetchPackageAsync(int maxDirs) { + fileSystem.prefetchPackageAsync(this, maxDirs); + } + /** * Compare Paths of the same file system using their PathFragments. *
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java index 82b5e55..9446aa3 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java
@@ -44,7 +44,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Pattern; @@ -71,7 +70,7 @@ GlobVisitor visitor = (threadPool == null) ? new GlobVisitor(checkForInterruption) - : new GlobVisitor(threadPool, checkForInterruption, -1); + : new GlobVisitor(threadPool, checkForInterruption); return visitor.glob(base, patterns, excludeDirectories, dirPred, syscalls); } @@ -85,7 +84,7 @@ GlobVisitor visitor = (threadPool == null) ? new GlobVisitor(checkForInterruption) - : new GlobVisitor(threadPool, checkForInterruption, -1); + : new GlobVisitor(threadPool, checkForInterruption); visitor.glob(base, patterns, excludeDirectories, dirPred, syscalls); return visitor.getNumGlobTasksForTesting(); } @@ -97,10 +96,9 @@ Predicate<Path> dirPred, FilesystemCalls syscalls, boolean checkForInterruption, - ThreadPoolExecutor threadPool, - int maxDirectoriesToEagerlyVisit) { + ThreadPoolExecutor threadPool) { Preconditions.checkNotNull(threadPool, "%s %s", base, patterns); - return new GlobVisitor(threadPool, checkForInterruption, maxDirectoriesToEagerlyVisit) + return new GlobVisitor(threadPool, checkForInterruption) .globAsync(base, patterns, excludeDirectories, dirPred, syscalls); } @@ -309,7 +307,6 @@ private ThreadPoolExecutor threadPool; private AtomicReference<? extends FilesystemCalls> syscalls = new AtomicReference<>(DEFAULT_SYSCALLS); - private int maxDirectoriesToEagerlyVisit = -1; /** * Creates a glob builder with the given base path. @@ -390,11 +387,6 @@ return this; } - public Builder setMaxDirectoriesToEagerlyVisit(int maxDirectoriesToEagerlyVisit) { - this.maxDirectoriesToEagerlyVisit = maxDirectoriesToEagerlyVisit; - return this; - } - /** * Executes the glob. */ @@ -439,8 +431,7 @@ pathFilter, syscalls.get(), checkForInterrupt, - threadPool, - maxDirectoriesToEagerlyVisit); + threadPool); } } @@ -507,21 +498,17 @@ private final AtomicLong totalOps = new AtomicLong(0); private final AtomicLong pendingOps = new AtomicLong(0); private final AtomicReference<IOException> failure = new AtomicReference<>(); - private final int maxDirectoriesToEagerlyVisit; - private final AtomicInteger visitedDirectories = new AtomicInteger(0); private volatile boolean canceled = false; GlobVisitor( ThreadPoolExecutor executor, - boolean failFastOnInterrupt, - int maxDirectoriesToEagerlyVisit) { + boolean failFastOnInterrupt) { this.executor = executor; this.result = new GlobFuture(this, failFastOnInterrupt); - this.maxDirectoriesToEagerlyVisit = maxDirectoriesToEagerlyVisit; } GlobVisitor(boolean failFastOnInterrupt) { - this(null, failFastOnInterrupt, -1); + this(null, failFastOnInterrupt); } /** @@ -558,14 +545,6 @@ } /** - * Whether or not to store the results of this glob. If this glob is being done purely to warm - * the filesystem, we do not store the results, since it would take unnecessary memory. - */ - private boolean storeGlobResults() { - return maxDirectoriesToEagerlyVisit == -1; - } - - /** * Same as {@link #glob}, except does so asynchronously and returns a {@link Future} for the * result. */ @@ -780,7 +759,7 @@ } if (idx == context.patternParts.length) { // Base case. - if (storeGlobResults() && !(context.excludeDirectories && baseIsDir)) { + if (!(context.excludeDirectories && baseIsDir)) { results.add(base); } @@ -792,10 +771,6 @@ return; } - if (maxDirectoriesToEagerlyVisit > -1 - && visitedDirectories.incrementAndGet() > maxDirectoriesToEagerlyVisit) { - return; - } final String pattern = context.patternParts[idx]; // ** is special: it can match nothing at all. @@ -843,7 +818,7 @@ context.queueGlob(child, childIsDir, idx + 1); } else { // Instead of using an async call, just repeat the base case above. - if (storeGlobResults() && idx + 1 == context.patternParts.length) { + if (idx + 1 == context.patternParts.length) { results.add(child); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index 3c960df..58321cc 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; @@ -35,7 +36,6 @@ import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.testutil.ManualClock; -import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.BlazeClock; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Dirent; @@ -56,10 +56,8 @@ import java.io.IOException; import java.util.Collection; import java.util.Map; +import java.util.Set; import java.util.UUID; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; import org.junit.Test; import org.junit.runner.RunWith; @@ -760,255 +758,74 @@ } } - @Test - public void testGlobsHappenInParallel() throws Exception { - scratch.file( - "foo/BUILD", - "load('//foo:my_library.bzl', 'my_library')", - "[sh_library(name = x + '-matched') for x in glob(['bar/*'], exclude_directories = 0)]", - "cc_library(name = 'cc', srcs = glob(['cc/*']))", - "my_library(name = 'my', srcs = glob(['sh/*']))"); - scratch.file( - "foo/my_library.bzl", - "def my_library(name = None, srcs = []):", - " native.sh_library(name = name, srcs = srcs, deps = native.glob(['inner/*']))"); - scratch.file("foo/bar/1"); - Path barPath = scratch.file("foo/bar/2").getParentDirectory(); - Path ccPath = scratch.file("foo/cc/src.file").getParentDirectory(); - Path shPath = scratch.dir("foo/sh"); - Path innerPath = scratch.dir("foo/inner"); - PackageCacheOptions packageCacheOptions = Options.getDefaults(PackageCacheOptions.class); - packageCacheOptions.defaultVisibility = ConstantRuleVisibility.PUBLIC; - packageCacheOptions.showLoadingProgress = true; - packageCacheOptions.globbingThreads = 7; - packageCacheOptions.maxDirectoriesToEagerlyVisitInGlobbing = 10; - getSkyframeExecutor() - .preparePackageLoading( - new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)), - packageCacheOptions, - "", - UUID.randomUUID(), - ImmutableMap.<String, String>of(), - new TimestampGranularityMonitor(BlazeClock.instance())); - - SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); - final CountDownLatch allDirsRequested = new CountDownLatch(4); - Listener synchronizeListener = - new Listener() { - @Override - public Object accept(Path path, FileOp op, Order order) throws IOException { - if (op == FileOp.READDIR && order == Order.BEFORE) { - allDirsRequested.countDown(); - try { - assertThat( - allDirsRequested.await( - TestUtils.WAIT_TIMEOUT_MILLISECONDS, TimeUnit.MILLISECONDS)) - .isTrue(); - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } - } - return NO_RESULT_MARKER; - } - }; - fs.setCustomOverride(barPath, synchronizeListener); - fs.setCustomOverride(ccPath, synchronizeListener); - fs.setCustomOverride(shPath, synchronizeListener); - fs.setCustomOverride(innerPath, synchronizeListener); - PackageValue value = validPackage(skyKey); - assertFalse(value.getPackage().containsErrors()); - assertThat(value.getPackage().getTarget("bar/1-matched").getName()).isEqualTo("bar/1-matched"); - assertThat(value.getPackage().getTarget("cc/src.file")).isNotNull(); - assertThat( - (Iterable<?>) - value - .getPackage() - .getTarget("my") - .getAssociatedRule() - .getAttributeContainer() - .getAttr("srcs")) - .isEmpty(); - } - - @Test - public void testGlobsDontHappenInParallel() throws Exception { - scratch.file( - "foo/BUILD", - "load('//foo:my_library.bzl', 'my_library')", - "[sh_library(name = x + '-matched') for x in glob(['bar/*'], exclude_directories = 0)]", - "cc_library(name = 'cc', srcs = glob(['cc/*']))", - "my_library(name = 'my', srcs = glob(['sh/*']))"); - scratch.file( - "foo/my_library.bzl", - "def my_library(name = None, srcs = []):", - " native.sh_library(name = name, srcs = srcs, deps = native.glob(['inner/*']))"); - scratch.file("foo/bar/1"); - Path barPath = scratch.file("foo/bar/2").getParentDirectory(); - Path ccPath = scratch.file("foo/cc/src.file").getParentDirectory(); - Path shPath = scratch.dir("foo/sh"); - Path innerPath = scratch.dir("foo/inner"); - PackageCacheOptions packageCacheOptions = Options.getDefaults(PackageCacheOptions.class); - packageCacheOptions.defaultVisibility = ConstantRuleVisibility.PUBLIC; - packageCacheOptions.showLoadingProgress = true; - packageCacheOptions.globbingThreads = 7; - packageCacheOptions.maxDirectoriesToEagerlyVisitInGlobbing = -1; - getSkyframeExecutor() - .preparePackageLoading( - new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)), - packageCacheOptions, - "", - UUID.randomUUID(), - ImmutableMap.<String, String>of(), - new TimestampGranularityMonitor(BlazeClock.instance())); - - SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); - final AtomicBoolean atLeastOneUnfinishedRequest = new AtomicBoolean(false); - final CountDownLatch allDirsRequested = new CountDownLatch(4); - Listener synchronizeListener = - new Listener() { - @Override - public Object accept(Path path, FileOp op, Order order) throws IOException { - if (op == FileOp.READDIR && order == Order.BEFORE) { - allDirsRequested.countDown(); - try { - if (!allDirsRequested.await(1, TimeUnit.SECONDS)) { - atLeastOneUnfinishedRequest.set(true); - } - } catch (InterruptedException e) { - throw new IllegalStateException(e); - } - } - return NO_RESULT_MARKER; - } - }; - fs.setCustomOverride(barPath, synchronizeListener); - fs.setCustomOverride(ccPath, synchronizeListener); - fs.setCustomOverride(shPath, synchronizeListener); - fs.setCustomOverride(innerPath, synchronizeListener); - PackageValue value = validPackage(skyKey); - assertFalse(value.getPackage().containsErrors()); - assertThat(value.getPackage().getTarget("bar/1-matched").getName()).isEqualTo("bar/1-matched"); - assertThat(value.getPackage().getTarget("cc/src.file")).isNotNull(); - assertThat( - (Iterable<?>) - value - .getPackage() - .getTarget("my") - .getAssociatedRule() - .getAttributeContainer() - .getAttr("srcs")) - .isEmpty(); - assertThat(atLeastOneUnfinishedRequest.get()).isTrue(); - } - private static class CustomInMemoryFs extends InMemoryFileSystem { - private final Map<Path, Listener> customOverrides = Maps.newHashMap(); + private abstract static class FileStatusOrException { + abstract FileStatus get() throws IOException; + + private static class ExceptionImpl extends FileStatusOrException { + private final IOException exn; + + private ExceptionImpl(IOException exn) { + this.exn = exn; + } + + @Override + FileStatus get() throws IOException { + throw exn; + } + } + + private static class FileStatusImpl extends FileStatusOrException { + + @Nullable + private final FileStatus fileStatus; + + private FileStatusImpl(@Nullable FileStatus fileStatus) { + this.fileStatus = fileStatus; + } + + @Override + @Nullable + FileStatus get() { + return fileStatus; + } + } + } + + private Map<Path, FileStatusOrException> stubbedStats = Maps.newHashMap(); + private Set<Path> makeUnreadableAfterReaddir = Sets.newHashSet(); public CustomInMemoryFs(ManualClock manualClock) { super(manualClock); } - public void stubStat(final Path targetPath, @Nullable final FileStatus stubbedResult) { - setCustomOverride( - targetPath, - new Listener() { - @Override - public Object accept(Path path, FileOp op, Order order) { - if (targetPath.equals(path) && op == FileOp.STAT && order == Order.BEFORE) { - return stubbedResult; - } else { - return NO_RESULT_MARKER; - } - } - }); + public void stubStat(Path path, @Nullable FileStatus stubbedResult) { + stubbedStats.put(path, new FileStatusOrException.FileStatusImpl(stubbedResult)); } - public void stubStatError(final Path targetPath, final IOException stubbedResult) { - setCustomOverride( - targetPath, - new Listener() { - @Override - public Object accept(Path path, FileOp op, Order order) throws IOException { - if (targetPath.equals(path) && op == FileOp.STAT && order == Order.BEFORE) { - throw stubbedResult; - } else { - return NO_RESULT_MARKER; - } - } - }); - } - - void setCustomOverride(Path path, Listener listener) { - customOverrides.put(path, listener); + public void stubStatError(Path path, IOException stubbedResult) { + stubbedStats.put(path, new FileStatusOrException.ExceptionImpl(stubbedResult)); } @Override public FileStatus stat(Path path, boolean followSymlinks) throws IOException { - Listener listener = customOverrides.get(path); - if (listener != null) { - Object status = listener.accept(path, FileOp.STAT, Order.BEFORE); - if (status != NO_RESULT_MARKER) { - return (FileStatus) status; - } + if (stubbedStats.containsKey(path)) { + return stubbedStats.get(path).get(); } - FileStatus fileStatus = super.stat(path, followSymlinks); - if (listener != null) { - Object status = listener.accept(path, FileOp.STAT, Order.AFTER); - if (status != NO_RESULT_MARKER) { - return (FileStatus) status; - } - } - return fileStatus; + return super.stat(path, followSymlinks); } - public void scheduleMakeUnreadableAfterReaddir(final Path targetPath) { - setCustomOverride( - targetPath, - new Listener() { - @Override - public Object accept(Path path, FileOp op, Order order) throws IOException { - if (targetPath.equals(path) && op == FileOp.READDIR && order == Order.AFTER) { - targetPath.setReadable(false); - } - return NO_RESULT_MARKER; - } - }); + public void scheduleMakeUnreadableAfterReaddir(Path path) { + makeUnreadableAfterReaddir.add(path); } - @SuppressWarnings("unchecked") @Override public Collection<Dirent> readdir(Path path, boolean followSymlinks) throws IOException { - Listener listener = customOverrides.get(path); - if (listener != null) { - Object status = listener.accept(path, FileOp.READDIR, Order.BEFORE); - if (status != NO_RESULT_MARKER) { - return (Collection<Dirent>) status; - } - } Collection<Dirent> result = super.readdir(path, followSymlinks); - if (listener != null) { - Object status = listener.accept(path, FileOp.READDIR, Order.AFTER); - if (status != NO_RESULT_MARKER) { - return (Collection<Dirent>) status; - } + if (makeUnreadableAfterReaddir.contains(path)) { + path.setReadable(false); } return result; } } - - private static final Object NO_RESULT_MARKER = new Object(); - - private enum Order { - BEFORE, - AFTER - } - - private enum FileOp { - STAT, - READDIR - } - - private interface Listener { - Object accept(Path path, FileOp op, Order order) throws IOException; - } }