Skip to content

Commit

Permalink
[7.1.0] Avoid exception-based control flow in RAFS#getDigest and RAFS…
Browse files Browse the repository at this point in the history
…#getFastDigest. (#21264)

In the same spirit as
9a86600,
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
  • Loading branch information
tjgq authored Feb 8, 2024
1 parent 8632aaf commit a91fce1
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit a91fce1

Please sign in to comment.