From 3769cc7f832b89096e037d912f50f5199f0a5994 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 7 Feb 2024 07:15:11 -0800 Subject: [PATCH] [7.1.0] Avoid exception-based control flow in RAFS#getDigest and RAFS#getFastDigest. In the same spirit as https://github.com/bazelbuild/bazel/commit/9a86600f68dfe1d802d4e7b2a757e69f20a308d0, avoid unnecessarily allocating a FileNotFoundException when it's not going to be propagated. It makes getFastDigest rather slow in the failure case. This requires changing the contract of statUnchecked (now statInternal) to return null on not found, leaving the decision of whether to throw an exception up to the caller. I/O exceptions other than not found are always propagated, as before. Also improve consistency in terminology: * "deferencing a path" => "canonicalizing a path" * "in-memory filesystem" => "remote output tree" PiperOrigin-RevId: 604973882 Change-Id: I1f78b8c5cce3707eb0608a8c0c85a2b04445a510 --- .../lib/remote/RemoteActionFileSystem.java | 93 ++++++++++--------- .../devtools/build/lib/vfs/FileSystem.java | 1 + 2 files changed, 49 insertions(+), 45 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index ad29e61a593972..27b162a9027dfc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -90,21 +90,21 @@ public class RemoteActionFileSystem extends AbstractFileSystemWithCustomStat { @Nullable private ActionExecutionMetadata action = null; - /** Describes how to handle symlinks when calling {@link #statUnchecked}. */ + /** Describes how to handle symlinks when calling {@link #statInternal}. */ private enum FollowMode { - /** Dereference the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */ + /** Canonicalize the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */ FOLLOW_ALL, - /** Dereference the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */ + /** Canonicalize the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */ FOLLOW_PARENT, - /** Do not dereference. This is only used internally to resolve symlinks efficiently. */ + /** Do not canonicalize. This is only used internally to resolve symlinks efficiently. */ FOLLOW_NONE }; - /** Describes which sources to consider when statting a file. */ + /** Describes which sources to consider when calling {@link #statInternal}. */ private enum StatSources { - /** Consider all sources (action input map, in-memory filesystem and local filesystem). */ + /** Consider all sources (action input map, remote output tree and local filesystem). */ ALL, - /** Consider only in-memory sources (action input map and in-memory filesystem). */ + /** Consider only in-memory sources (action input map and remote output tree). */ IN_MEMORY_ONLY, } @@ -267,13 +267,9 @@ boolean isRemote(Path path) throws IOException { private boolean isRemote(PathFragment path) throws IOException { // Files in the local filesystem are non-remote by definition, so stat only in-memory sources. - try { - var status = statUnchecked(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY); - return (status instanceof FileStatusWithMetadata) - && ((FileStatusWithMetadata) status).getMetadata().isRemote(); - } catch (FileNotFoundException e) { - return false; - } + var status = statInternal(path, FollowMode.FOLLOW_ALL, StatSources.IN_MEMORY_ONLY); + return (status instanceof FileStatusWithMetadata) + && ((FileStatusWithMetadata) status).getMetadata().isRemote(); } public void updateContext(ActionExecutionMetadata action) { @@ -365,18 +361,15 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) } @Override + @Nullable protected byte[] getFastDigest(PathFragment path) throws IOException { path = resolveSymbolicLinks(path).asFragment(); // Try to obtain a fast digest through a stat. This is only possible for in-memory files. // The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is // effectively the same as FOLLOW_PARENT, but more efficient. - try { - var status = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY); - if (status instanceof FileStatusWithDigest) { - return ((FileStatusWithDigest) status).getDigest(); - } - } catch (FileNotFoundException e) { - // Intentionally ignored. + var status = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY); + if (status instanceof FileStatusWithDigest) { + return ((FileStatusWithDigest) status).getDigest(); } return localFs.getPath(path).getFastDigest(); } @@ -387,13 +380,9 @@ protected byte[] getDigest(PathFragment path) throws IOException { // Try to obtain a fast digest through a stat. This is only possible for in-memory files. // The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is // effectively the same as FOLLOW_PARENT, but more efficient. - try { - var status = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY); - if (status instanceof FileStatusWithDigest) { - return ((FileStatusWithDigest) status).getDigest(); - } - } catch (FileNotFoundException e) { - // Intentionally ignored. + var status = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.IN_MEMORY_ONLY); + if (status instanceof FileStatusWithDigest) { + return ((FileStatusWithDigest) status).getDigest(); } return localFs.getPath(path).getDigest(); } @@ -529,7 +518,10 @@ protected PathFragment resolveOneLink(PathFragment path) throws IOException { // // The parent path has already been canonicalized, so FOLLOW_NONE is effectively the same as // FOLLOW_PARENT, but much more efficient as it doesn't call stat recursively. - var stat = statUnchecked(path, FollowMode.FOLLOW_NONE, StatSources.ALL); + var stat = statInternal(path, FollowMode.FOLLOW_NONE, StatSources.ALL); + if (stat == null) { + throw new FileNotFoundException(path.getPathString() + " (No such file or directory)"); + } return stat.isSymbolicLink() ? readSymbolicLink(path) : null; } @@ -554,34 +546,45 @@ protected boolean exists(PathFragment path, boolean followSymlinks) { } } + @Override + protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { + FileStatus stat = statIfFound(path, followSymlinks); + if (stat == null) { + throw new FileNotFoundException(path.getPathString() + " (No such file or directory)"); + } + return stat; + } + @Nullable @Override protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException { - try { - return stat(path, followSymlinks); - } catch (FileNotFoundException e) { - return null; - } + return statInternal( + path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT, StatSources.ALL); } @Nullable @Override protected FileStatus statNullable(PathFragment path, boolean followSymlinks) { try { - return stat(path, followSymlinks); + return statIfFound(path, followSymlinks); } catch (IOException e) { return null; } } - @Override - protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException { - return statUnchecked( - path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT, StatSources.ALL); - } - - private FileStatus statUnchecked( - PathFragment path, FollowMode followMode, StatSources statSources) throws IOException { + /** + * Internal stat implementation. + * + * @param path the path to stat + * @param followMode whether and how to canonicalize the path + * @param statSources which sources to consider + * @return the file status on success, or null if the file was not found in any of the sources + * under consideration + * @throws IOException if an error other than file not found occurred + */ + @Nullable + private FileStatus statInternal(PathFragment path, FollowMode followMode, StatSources statSources) + throws IOException { // Canonicalize the path. if (followMode == FollowMode.FOLLOW_ALL) { path = resolveSymbolicLinks(path).asFragment(); @@ -611,10 +614,10 @@ private FileStatus statUnchecked( } if (statSources == StatSources.ALL) { - return localFs.getPath(path).stat(Symlinks.NOFOLLOW); + return localFs.getPath(path).statIfFound(Symlinks.NOFOLLOW); } - throw new FileNotFoundException(path.getPathString() + " (No such file or directory)"); + return null; } private static FileStatusWithMetadata statFromMetadata(FileArtifactValue m) { 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 e540075115ccac..46a35b971fca20 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 @@ -331,6 +331,7 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks) * filesystem doesn't support them. This digest should be suitable for detecting changes to the * file. */ + @Nullable protected byte[] getFastDigest(PathFragment path) throws IOException { return null; }