Skip path metadata lookup in bytestream uploader when artefact won't be uploaded #23374
Conversation
…er-fastpath-skipped-uploads
github-actions
bot
added
team-Remote-Exec
| 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, | ||
| digestUtil.compute(path), | ||
| /* directory= */ false, | ||
| /* symlink= */ false, | ||
| /* remote= */ false, | ||
| /* omitted= */ true, | ||
| /* isBuildToolLog= */ false, | ||
| digestUtil.getDigestFunction()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm having a hard time trying to understand what functionally changes in this PR. What do we save by exiting early here, considering that we're still calling digestUtil.compute?
There was a problem hiding this comment.
I find myself asking the same question. This went through a number of iterations, as I discovered more and more requirements that incrementally eliminated any kind of potential optimisation.
I'm going to close this. There is room for improvement in the bytestream uploader (I had a stab at some in #23252 but hit a NPE I could not track down, at face value the area seems like it would benefit from a refactor and perhaps alignment across full and minimal upload modes).
There was a problem hiding this comment.
FWIW, I don't think the uploader should be digesting files at all; all output files have already been digested (and the digests cached in memory as a FileArtifactValue) during action execution, so we should figure out a way to plumb that data into the uploader.
There was a problem hiding this comment.
Some spawn intermediate outputs might not be stored, though.
github-actions
bot
removed
the
awaiting-review
Fresh take on #23252 without the NPE.
This PR is an alternate implementation of #20575 that defers to
LocalFileTypewhere possible, adds a test case and fixes a latent NPE insrc/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java(path converter returns null for files lacking a digest, such as directories, symlinks and anything skipped under minimal mode).This should improve performance. Benchmarking (in Bazel v6) has shown 54% of a Bazel profile for a fully cached build is spent in
readPathMetadata. Gains likely won't be as significant as digest lookups are still required (required to produce bytestream URI).Fixes #20576.