Add support for compression on gRPC cache #14041

AlessandroPatti · 2021-09-26T10:13:30Z

Add support for compressed transfers from/to gRPC remote caches. Tested with bazel-remote 2.13.

Relates to #13344

google-cla · 2021-09-26T10:13:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

AlessandroPatti · 2021-09-26T10:41:49Z

@googlebot I signed it!

coeuvre

Thanks for the PR!

For reasons, we can't merge PR that contains both non third_party and third_party changes. Can you split third_party changes into another PR?

src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java

src/main/java/com/google/devtools/build/lib/remote/Chunker.java

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

benjaminp · 2021-09-28T13:49:46Z

Note that zstd-jni has a sad history here: #12437, #11968

AlessandroPatti · 2021-09-29T08:21:13Z

Note that zstd-jni has a sad history here: #12437, #11968

Thank you @benjaminp for pointing that out. @philwo seems like #12437 got reverted, what was the reason of the revert? From #11968, it seems there was an issue with the version of apache commons compress, which is not used here. Can zstd-jni be merged back?

philwo · 2021-09-29T13:41:12Z

seems like #12437 got reverted, what was the reason of the revert?

From the rollback commit: "We're rolling back this feature before cutting Bazel 4.0 LTS, as we can't merge the remaining part in time and are worried that the newly added patched JNI dependency will make it hard for distributions like Debian to accept. Let's revisit this when we find a suitable pure Java version of Zstd."

The only pure Java version I could find was https://github.com/airlift/aircompressor, which still doesn't support Big Endian (and it doesn't look like they care about that). So, we'd be OK with adding zstdlib and zstd-jni now, probably.

Can zstd-jni be merged back?

@meteorcloudy @olekw how difficult would this additional dependency be for Debian (it seems like it's currently not packaged, but maybe it could easily be)?
We can't accept prebuilt binary JARs (I know we still have some, but we really need to get rid of them) and instead would like to build all dependencies from source. We'd have to replace the checked in JAR files in this PR with instead downloading zstdlib and zstd-jni's sources in our WORKSPACE file, creating BUILD files for them and then using that to build the whole thing from source. I'd love to open-source the BUILD files that we use internally for these two libraries for your reference, let me see whether I can easily do this...
@meisterT should have another look at it from a performance / binary size increase perspective then

meteorcloudy · 2021-09-29T15:30:05Z

@meteorcloudy @olekw how difficult would this additional dependency be for Debian (it seems like it's currently not packaged, but maybe it could easily be)?

I assume https://github.com/luben/zstd-jni is the repo of this library? Looks like it doesn't have much dependencies and already builds with gradle, so should be easy to package for Debian. But leaving @olekw to confirm.

AlessandroPatti · 2021-10-18T08:11:23Z

@philwo @coeuvre I've addressed the issues from your comment, could you please have another look? If it looks good, I'll create another PR to merge changes for third-party code first.

philwo

This looks great to me, thank you! I'd like someone from @bazelbuild/remote-execution to give an LGTM, too, then we can import this.

philwo · 2021-10-19T12:36:29Z

I'll create another PR to merge changes for third-party code first.

In this case I think it should be possible to import the third-party change directly from this PR, so I can try that first once we're ready to merge.

AlessandroPatti · 2021-10-19T15:58:56Z

distdir_deps.bzl

+        "archive": "v1.5.0-4.zip",
+        "sha256": "d320d59b89a163c5efccbe4915ae6a49883ce653cdc670643dfa21c6063108e4",
+        "urls": [
+            "https://github.com/luben/zstd-jni/archive/v1.5.0-4.zip",


It might be good to mirror this in mirror.bazel.build. @philwo could you help with that as part of the merge?

https://mirror.bazel.build/github.com/luben/zstd-jni/archive/v1.5.0-4.zip is now available.

coeuvre

LGTM. @philwo we can start to merge this.

third_party/zstd-jni/zstd-jni.BUILD

coeuvre · 2021-11-04T06:42:50Z

The import is almost done except that we use an older zstd-jni version internally which doesn't contain the fix luben/zstd-jni@3d51bdc. So the test streamCanBeDecompressedOneByteAtATime is stuck in an infinite loop.

@AlessandroPatti, while we are waiting for upgrading internal zstd-jni (it's not an easy task and I am afraid that it won't be done soon), is there any workaround we could apply?

AlessandroPatti · 2021-11-04T09:07:52Z

The import is almost done except that we use an older zstd-jni version internally which doesn't contain the fix luben/zstd-jni@3d51bdc. So the test streamCanBeDecompressedOneByteAtATime is stuck in an infinite loop.

@AlessandroPatti, while we are waiting for upgrading internal zstd-jni (it's not an easy task and I am afraid that it won't be done soon), is there any workaround we could apply?

What version are you using internally? I can try to reproduce on my end and see if there is any workaround.
Alternatively, can we disable the test for lower zstd versions (e.g. if (ZstdVersion.VERSION != "1.5.0-4") return;) to make the Pr merge cleanly? As long as the compression does not get enabled, this code path wouldn't get hit internally and it can wait for zstd-jni to be upgraded.

coeuvre · 2021-11-05T02:57:53Z

We are at luben/zstd-jni@190e4f2 with some custom patches.

Alternatively, can we disable the test for lower zstd versions (e.g. if (ZstdVersion.VERSION != "1.5.0-4") return;) to make the Pr merge cleanly?

SGTM.

coeuvre · 2021-11-05T07:44:58Z

Imported and waiting for the final review.

Some zstd tests are not tested internally but should be fine since they are always tested on Bazel CI.

brentleyjones · 2021-11-12T17:13:50Z

@coeuvre What's your feeling on the risk level of getting this into 5.0?

coeuvre · 2021-11-15T03:21:09Z

It should be fine to get this into 5.0 since it is guarded by an experimental flag.

Add support for compressed transfers from/to gRPC remote caches with flag --experimental_remote_cache_compression. Fixes bazelbuild#13344. Closes bazelbuild#14041. PiperOrigin-RevId: 409328001

* Add patch files for zstd-jni Partial commit for third_party/*, see #14203. Closes #14203 Signed-off-by: Yun Peng <pcloudy@google.com> * Remote: Add support for compression on gRPC cache Add support for compressed transfers from/to gRPC remote caches with flag --experimental_remote_cache_compression. Fixes #13344. Closes #14041. PiperOrigin-RevId: 409328001 Co-authored-by: Alessandro Patti <ale812@yahoo.it>

werkt · 2022-11-20T16:11:47Z

So I got up close and personal with this implementation when I added zstd support to buildfarm, per the supplied streams in the zstd package, and I feel this needs to be reworked.

The overall stream is not being compressed for transport in a tidily shorter sequence of upload/download chunks. Instead, each chunk is being compressed and sent as a shorter WriteRequest/ReadResponse. This results in drastically poorer overall byte ratio of identity:compressed compared to a simple zstd invocation on blob content, with a higher possibility of blob inflation due to the reduced context and overhead of the smaller blocks.

This was discovered when the upload went through buildfarm's rechunking mechanism for shards, where request chunks would be combined or split on 16k boundaries by default, and the stream processor only worked on the current chunk, discarding appended bytes, and eliciting decompression corruption.

Overall, this implementation is in conflict with the zstd compressor description in the remote apis, which makes no mention of sequential compressor streams in compressed packed blob content, but instead expects the full content to be deliverable to a compressor engine in a single context.

I'm going to try to adjust this to get a full blob's worth of compression going, with even block sizes and no per-request/response semantics for the stream.

google-cla bot added the cla: no label Sep 26, 2021

google-cla bot added cla: yes and removed cla: no labels Sep 26, 2021

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch 4 times, most recently from a522fc5 to 57f86be Compare September 27, 2021 07:14

coeuvre requested changes Sep 28, 2021

View reviewed changes

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch 2 times, most recently from cc27594 to 90c49c9 Compare September 29, 2021 08:15

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch from 90c49c9 to 99d43c5 Compare September 29, 2021 08:32

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch from 99d43c5 to d86b71e Compare October 13, 2021 21:02

AlessandroPatti requested a review from a team as a code owner October 13, 2021 21:02

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch 4 times, most recently from 1f82978 to 4250f0f Compare October 14, 2021 09:07

AlessandroPatti added 7 commits October 15, 2021 18:47

[Compression] Add experimental option and verify capabilities

ccea6a4

[Compression] Implement bytestream compressed cache downloads

148ebc6

[Compression] Implement bytestream compressed cache uploads

bb6b53f

[Compression][Tests] Test chunker with compression

ad5b0b4

[Compression][Tests] Test compressed cache uplaods

c0a3b3e

[Compression][Tests] Test compressed cache downloads

1d643aa

Allow calling getActualSize when upload are not finished

4d1dc31

Add simple tests for zstd streams

6c13594

AlessandroPatti force-pushed the apatti/cache-bytestream-compression branch from e8369f5 to 6c13594 Compare October 18, 2021 07:40

philwo approved these changes Oct 19, 2021

View reviewed changes

AlessandroPatti commented Oct 19, 2021

View reviewed changes

coeuvre approved these changes Oct 20, 2021

View reviewed changes

meteorcloudy requested changes Oct 20, 2021

View reviewed changes

third_party/zstd-jni/zstd-jni.BUILD Show resolved Hide resolved

AlessandroPatti added 2 commits October 20, 2021 21:57

Move native patch to patch file

9648105

Add mirror

522a07f

AlessandroPatti requested a review from meteorcloudy October 20, 2021 20:52

meteorcloudy approved these changes Oct 21, 2021

View reviewed changes

third_party/zstd-jni/zstd-jni.BUILD Show resolved Hide resolved

coeuvre mentioned this pull request Nov 1, 2021

Add support for compression on gRPC cache (third-party changes) #14203

Closed

bazel-io closed this in 6da8086 Nov 12, 2021

coeuvre mentioned this pull request Nov 15, 2021

[5.x] Remote: Add support for compression on gRPC cache #14277

Merged

coeuvre mentioned this pull request Nov 26, 2021

Support compressed Bytestream transfers for remote execution #12670

Closed

shirchen mentioned this pull request Jan 6, 2022

bazel 5.x.x crashes if uncompressed CAS is uploaded to remote cache #14522

Open

meteorcloudy mentioned this pull request Sep 16, 2022

UnsatisfiedLinkError with --experimental_remote_cache_compression on Windows #16041

Closed

werkt reopened this Nov 20, 2022

werkt closed this Nov 20, 2022

werkt mentioned this pull request Nov 20, 2022

Deliver/Expect singly compressed content with a single ByteStream::Write/Read #16808

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for compression on gRPC cache #14041

Add support for compression on gRPC cache #14041

AlessandroPatti commented Sep 26, 2021

google-cla bot commented Sep 26, 2021

AlessandroPatti commented Sep 26, 2021

coeuvre left a comment

benjaminp commented Sep 28, 2021

AlessandroPatti commented Sep 29, 2021

philwo commented Sep 29, 2021

meteorcloudy commented Sep 29, 2021

AlessandroPatti commented Oct 18, 2021

philwo left a comment

philwo commented Oct 19, 2021

AlessandroPatti Oct 19, 2021 •

edited

meteorcloudy Oct 20, 2021

coeuvre left a comment

coeuvre commented Nov 4, 2021 •

edited

AlessandroPatti commented Nov 4, 2021

coeuvre commented Nov 5, 2021

coeuvre commented Nov 5, 2021

brentleyjones commented Nov 12, 2021

coeuvre commented Nov 15, 2021

werkt commented Nov 20, 2022

Add support for compression on gRPC cache #14041

Add support for compression on gRPC cache #14041

Conversation

AlessandroPatti commented Sep 26, 2021

google-cla bot commented Sep 26, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

AlessandroPatti commented Sep 26, 2021

coeuvre left a comment

Choose a reason for hiding this comment

benjaminp commented Sep 28, 2021

AlessandroPatti commented Sep 29, 2021

philwo commented Sep 29, 2021

meteorcloudy commented Sep 29, 2021

AlessandroPatti commented Oct 18, 2021

philwo left a comment

Choose a reason for hiding this comment

philwo commented Oct 19, 2021

AlessandroPatti Oct 19, 2021 • edited

Choose a reason for hiding this comment

meteorcloudy Oct 20, 2021

Choose a reason for hiding this comment

coeuvre left a comment

Choose a reason for hiding this comment

coeuvre commented Nov 4, 2021 • edited

AlessandroPatti commented Nov 4, 2021

coeuvre commented Nov 5, 2021

coeuvre commented Nov 5, 2021

brentleyjones commented Nov 12, 2021

coeuvre commented Nov 15, 2021

werkt commented Nov 20, 2022

AlessandroPatti Oct 19, 2021 •

edited

coeuvre commented Nov 4, 2021 •

edited