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;
- }
}
