Introduce a FileSystemCalls.getType() function that just returns the …

…type of a

file in question. In quite a few places that is sufficient and we might be able
to get the result based on a previously existing readdir() or stat() request if
we have either of them cached.

While at it, re-use the PerBuildSysCall cache inside of FileStateFunction in
order to profit from this.

This can prevent a whole bunch of unnecessary stat() calls, especially for
whether or not a BUILD file is present and for creating the FileStateValue for
symlinks and directories (for regular files we still need to stat() in order to
get modification time information).

RELNOTES: None.
PiperOrigin-RevId: 240794051

src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java

@@ -24,13 +24,15 @@
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.BigIntegerFingerprint;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
+import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 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.AbstractSkyKey;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -73,6 +75,34 @@ public abstract class FileStateValue implements SkyValue {
   protected FileStateValue() {
   }
 
+  public static FileStateValue create(
+      RootedPath rootedPath,
+      FilesystemCalls syscallCache,
+      @Nullable TimestampGranularityMonitor tsgm)
+      throws InconsistentFilesystemException, IOException {
+    Path path = rootedPath.asPath();
+    Dirent.Type type = syscallCache.getType(path, Symlinks.NOFOLLOW);
+    if (type == null) {
+      return NONEXISTENT_FILE_STATE_NODE;
+    }
+    switch (type) {
+      case DIRECTORY:
+        return DIRECTORY_FILE_STATE_NODE;
+      case SYMLINK:
+        return new SymlinkFileStateValue(path.readSymbolicLinkUnchecked());
+      case FILE:
+      case UNKNOWN:
+        {
+          FileStatus stat = syscallCache.statIfFound(path, Symlinks.NOFOLLOW);
+          Preconditions.checkNotNull(
+              stat, "File %s found in directory, but stat failed", rootedPath);
+          return createWithStatNoFollow(rootedPath, FileStatusWithDigestAdapter.adapt(stat), tsgm);
+        }
+      default:
+        throw new IllegalStateException(type.toString());
+    }
+  }
+
   public static FileStateValue create(RootedPath rootedPath,
       @Nullable TimestampGranularityMonitor tsgm) throws InconsistentFilesystemException,
       IOException {

src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.packages.BuildFileName;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -257,8 +258,8 @@ private Path getFilePath(PathFragment suffix,
     for (Root pathEntry : pathEntries) {
       Path buildFile = pathEntry.getRelative(suffix);
       try {
-        FileStatus stat = cache.get().statIfFound(buildFile, Symlinks.FOLLOW);
-        if (stat != null && stat.isFile()) {
+        Dirent.Type type = cache.get().getType(buildFile, Symlinks.FOLLOW);
+        if (type == Dirent.Type.FILE || type == Dirent.Type.UNKNOWN) {
           return buildFile;
         }
       } catch (IOException ignored) {

src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java

@@ -16,6 +16,7 @@
 import com.google.devtools.build.lib.actions.FileStateValue;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.RootedPath;
+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;
@@ -31,11 +32,15 @@
 public class FileStateFunction implements SkyFunction {
 
   private final AtomicReference<TimestampGranularityMonitor> tsgm;
+  private final AtomicReference<FilesystemCalls> syscallCache;
   private final ExternalFilesHelper externalFilesHelper;
 
-  public FileStateFunction(AtomicReference<TimestampGranularityMonitor> tsgm,
+  public FileStateFunction(
+      AtomicReference<TimestampGranularityMonitor> tsgm,
+      AtomicReference<FilesystemCalls> syscallCache,
       ExternalFilesHelper externalFilesHelper) {
     this.tsgm = tsgm;
+    this.syscallCache = syscallCache;
     this.externalFilesHelper = externalFilesHelper;
   }
 
@@ -49,7 +54,7 @@ public FileStateValue compute(SkyKey skyKey, Environment env)
       if (env.valuesMissing()) {
         return null;
       }
-      return FileStateValue.create(rootedPath, tsgm.get());
+      return FileStateValue.create(rootedPath, syscallCache.get(), tsgm.get());
     } catch (ExternalFilesHelper.NonexistentImmutableExternalFileException e) {
       return FileStateValue.NONEXISTENT_FILE_STATE_NODE;
     } catch (IOException e) {

src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java

@@ -111,6 +111,55 @@ public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
     return (status == NO_STATUS) ? null : status;
   }
 
+  @Override
+  public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
+    // Use a cached stat call if we have one. This is done first so that we don't need to iterate
+    // over a list of directory entries as we do for cached readdir() entries. We don't ever expect
+    // to get a cache hit if symlinks == Symlinks.NOFOLLOW and so we don't bother to check.
+    if (symlinks == Symlinks.FOLLOW) {
+      Pair key = Pair.of(path, symlinks);
+      Object result = statCache.getIfPresent(key);
+      if (result != null && !(result instanceof IOException)) {
+        if (result == NO_STATUS) {
+          return null;
+        }
+        return UnixGlob.statusToDirentType((FileStatus) result);
+      }
+    }
+
+    // Answer based on a cached readdir() call if possible. The cache might already be populated
+    // from Skyframe directory lising (DirectoryListingFunction) or by globbing via
+    // {@link UnixGlob}. We generally try to avoid following symlinks in readdir() calls as in a
+    // directory with many symlinks, these would be resolved basically using a stat anyway and they
+    // would be resolved sequentially which can be slow on high-latency file systems. If we request
+    // the type of a file with FOLLOW, and find a symlink in the directory, we fall back to doing a
+    // stat.
+    Pair key = Pair.of(path.getParentDirectory(), Symlinks.NOFOLLOW);
+    Object result = readdirCache.getIfPresent(key);
+    if (result != null && !(result instanceof IOException)) {
+      for (Dirent dirent : (Collection<Dirent>) result) {
+        // TODO(djasper): Dealing with filesystem case is a bit of a code smell. Figure out a better
+        // way to store Dirents, e.g. with names normalized.
+        if (path.getFileSystem().isFilePathCaseSensitive()
+            && !dirent.getName().equals(path.getBaseName())) {
+          continue;
+        }
+        if (!path.getFileSystem().isFilePathCaseSensitive()
+            && !dirent.getName().equalsIgnoreCase(path.getBaseName())) {
+          continue;
+        }
+        if (dirent.getType() == Dirent.Type.SYMLINK && symlinks == Symlinks.FOLLOW) {
+          // See above: We don't want to follow symlinks with readdir(). Do a stat() instead.
+          return UnixGlob.statusToDirentType(statIfFound(path, Symlinks.FOLLOW));
+        }
+        return dirent.getType();
+      }
+      return null;
+    }
+
+    return UnixGlob.statusToDirentType(statIfFound(path, symlinks));
+  }
+
   public void clear() {
     statCache.invalidateAll();
     readdirCache.invalidateAll();

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -596,7 +596,7 @@ protected boolean traverseTestSuites() {
   }
 
   protected SkyFunction newFileStateFunction() {
-    return new FileStateFunction(tsgm, externalFilesHelper);
+    return new FileStateFunction(tsgm, syscalls, externalFilesHelper);
   }
 
   protected SkyFunction newDirectoryListingStateFunction() {

src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java

@@ -74,6 +74,7 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
 import com.google.devtools.build.skyframe.BuildDriver;
 import com.google.devtools.build.skyframe.Differencer;
 import com.google.devtools.build.skyframe.ErrorInfo;
@@ -384,7 +385,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions() {
     Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache =
         CacheBuilder.newBuilder().build();
     Cache<PackageIdentifier, AstParseResult> astCache = CacheBuilder.newBuilder().build();
-    AtomicReference<PerBuildSyscallCache> syscallCacheRef =
+    AtomicReference<FilesystemCalls> syscallCacheRef =
         new AtomicReference<>(
             PerBuildSyscallCache.newBuilder().setConcurrencyLevel(legacyGlobbingThreads).build());
     pkgFactory.setGlobbingThreads(legacyGlobbingThreads);
@@ -402,7 +403,9 @@ public Path getBuildFileForPackage(PackageIdentifier packageName) {
     ImmutableMap.Builder<SkyFunctionName, SkyFunction> builder = ImmutableMap.builder();
     builder
         .put(SkyFunctions.PRECOMPUTED, new PrecomputedFunction())
-        .put(FileStateValue.FILE_STATE, new FileStateFunction(tsgm, externalFilesHelper))
+        .put(
+            FileStateValue.FILE_STATE,
+            new FileStateFunction(tsgm, syscallCacheRef, externalFilesHelper))
         .put(SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction())
         .put(
             SkyFunctions.FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS,

src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java

@@ -258,28 +258,49 @@ private static Pattern makePatternFromWildcard(String pattern) {
    * Filesystem calls required for glob().
    */
   public interface FilesystemCalls {
-    /**
-     * Get directory entries and their types.
-     */
+    /** Get directory entries and their types. */
     Collection<Dirent> readdir(Path path, Symlinks symlinks) throws IOException;
 
+    /** Return the stat() for the given path, or null. */
+    FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException;
+
     /**
-     * Return the stat() for the given path, or null.
+     * Return the type of a specific file. This may be answered using stat() or readdir(). Returns
+     * null if the path does not exist.
      */
-    FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException;
+    Dirent.Type getType(Path path, Symlinks symlinks) throws IOException;
   }
 
-  public static FilesystemCalls DEFAULT_SYSCALLS = new FilesystemCalls() {
-    @Override
-    public Collection<Dirent> readdir(Path path, Symlinks symlinks) throws IOException {
-      return path.readdir(symlinks);
-    }
+  public static final FilesystemCalls DEFAULT_SYSCALLS =
+      new FilesystemCalls() {
+        @Override
+        public Collection<Dirent> readdir(Path path, Symlinks symlinks) throws IOException {
+          return path.readdir(symlinks);
+        }
 
-    @Override
-    public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
-      return path.statIfFound(symlinks);
-    }
-  };
+        @Override
+        public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
+          return path.statIfFound(symlinks);
+        }
+
+        @Override
+        public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
+          return statusToDirentType(statIfFound(path, symlinks));
+        }
+      };
+
+  public static Dirent.Type statusToDirentType(FileStatus status) {
+    if (status == null) {
+      return null;
+    } else if (status.isFile()) {
+      return Dirent.Type.FILE;
+    } else if (status.isDirectory()) {
+      return Dirent.Type.DIRECTORY;
+    } else if (status.isSymbolicLink()) {
+      return Dirent.Type.SYMLINK;
+    }
+    return Dirent.Type.UNKNOWN;
+  }
 
   public static final AtomicReference<FilesystemCalls> DEFAULT_SYSCALLS_REF =
       new AtomicReference<>(DEFAULT_SYSCALLS);

src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java

@@ -95,6 +95,7 @@ protected Collection<Dirent> readdir(Path path, boolean followSymlinks) throws I
   @Test
   public void testBadStatKeepGoing() throws Exception {
     reporter.removeHandler(failFastHandler);
+    getSkyframeExecutor().turnOffSyscallCacheForTesting();
     // Given a package, "parent",
     Path parent = scratch.file("parent/BUILD", "sh_library(name = 'parent')").getParentDirectory();
     // And a child, "badstat",
@@ -123,6 +124,7 @@ public void testBadStatKeepGoing() throws Exception {
   @Test
   public void testBadReaddirKeepGoing() throws Exception {
     reporter.removeHandler(failFastHandler);
+    skyframeExecutor.turnOffSyscallCacheForTesting();
     // Given a package, "parent",
     Path parent = scratch.file("parent/BUILD", "sh_library(name = 'parent')").getParentDirectory();
     // And a child, "badstat",

src/test/java/com/google/devtools/build/lib/repository/ExternalPackageUtilTest.java

@@ -57,6 +57,7 @@
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 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.EvaluationContext;
 import com.google.devtools.build.skyframe.EvaluationResult;
@@ -125,7 +126,9 @@ public void createEnvironment() {
     skyFunctions.put(
         FileStateValue.FILE_STATE,
         new FileStateFunction(
-            new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
+            new AtomicReference<TimestampGranularityMonitor>(),
+            new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+            externalFilesHelper));
     skyFunctions.put(FileValue.FILE, new FileFunction(pkgLocator));
     RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider();
     skyFunctions.put(SkyFunctions.WORKSPACE_AST, new WorkspaceASTFunction(ruleClassProvider));

src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java

@@ -51,6 +51,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;
@@ -109,7 +110,9 @@ public void setupDelegator() throws Exception {
                 .put(
                     FileStateValue.FILE_STATE,
                     new FileStateFunction(
-                        new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper))
+                        new AtomicReference<TimestampGranularityMonitor>(),
+                        new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+                        externalFilesHelper))
                 .put(FileValue.FILE, new FileFunction(pkgLocator))
                 .put(SkyFunctions.REPOSITORY_DIRECTORY, delegatorFunction)
                 .put(

src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java

@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.build.lib.vfs.UnixGlob;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.build.skyframe.InMemoryMemoizingEvaluator;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
@@ -97,7 +98,9 @@ public void baseSetUp() throws Exception {
                 .put(
                     FileStateValue.FILE_STATE,
                     new FileStateFunction(
-                        new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper))
+                        new AtomicReference<TimestampGranularityMonitor>(),
+                        new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+                        externalFilesHelper))
                 .put(FileValue.FILE, new FileFunction(pkgLocator))
                 .put(Artifact.ARTIFACT, new ArtifactFunction(() -> true))
                 .put(SkyFunctions.ACTION_EXECUTION, new SimpleActionExecutionFunction())

src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java

@@ -121,7 +121,9 @@ public final void setUp() throws Exception  {
     skyFunctions.put(
         FileStateValue.FILE_STATE,
         new FileStateFunction(
-            new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
+            new AtomicReference<TimestampGranularityMonitor>(),
+            new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+            externalFilesHelper));
     skyFunctions.put(FileValue.FILE, new FileFunction(pkgLocator));
     skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
     skyFunctions.put(

src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java

@@ -67,6 +67,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.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.build.lib.vfs.util.FileSystems;
 import com.google.devtools.build.skyframe.BuildDriver;
@@ -163,7 +164,9 @@ private SequentialBuildDriver makeDriver(ExternalFileAction externalFileAction)
                 .put(
                     FileStateValue.FILE_STATE,
                     new FileStateFunction(
-                        new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper))
+                        new AtomicReference<TimestampGranularityMonitor>(),
+                        new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+                        externalFilesHelper))
                 .put(
                     SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS,
                     new FileSymlinkCycleUniquenessFunction())

src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java

@@ -106,7 +106,9 @@ public final void setUp() throws Exception  {
     skyFunctions.put(
         FileStateValue.FILE_STATE,
         new FileStateFunction(
-            new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
+            new AtomicReference<TimestampGranularityMonitor>(),
+            new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+            externalFilesHelper));
     skyFunctions.put(FileValue.FILE, new FileFunction(pkgLocator));
     skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
     skyFunctions.put(

src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java

@@ -64,6 +64,7 @@
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
+import com.google.devtools.build.lib.vfs.UnixGlob;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.devtools.build.skyframe.Differencer.Diff;
 import com.google.devtools.build.skyframe.EvaluationContext;
@@ -137,7 +138,9 @@ public final void setUp() throws Exception  {
     skyFunctions.put(
         FileStateValue.FILE_STATE,
         new FileStateFunction(
-            new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
+            new AtomicReference<TimestampGranularityMonitor>(),
+            new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
+            externalFilesHelper));
     skyFunctions.put(FileValue.FILE, new FileFunction(pkgLocator));
     skyFunctions.put(
         SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction());