Support zstd decompression for external dependencies #11968

glukasiknuro · 2020-08-18T16:32:34Z

Bug: Add support for .tar.zst to http_archive

Handling of jni headers and loading system library is very similar to one already in bazel for native os library and possibly can be unified at some point.

Checked repo with big external deps, and noticed following improvements when checking bazel tracing profile:

size (tar.gz) |  extracting tar.gz  | extracting tar.zst
--------------------------------------------------------
    773M      |       13 s          |       3.2 s
    421M      |       13 s          |         5 s
    346M      |      9.2 s          |       3.3 s
    261M      |      7.5 s          |         3 s

One thing to check is whether bazel distributes binaries compiled with -c opt - personally tested with version compiled with this option.

googlebot · 2020-08-18T16:32:40Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

gjasny · 2020-08-18T22:37:03Z

@googlebot I consent

googlebot · 2020-08-18T22:37:20Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Looks adding zstd slightly increased current size and getting an error: Size of install_base is 281984 kB, expected it to be less than 281600 kB

- a number of C2036 fixes - void * arithmetic is not supported by MSVC - gcc compiler options not supported by MSVC

Possibly can be fixed upstream: ``` INFO: From Compiling third_party/zstd-jni/src/main/native/jni_zdict.c: third_party/zstd-jni/src/main/native/jni_zdict.c: In function 'Java_com_github_luben_zstd_Zstd_trainFromBuffer': third_party/zstd-jni/src/main/native/jni_zdict.c:55:12: warning: 'size' may be used uninitialized in this function [-Wmaybe-uninitialized] E1: return size; ^ third_party/zstd-jni/src/main/native/jni_zdict.c: In function 'Java_com_github_luben_zstd_Zstd_trainFromBufferDirect': third_party/zstd-jni/src/main/native/jni_zdict.c:89:12: warning: 'size' may be used uninitialized in this function [-Wmaybe-uninitialized] E1: return size; ^ ```

glukasiknuro · 2020-08-24T17:29:16Z

@meisterT could you review this PR or suggest how to help with proceeding with this change?

meisterT · 2020-08-31T11:30:10Z

Sorry for the delay, I was OOO.

What is the cost in terms of Bazel binary size with this change?

cc @meteorcloudy for the potential additional dependency

glukasiknuro · 2020-10-29T20:54:29Z

PTAL

Fixed windows support - System.loadLibrary under windows does not add "lib" in from of shared library name.
Loading from Runfiles is needed in tests.

Also upgraded zstd-jni to 1.4.5-11 which contains fixes for pointer arithmetic which allows the patch to be smaller (luben/zstd-jni#140). Would love to remove the patch for Native.java at all, but need to figure out how to do it so that zstd-jni will work as well.

Propagated changes to #12274, and tested under windows and in our internal codebase (@meteorcloudy could you double check it works for you as well?)

philwo · 2020-11-09T15:48:33Z

@glukasiknuro I'm importing this now. I will have to split it up into a couple of commits due to the changes to //third_party, don't be surprised. :)

Partial import of #11968. PiperOrigin-RevId: 341407243

Extracted from #11968, as I have to merge this separately from the rest of that PR.

Extracted from #11968, as I have to merge this separately from the rest of that PR. Partial commit for third_party/*, see #12437. Signed-off-by: Philipp Wollermann <[email protected]>

philwo · 2020-11-10T13:58:47Z

@glukasiknuro Two out of three parts are merged, I prepared a code review internally for the remaining changes - unfortunately I noticed that internally we appear to have a version of Apache Commons Compress without zstd support, so this doesn't compile when I build it in Google's monorepo. 😞 I'll have to figure out how to either upgrade our version of that library or change things so that our internal version of Bazel builds without the TarZstdFunction part (essentially disabling this feature just for our internal version).

philwo · 2020-11-10T16:11:06Z

@glukasiknuro Would it be an option to use https://github.com/airlift/aircompressor as an alternative to the JNI version?

glukasiknuro · 2020-11-10T18:34:44Z

@glukasiknuro Would it be an option to use https://github.com/airlift/aircompressor as an alternative to the JNI version?

Using aircompressor would be nice b/c it's in pure java.

Although looks like aircompressor does not support streaming (i.e. whole file needs to be decompressed at once) and it does not support bigendian machines because of UNSAFE usage (this maybe smaller issue). But let me know if you think those are non-blockers, i may try to use aircompressor.

BTW tangential effort is supporting zstd to compress blobs in remote cache (bazelbuild/remote-apis#168), I believe having zstd in some form would be needed for this effort as well. Looks like aircompressor implements only default zstd compression level, but this can be probably OK (@mostynb ?)

mostynb · 2020-11-10T22:21:10Z

BTW tangential effort is supporting zstd to compress blobs in remote cache (bazelbuild/remote-apis#168), I believe having zstd in some form would be needed for this effort as well. Looks like aircompressor implements only default zstd compression level, but this can be probably OK (@mostynb ?)

The REAPI compressed-blob bytestream proposal leaves the compression level up to the implementations, I don't think using a library that only supports the default compression level would cause any interoperability issues. But you probably want to at least experiment with the default level and the "fastest" level before committing to a particular library.

philwo · 2020-11-12T10:28:13Z

it does not support bigendian machines because of UNSAFE usage

That's unfortunately a blocker, as we don't want to break our s390x support. :/

I'm looking into merging the current version for now.

philwo · 2020-11-12T11:26:40Z

I'm sorry - I'm rolling back the partially merged PR 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 is too risky and will make it hard for distributions like Debian to accept.

Let's revisit this when we find a suitable pure Java version of Zstd.

*** Reason for rollback *** 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. *** Original change description *** Add @com_github_luben_zstd_jni repo to WORKSPACE. Partial import of #11968. PiperOrigin-RevId: 342024383

aignas · 2022-01-21T09:36:55Z

Hello all, what would be required to have this feature in bazel given that zstd compression/decompression feature was landed for the grpc remote cache functionality?

meteorcloudy · 2022-01-21T09:49:30Z

@glukasiknuro Can you try to rebase with Bazel@HEAD and see if you can already use the existing zstd-jni library?

This is mostly a rebase of bazelbuild#11968 with a few tweaks of my own. Fixes bazelbuild#10342. Co-authored-by: Grzegorz Lukasik <[email protected]> RELNOTES[new]: `repository_ctx.extract`, and thus `http_archive`, can now decompress zstandard-compressed archives.

This is mostly a rebase of #11968 with a few tweaks of my own. Fixes #10342. Co-authored-by: Grzegorz Lukasik <[email protected]> RELNOTES[new]: `repository_ctx.extract`, and thus `http_archive`, can now decompress zstandard-compressed archives. Closes #15026. PiperOrigin-RevId: 436146779

This is mostly a rebase of bazelbuild#11968 with a few tweaks of my own. Fixes bazelbuild#10342. Co-authored-by: Grzegorz Lukasik <[email protected]> RELNOTES[new]: `repository_ctx.extract`, and thus `http_archive`, can now decompress zstandard-compressed archives. Closes bazelbuild#15026. PiperOrigin-RevId: 436146779 (cherry picked from commit 00d74ff)

This is mostly a rebase of #11968 with a few tweaks of my own. Fixes #10342. Co-authored-by: Grzegorz Lukasik <[email protected]> RELNOTES[new]: `repository_ctx.extract`, and thus `http_archive`, can now decompress zstandard-compressed archives. Closes #15026. PiperOrigin-RevId: 436146779 (cherry picked from commit 00d74ff) Co-authored-by: Benjamin Peterson <[email protected]>

meteorcloudy · 2022-04-08T08:43:26Z

Closing due to #15026

googlebot added the cla: no label Aug 18, 2020

glukasiknuro mentioned this pull request Aug 18, 2020

Add support for .tar.zst to http_archive #10342

Closed

googlebot added cla: yes and removed cla: no labels Aug 18, 2020

glukasiknuro force-pushed the zstd branch 3 times, most recently from fe720d8 to 24b67dc Compare August 19, 2020 19:03

gjasny added 2 commits August 19, 2020 12:55

Update apache-commons-compress to version 1.19

4605991

Add zstd-jni 1.4.4-3

3c8250e

glukasiknuro force-pushed the zstd branch from 9bae5ac to d0712c2 Compare August 19, 2020 20:00

glukasiknuro and others added 13 commits August 19, 2020 13:01

Upgraded zstd-jni to v1.4.5-4

5f9a342

Add support for zstd decompression

f251551

Added TarZstdFunctionTest.java

520bd93

Changed extension of library to .so

12ed30e

Include zstd srcs in third_party:srcs

e88ca51

Support Mac builds

4c2ab40

Incrase minimal_jdk_size 275M -> 280M.

1915173

Looks adding zstd slightly increased current size and getting an error: Size of install_base is 281984 kB, expected it to be less than 281600 kB

Loading native library first using System and then from runfiles

e3b1262

Support windows builds

02e0515

- a number of C2036 fixes - void * arithmetic is not supported by MSVC - gcc compiler options not supported by MSVC

Put zstd-jni into deps of compress instead of runtime_deps.

fe0a256

Added native zstd lib to package-zip

0f2e250

Fixed name of native library

811d0b7

glukasiknuro force-pushed the zstd branch from d0712c2 to 811d0b7 Compare August 19, 2020 20:02

glukasiknuro marked this pull request as ready for review August 19, 2020 20:51

bazel-io pushed a commit that referenced this pull request Nov 9, 2020

Add @com_github_luben_zstd_jni repo to WORKSPACE.

5137f36

Partial import of #11968. PiperOrigin-RevId: 341407243

philwo added a commit that referenced this pull request Nov 9, 2020

Add zstd-jni to //third_party.

354f09e

Extracted from #11968, as I have to merge this separately from the rest of that PR.

philwo mentioned this pull request Nov 9, 2020

Add zstd-jni to //third_party. #12437

Closed

philwo added a commit that referenced this pull request Nov 9, 2020

Add zstd-jni to //third_party.

fb465cb

Extracted from #11968, as I have to merge this separately from the rest of that PR.

bazel-io pushed a commit that referenced this pull request Nov 9, 2020

Add zstd-jni to //third_party.

941fe0a

Extracted from #11968, as I have to merge this separately from the rest of that PR. Partial commit for third_party/*, see #12437. Signed-off-by: Philipp Wollermann <[email protected]>

aiuto added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Mar 31, 2021

benjaminp mentioned this pull request Sep 28, 2021

Add support for compression on gRPC cache #14041

Closed

philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021

philwo force-pushed the master branch from 26cb401 to 168b89b Compare December 2, 2021 18:07

thundergolfer mentioned this pull request Feb 8, 2022

feat: cpython toolchains bazelbuild/rules_python#618

Merged

12 tasks

benjaminp mentioned this pull request Mar 11, 2022

Support decompressing zstd tar archives for repository rules. #15026

Closed

brentleyjones mentioned this pull request Mar 21, 2022

[5.1] Support decompressing zstd tar archives for repository rules. #15087

Merged

meteorcloudy closed this Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support zstd decompression for external dependencies #11968

Support zstd decompression for external dependencies #11968

glukasiknuro commented Aug 18, 2020 •

edited

Loading

googlebot commented Aug 18, 2020

gjasny commented Aug 18, 2020

googlebot commented Aug 18, 2020

glukasiknuro commented Aug 24, 2020

meisterT commented Aug 31, 2020

glukasiknuro commented Oct 29, 2020

philwo commented Nov 9, 2020

philwo commented Nov 10, 2020

philwo commented Nov 10, 2020

glukasiknuro commented Nov 10, 2020 •

edited

Loading

mostynb commented Nov 10, 2020

philwo commented Nov 12, 2020

philwo commented Nov 12, 2020

aignas commented Jan 21, 2022

meteorcloudy commented Jan 21, 2022

meteorcloudy commented Apr 8, 2022

Support zstd decompression for external dependencies #11968

Support zstd decompression for external dependencies #11968

Conversation

glukasiknuro commented Aug 18, 2020 • edited Loading

googlebot commented Aug 18, 2020

gjasny commented Aug 18, 2020

googlebot commented Aug 18, 2020

glukasiknuro commented Aug 24, 2020

meisterT commented Aug 31, 2020

glukasiknuro commented Oct 29, 2020

philwo commented Nov 9, 2020

philwo commented Nov 10, 2020

philwo commented Nov 10, 2020

glukasiknuro commented Nov 10, 2020 • edited Loading

mostynb commented Nov 10, 2020

philwo commented Nov 12, 2020

philwo commented Nov 12, 2020

aignas commented Jan 21, 2022

meteorcloudy commented Jan 21, 2022

meteorcloudy commented Apr 8, 2022

glukasiknuro commented Aug 18, 2020 •

edited

Loading

glukasiknuro commented Nov 10, 2020 •

edited

Loading