From 01d71d3dbbb8914c8458a18389a5213561fcec78 Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Wed, 9 Nov 2022 03:25:21 -0800 Subject: [PATCH] Emit Tree objects in topological order remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - https://github.com/bazelbuild/remote-apis/pull/229 - https://github.com/bazelbuild/remote-apis/pull/230 Closes #16463. PiperOrigin-RevId: 487196375 Change-Id: Iafcfd617fc101fec7bfa943552113ce57ab8041b --- .../build/lib/remote/UploadManifest.java | 49 ++++-- .../build/lib/remote/GrpcCacheClientTest.java | 18 ++- .../remote/RemoteExecutionServiceTest.java | 18 ++- .../build/lib/remote/UploadManifestTest.java | 145 ++++++++++++++++-- 4 files changed, 198 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java index 742c74f3074404..07c02ac35688db 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java +++ b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java @@ -31,6 +31,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; import com.google.devtools.build.lib.actions.ActionUploadFinishedEvent; import com.google.devtools.build.lib.actions.ActionUploadStartedEvent; @@ -54,10 +55,12 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.protobuf.ByteString; +import com.google.protobuf.CodedOutputStream; import com.google.protobuf.Timestamp; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; import io.reactivex.rxjava3.core.Single; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.time.Duration; import java.time.Instant; @@ -65,9 +68,11 @@ import java.util.Collection; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -303,25 +308,43 @@ private void addFile(Digest digest, Path file) throws IOException { digestToFile.put(digest, file); } + // Field numbers of the 'root' and 'directory' fields in the Tree message. + private static final int TREE_ROOT_FIELD_NUMBER = + Tree.getDescriptor().findFieldByName("root").getNumber(); + private static final int TREE_CHILDREN_FIELD_NUMBER = + Tree.getDescriptor().findFieldByName("children").getNumber(); + private void addDirectory(Path dir) throws ExecException, IOException { - Tree.Builder tree = Tree.newBuilder(); - Directory root = computeDirectory(dir, tree); - tree.setRoot(root); + Set directories = new LinkedHashSet<>(); + var ignored = computeDirectory(dir, directories); + + // Convert individual Directory messages to a Tree message. As we want the + // records to be topologically sorted (parents before children), we iterate + // over the directories in reverse insertion order. + ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); + CodedOutputStream codedOutputStream = CodedOutputStream.newInstance(byteArrayOutputStream); + int fieldNumber = TREE_ROOT_FIELD_NUMBER; + for (ByteString directory : Lists.reverse(new ArrayList(directories))) { + codedOutputStream.writeBytes(fieldNumber, directory); + fieldNumber = TREE_CHILDREN_FIELD_NUMBER; + } + codedOutputStream.flush(); - ByteString data = tree.build().toByteString(); + ByteString data = ByteString.copyFrom(byteArrayOutputStream.toByteArray()); Digest digest = digestUtil.compute(data.toByteArray()); if (result != null) { result .addOutputDirectoriesBuilder() .setPath(remotePathResolver.localPathToOutputPath(dir)) - .setTreeDigest(digest); + .setTreeDigest(digest) + .setIsTopologicallySorted(true); } digestToBlobs.put(digest, data); } - private Directory computeDirectory(Path path, Tree.Builder tree) + private ByteString computeDirectory(Path path, Set directories) throws ExecException, IOException { Directory.Builder b = Directory.newBuilder(); @@ -332,9 +355,8 @@ private Directory computeDirectory(Path path, Tree.Builder tree) String name = dirent.getName(); Path child = path.getRelative(name); if (dirent.getType() == Dirent.Type.DIRECTORY) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); + ByteString dir = computeDirectory(child, directories); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray())); } else if (dirent.getType() == Dirent.Type.SYMLINK) { PathFragment target = child.readSymbolicLink(); if (!followSymlinks && !target.isAbsolute()) { @@ -353,9 +375,8 @@ private Directory computeDirectory(Path path, Tree.Builder tree) b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); digestToFile.put(digest, child); } else if (statFollow.isDirectory()) { - Directory dir = computeDirectory(child, tree); - b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); - tree.addChildren(dir); + ByteString dir = computeDirectory(child, directories); + b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray())); } else { illegalOutput(child); } @@ -368,7 +389,9 @@ private Directory computeDirectory(Path path, Tree.Builder tree) } } - return b.build(); + ByteString directory = b.build().toByteString(); + directories.add(directory); + return directory; } private void illegalOutput(Path path) throws ExecException { diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 4df12d4cb1e107..9f7b55148c6330 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -369,7 +369,11 @@ public void updateActionResult( .setPath("a/foo") .setDigest(fooDigest) .setIsExecutable(true); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } @@ -409,7 +413,11 @@ public void updateActionResult( ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } @@ -472,7 +480,11 @@ public void updateActionResult( ActionResult result = uploadDirectory(remoteCache, ImmutableList.of(barDir)); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(result).isEqualTo(expectedResult.build()); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 8341d8ca89a1c5..1808b86d66c5ef 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1314,7 +1314,11 @@ public void uploadOutputs_uploadDirectory_works() throws Exception { .setPath("outputs/a/foo") .setDigest(fooDigest) .setIsExecutable(true); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); ImmutableList toQuery = ImmutableList.of(fooDigest, quxDigest, barDigest); @@ -1352,7 +1356,11 @@ public void uploadOutputs_uploadEmptyDirectory_works() throws Exception { // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); assertThat( getFromFuture( @@ -1417,7 +1425,11 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception { // assert ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("outputs/bar").setTreeDigest(barDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("outputs/bar") + .setTreeDigest(barDigest) + .setIsTopologicallySorted(true); assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build()); ImmutableList toQuery = ImmutableList.of(wobbleDigest, quxDigest, barDigest); diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java index bc758073d7a9c9..7f28efb8efb9d9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -41,6 +42,8 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -249,7 +252,11 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkAsDirectory() th Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -309,7 +316,11 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkAsDirectory() Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -369,7 +380,11 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkAsDirectory() th Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("link").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("link") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -570,7 +585,11 @@ public void actionResult_followSymlinks_absoluteFileSymlinkInDirectoryAsFile() t Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -615,7 +634,11 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkInDirectoryAsDir Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -651,7 +674,11 @@ public void actionResult_noFollowSymlinks_absoluteFileSymlinkInDirectoryAsFile() Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -696,7 +723,11 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkInDirectoryAsD Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -731,7 +762,11 @@ public void actionResult_followSymlinks_relativeFileSymlinkInDirectoryAsFile() t Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -776,7 +811,11 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkInDirectoryAsDir Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -811,7 +850,11 @@ public void actionResult_noFollowSymlinks_relativeFileSymlinkInDirectoryAsSymlin Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -848,7 +891,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -933,7 +980,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -968,7 +1019,11 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS Digest treeDigest = digestUtil.compute(tree); ActionResult.Builder expectedResult = ActionResult.newBuilder(); - expectedResult.addOutputDirectoriesBuilder().setPath("dir").setTreeDigest(treeDigest); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); assertThat(result.build()).isEqualTo(expectedResult.build()); } @@ -1053,6 +1108,70 @@ public void actionResult_followSymlinks_specialFileSymlinkInDirectoryError() thr assertThat(e).hasMessageThat().contains("dir/link"); } + @Test + public void actionResult_topologicallySortedAndDeduplicatedTree() throws Exception { + // Create 5^3 identical files named "dir/%d/%d/%d/file". + Path dir = execRoot.getRelative("dir"); + dir.createDirectory(); + byte[] fileContents = new byte[] {1, 2, 3, 4, 5}; + final int childrenPerDirectory = 5; + for (int a = 0; a < childrenPerDirectory; a++) { + Path pathA = dir.getRelative(Integer.toString(a)); + pathA.createDirectory(); + for (int b = 0; b < childrenPerDirectory; b++) { + Path pathB = pathA.getRelative(Integer.toString(b)); + pathB.createDirectory(); + for (int c = 0; c < childrenPerDirectory; c++) { + Path pathC = pathB.getRelative(Integer.toString(c)); + pathC.createDirectory(); + Path file = pathC.getRelative("file"); + FileSystemUtils.writeContent(file, fileContents); + } + } + } + + ActionResult.Builder result = ActionResult.newBuilder(); + UploadManifest um = + new UploadManifest( + digestUtil, + remotePathResolver, + result, + /*followSymlinks=*/ false, + /*allowDanglingSymlinks=*/ false, + /*allowAbsoluteSymlinks=*/ false); + um.addFiles(ImmutableList.of(dir)); + + // Even though we constructed 1 + 5 + 5^2 + 5^3 directories, the resulting + // Tree message should only contain four unique instances. The directories + // should also be topologically sorted. + List children = new ArrayList<>(); + Directory root = + Directory.newBuilder() + .addFiles( + FileNode.newBuilder().setName("file").setDigest(digestUtil.compute(fileContents))) + .build(); + for (int depth = 0; depth < 3; depth++) { + Directory.Builder b = Directory.newBuilder(); + Digest parentDigest = digestUtil.compute(root.toByteArray()); + for (int i = 0; i < childrenPerDirectory; i++) { + b.addDirectories( + DirectoryNode.newBuilder().setName(Integer.toString(i)).setDigest(parentDigest)); + } + children.add(0, root); + root = b.build(); + } + Tree tree = Tree.newBuilder().setRoot(root).addAllChildren(children).build(); + Digest treeDigest = digestUtil.compute(tree); + + ActionResult.Builder expectedResult = ActionResult.newBuilder(); + expectedResult + .addOutputDirectoriesBuilder() + .setPath("dir") + .setTreeDigest(treeDigest) + .setIsTopologicallySorted(true); + assertThat(result.build()).isEqualTo(expectedResult.build()); + } + private Path createSpecialFile(String execPath) throws IOException { Path special = mock(Path.class); when(special.statIfFound(Symlinks.NOFOLLOW)).thenReturn(SPECIAL_FILE_STATUS);