Let DirectoryListingStateFunction re-use the PerBuildSyscallCache. The listing
function performs a readdir() that we will likely have already executed earlier
for globbing.
RELNOTES: None.
PiperOrigin-RevId: 233908833
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
index e9545b2..49a6aaa 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
@@ -14,10 +14,13 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.Symlinks;
+import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import java.io.IOException;
+import java.util.concurrent.atomic.AtomicReference;
/**
* A {@link SkyFunction} for {@link DirectoryListingStateValue}s.
@@ -29,8 +32,17 @@
private final ExternalFilesHelper externalFilesHelper;
- public DirectoryListingStateFunction(ExternalFilesHelper externalFilesHelper) {
+ /**
+ * A file-system abstraction to use. This can e.g. be a {@link PerBuildSyscallCache} which helps
+ * re-use the results of expensive readdir() operations, that are likely already executed for
+ * evaluating globs.
+ */
+ private final AtomicReference<FilesystemCalls> syscallCache;
+
+ public DirectoryListingStateFunction(
+ ExternalFilesHelper externalFilesHelper, AtomicReference<FilesystemCalls> syscallCache) {
this.externalFilesHelper = externalFilesHelper;
+ this.syscallCache = syscallCache;
}
@Override
@@ -43,7 +55,8 @@
if (env.valuesMissing()) {
return null;
}
- return DirectoryListingStateValue.create(dirRootedPath);
+ return DirectoryListingStateValue.create(
+ syscallCache.get().readdir(dirRootedPath.asPath(), Symlinks.NOFOLLOW));
} catch (ExternalFilesHelper.NonexistentImmutableExternalFileException e) {
// DirectoryListingStateValue.key assumes the path exists. This exception here is therefore
// indicative of a programming bug.
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 595843b..5593383 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
@@ -601,7 +601,7 @@
}
protected SkyFunction newDirectoryListingStateFunction() {
- return new DirectoryListingStateFunction(externalFilesHelper);
+ return new DirectoryListingStateFunction(externalFilesHelper, syscalls);
}
protected SkyFunction newGlobFunction() {
@@ -2120,6 +2120,11 @@
invalidateFilesUnderPathForTestingImpl(eventHandler, modifiedFileSet, pathEntry);
}
+ @VisibleForTesting
+ public final void turnOffSyscallCacheForTesting() {
+ syscalls.set(UnixGlob.DEFAULT_SYSCALLS);
+ }
+
protected abstract void invalidateFilesUnderPathForTestingImpl(
ExtendedEventHandler eventHandler, ModifiedFileSet modifiedFileSet, Root pathEntry)
throws InterruptedException;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
index 31505d8..d0476fb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
@@ -43,6 +43,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionName;
import java.util.concurrent.atomic.AtomicBoolean;
@@ -100,7 +101,8 @@
new ClientEnvironmentFunction(new AtomicReference<>(ImmutableMap.of())))
.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper))
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)))
.put(SkyFunctions.ACTION_ENVIRONMENT_VARIABLE, new ActionEnvironmentFunction())
.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction())
.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction())
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
index 30f607a..7ec3715 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
@@ -46,6 +46,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -122,7 +123,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider();
skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider));
skyFunctions.put(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java
index bcee6cc..f24ec73 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java
@@ -49,6 +49,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
@@ -110,7 +111,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
skyFunctions.put(
SkyFunctions.RECURSIVE_FILESYSTEM_TRAVERSAL, new RecursiveFilesystemTraversalFunction());
skyFunctions.put(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
index 90a091a..fc9a625 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -163,7 +163,8 @@
skyFunctions.put(SkyFunctions.GLOB, new GlobFunction(alwaysUseDirListing()));
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.PACKAGE_LOOKUP,
@@ -183,7 +184,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
AnalysisMock analysisMock = AnalysisMock.get();
RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java
index 3c99c43..af487b3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunctionTest.java
@@ -43,6 +43,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
@@ -103,7 +104,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider();
skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider));
skyFunctions.put(
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 34fd0d5..9c00e7f 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
@@ -236,6 +236,11 @@
/** Regression test for unexpected exception type from PackageValue. */
@Test
public void testDiscrepancyBetweenLegacyAndSkyframePackageLoadingErrors() throws Exception {
+ // Normally, legacy globbing and skyframe globbing share a cache for `readdir` filesystem calls.
+ // In order to exercise a situation where they observe different results for filesystem calls,
+ // we disable the cache. This might happen in a real scenario, e.g. if the cache hits a limit
+ // and evicts entries.
+ getSkyframeExecutor().turnOffSyscallCacheForTesting();
reporter.removeHandler(failFastHandler);
Path fooBuildFile =
scratch.file("foo/BUILD", "sh_library(name = 'foo', srcs = glob(['bar/*.sh']))");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
index e11df3c..216d6c8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
@@ -54,6 +54,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
@@ -130,7 +131,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
skyFunctions.put(SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES,
new BlacklistedPackagePrefixesFunction(
/*hardcodedBlacklistedPackagePrefixes=*/ ImmutableSet.of(),
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index e2eab3b..8c1f869 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -62,6 +62,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.UnixGlob;
import com.google.devtools.build.skyframe.AbstractSkyKey;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationContext;
@@ -134,7 +135,8 @@
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
SkyFunctions.DIRECTORY_LISTING_STATE,
- new DirectoryListingStateFunction(externalFilesHelper));
+ new DirectoryListingStateFunction(
+ externalFilesHelper, new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS)));
skyFunctions.put(
SkyFunctions.RECURSIVE_FILESYSTEM_TRAVERSAL, new RecursiveFilesystemTraversalFunction());
skyFunctions.put(
