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.

Sign up for GitHub

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

Jump to bottom

Filter files early in bytestream uploader to avoid redundant work and double handling #23252

Closed
wants to merge 5 commits into from
Closed

Filter files early in bytestream uploader to avoid redundant work and double handling #23252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
532bc68
Filter artifacts early in bytestream uploader when in minimal upload …
Silic0nS0ldier Aug 9, 2024
9ad1b8b
Add test for bytestream uploader in minimal mode
Silic0nS0ldier Aug 9, 2024
3c85d83
Always check twice (spelling)
Silic0nS0ldier Aug 9, 2024
1c3bc38
Handle minimal mode the same as omitted files
Silic0nS0ldier Aug 10, 2024
7cf1a5b
Remove filter and unused methods
Silic0nS0ldier Aug 10, 2024
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
145 changes: 71 additions & 74 deletions src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@
class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
implements BuildEventArtifactUploader {
private static final Pattern TEST_LOG_PATTERN = Pattern.compile(".*/bazel-out/[^/]*/testlogs/.*");
private static final Pattern BUILD_LOG_PATTERN =
Pattern.compile(".*/bazel-out/_tmp/actions/std(err|out)-.*");

private final Executor executor;
private final ExtendedEventHandler reporter;
Expand Down Expand Up @@ -131,62 +129,70 @@ private static boolean isRemoteFile(Path file) throws IOException {
&& ((RemoteActionFileSystem) file.getFileSystem()).isRemote(file);
}

private enum PathType {
FILE,
DIRECTORY,
SYMLINK,
}

private static final class PathMetadata {

private final Path path;
@Nullable
private final Digest digest;
private final boolean directory;
private final boolean symlink;
/**
* Type of artifact this path represents.
* `null` means this path is omitted and the type unknown (only needed when it may be uploaded).
*/
@Nullable
private final PathType type;
/**
* Indicates file exists on remote and does not need to be uploaded.
*/
private final boolean remote;
private final boolean omitted;
private final boolean isBuildToolLog;
private final DigestFunction.Value digestFunction;

PathMetadata(
Path path,
@Nullable
Digest digest,
boolean directory,
boolean symlink,
@Nullable
PathType type,
boolean remote,
boolean omitted,
boolean isBuildToolLog,
DigestFunction.Value digestFunction) {
this.path = path;
this.digest = digest;
this.directory = directory;
this.symlink = symlink;
this.type = type;
this.remote = remote;
this.omitted = omitted;
this.isBuildToolLog = isBuildToolLog;
this.digestFunction = digestFunction;
}

public Path getPath() {
return path;
}

@Nullable
public Digest getDigest() {
return digest;
}

public boolean isDirectory() {
return directory;
}

public boolean isSymlink() {
return symlink;
@Nullable
public PathType getType() {
return type;
}

public boolean isRemote() {
return remote;
}

/**
* Omitted paths are not uploaded, but may exist on the remote.
* A path may be omitted by;
* - A tag (e.g. `no-remote-cache` and `no-remote-cache-upload`).
* - A CLI flag (e.g. `--remote_build_event_upload=minimal`).
*/
public boolean isOmitted() {
return omitted;
}

public boolean isBuildToolLog() {
return isBuildToolLog;
return type == null;
}

public DigestFunction.Value getDigestFunction() {
Expand All @@ -201,6 +207,26 @@ public DigestFunction.Value getDigestFunction() {
private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOException {
DigestUtil digestUtil = new DigestUtil(xattrProvider, path.getFileSystem().getDigestFunction());

if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) {
// Omit all unimportant paths
if (!(
// Logs (regex needed for test logs as they lack a dedicated `LocalFileType`)
TEST_LOG_PATTERN.matcher(path.getPathString()).matches()
|| file.type == LocalFileType.LOG
|| file.type == LocalFileType.PERFORMANCE_LOG
// std(out|err)
|| file.type == LocalFileType.STDOUT
|| file.type == LocalFileType.STDERR
)) {
return new PathMetadata(
path,
/* digest= */ null,
/* type= */ null,
/* remote= */ false,
digestUtil.getDigestFunction());
}
}

if (file.type == LocalFileType.OUTPUT_DIRECTORY
|| ((file.type == LocalFileType.SUCCESSFUL_TEST_OUTPUT
|| file.type == LocalFileType.FAILED_TEST_OUTPUT
Expand All @@ -209,49 +235,38 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept
return new PathMetadata(
path,
/* digest= */ null,
/* directory= */ true,
/* symlink= */ false,
PathType.DIRECTORY,
/* remote= */ false,
/* omitted= */ false,
/* isBuildToolLog= */ false,
/* digestFunction= */ digestUtil.getDigestFunction());
digestUtil.getDigestFunction());
}
if (file.type == LocalFileType.OUTPUT_SYMLINK) {
return new PathMetadata(
path,
/* digest= */ null,
/* directory= */ false,
/* symlink= */ true,
PathType.SYMLINK,
/* remote= */ false,
/* omitted= */ false,
/* isBuildToolLog= */ false,
/* digestFunction= */ digestUtil.getDigestFunction());
digestUtil.getDigestFunction());
}

PathFragment filePathFragment = path.asFragment();
boolean omitted = false;
PathType pathType = PathType.FILE;
if (omittedFiles.contains(filePathFragment)) {
omitted = true;
pathType = null;
} else {
for (PathFragment treeRoot : omittedTreeRoots) {
if (path.startsWith(treeRoot)) {
omittedFiles.add(filePathFragment);
omitted = true;
pathType = null;
}
}
}

Digest digest = digestUtil.compute(path);
boolean isBuildToolLog =
file.type == LocalFileType.LOG || file.type == LocalFileType.PERFORMANCE_LOG;
return new PathMetadata(
path,
digest,
/* directory= */ false,
/* symlink= */ false,
pathType,
isRemoteFile(path),
omitted,
isBuildToolLog,
digestUtil.getDigestFunction());
}

Expand All @@ -267,35 +282,18 @@ private static void processQueryResult(
new PathMetadata(
file.getPath(),
file.getDigest(),
file.isDirectory(),
file.isSymlink(),
file.getType(),
/* remote= */ true,
file.isOmitted(),
file.isBuildToolLog(),
file.getDigestFunction());
knownRemotePaths.add(remotePathMetadata);
}
}
}

private boolean shouldUpload(PathMetadata path) {
boolean result =
path.getDigest() != null
return path.getDigest() != null
&& !path.isRemote()
&& !path.isDirectory()
&& !path.isSymlink()
&& !path.isOmitted();

if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) {
result = result && (path.isBuildToolLog() || isBuildOrTestLog(path));
}

return result;
}

private boolean isBuildOrTestLog(PathMetadata path) {
return TEST_LOG_PATTERN.matcher(path.getPath().getPathString()).matches()
|| BUILD_LOG_PATTERN.matcher(path.getPath().getPathString()).matches();
&& path.getType() == PathType.FILE;
}

private Single<List<PathMetadata>> queryRemoteCache(
Expand Down Expand Up @@ -364,13 +362,10 @@ private Single<List<PathMetadata>> uploadLocalFiles(
new PathMetadata(
path.getPath(),
path.getDigest(),
path.isDirectory(),
path.isSymlink(),
path.getType(),
// set remote to true so the PathConverter will use bytestream://
// scheme to convert the URI for this file
/* remote= */ true,
path.isOmitted(),
path.isBuildToolLog(),
path.getDigestFunction()))
.onErrorResumeNext(
error -> {
Expand Down Expand Up @@ -409,7 +404,7 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {

return Single.using(
remoteCache::retain,
remoteCache ->
remoteCache ->
Flowable.fromIterable(files.entrySet())
.map(
entry -> {
Expand All @@ -422,11 +417,8 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
return new PathMetadata(
path,
/* digest= */ null,
/* directory= */ false,
/* symlink= */ false,
/* type= */ null,
/* remote= */ false,
/* omitted= */ false,
/* isBuildToolLog= */ false,
DigestFunction.Value.SHA256);
}
})
Expand All @@ -445,6 +437,11 @@ private Single<PathConverter> doUpload(Map<Path, LocalFile> files) {
RemoteCache::release);
}

/**
* NOTE: When in minimal upload mode not all files will be made available on the remote.
* The returned `PathMapper` will only reflect this, throwing `IllegalStateException`
* if given a path only available locally.
*/
@Override
public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
return toListenableFuture(doUpload(files).subscribeOn(scheduler));
Expand Down Expand Up @@ -518,7 +515,7 @@ public String apply(Path path) {
if (skippedPaths.contains(path)) {
return null;
}
// It's a programming error to reference a file that has not been uploaded.
// It's a programming error to reference a file that has not been uploaded or been omitted.
throw new IllegalStateException(
String.format("Illegal file reference: '%s'", path.getPathString()));
}
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
if (expandedArtifact.relPath == null) {
String uri =
pathConverter.apply(completionContext.pathResolver().toPath(expandedArtifact.artifact));
if (uri == null) {
continue;
}
BuildEventStreamProtos.File file =
newFileFromArtifact(
/* name= */ null, expandedArtifact.artifact, completionContext, uri);
Expand All @@ -119,6 +122,9 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
String uri =
pathConverter.apply(
completionContext.pathResolver().convertPath(expandedArtifact.target));
if (uri == null) {
continue;
}
BuildEventStreamProtos.File file =
newFileFromArtifact(
/* name= */ null,
Expand Down
Loading
Loading