Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Optimize RemoteActionFileSystem#readdir for the tree artifact input case. #21251

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ private <T extends Comparable<T>> ImmutableSortedSet<T> getDirectoryContents(
path = resolveSymbolicLinks(path).asFragment();

HashMap<String, Dirent> entries = new HashMap<>();
boolean found = false;
boolean exists = false;

if (path.startsWith(execRoot)) {
var execPath = path.relativeTo(execRoot);
Expand All @@ -791,31 +791,38 @@ private <T extends Comparable<T>> ImmutableSortedSet<T> getDirectoryContents(
for (var entry : treeEntries) {
entries.put(entry.getName(), entry);
}
found = true;
exists = true;
}
}

if (isOutput(path)) {
// Since actions are assumed not to modify their inputs, a directory belonging to an input tree
// artifact cannot also contain an output, so we can safely skip the other sources.
if (!exists) {
if (isOutput(path)) {
try {
for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
exists = true;
} catch (FileNotFoundException ignored) {
// Will be rethrown below if directory does not exist in any of the sources.
}
}

try {
for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
found = true;
exists = true;
} catch (FileNotFoundException ignored) {
// Will be rethrown below if directory exists on neither side.
// Will be rethrown below if directory does not exist in any of the sources.
}
}

try {
for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
} catch (FileNotFoundException e) {
if (!found) {
throw e;
}
if (!exists) {
throw new FileNotFoundException(path.getPathString() + " (No such file or directory)");
}

// Sort entries to get a deterministic order.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -658,9 +658,17 @@ public void readdir_fromInputArtifactData() throws Exception {
}

@Test
public void readdir_fromMultipleSources() throws Exception {
public void readdir_fromInputArtifactData_emptyDir() throws Exception {
ActionInputMap inputs = new ActionInputMap(1);
createLocalTreeArtifact("tree", ImmutableMap.of(), inputs);
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);

assertReaddir(actionFs, getOutputPath("tree"), /* followSymlinks= */ true);
}

@Test
public void readdir_fromRemoteOutputTreeAndLocalFilesystem() throws Exception {
ActionInputMap inputs = new ActionInputMap(1);
createLocalTreeArtifact("dir", ImmutableMap.of("subdir/subfile", ""), inputs);
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
writeLocalFile(actionFs, getOutputPath("dir/out1"), "contents1");
injectRemoteFile(actionFs, getOutputPath("dir/out2"), "contents2");
Expand All @@ -671,8 +679,18 @@ public void readdir_fromMultipleSources() throws Exception {
dirPath,
/* followSymlinks= */ true,
new Dirent("out1", Dirent.Type.FILE),
new Dirent("out2", Dirent.Type.FILE),
new Dirent("subdir", Dirent.Type.DIRECTORY));
new Dirent("out2", Dirent.Type.FILE));
}

@Test
public void readdir_fromRemoteOutputTreeAndLocalFilesystem_emptyDir() throws Exception {
ActionInputMap inputs = new ActionInputMap(1);
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
PathFragment dirPath = getOutputPath("dir");
actionFs.getRemoteOutputTree().getPath(dirPath).createDirectoryAndParents();
actionFs.getLocalFileSystem().getPath(dirPath).createDirectoryAndParents();

assertReaddir(actionFs, dirPath, /* followSymlinks= */ true);
}

@Test
Expand Down
Loading