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.

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

Add zstd support #9316

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Add zstd support #9316

merged 3 commits into from
Feb 7, 2025

Conversation

garazdawi
Copy link
Contributor

This PR adds a module to stdlib that wraps the Zstandard library. Zstandard is a compression library that allows for faster compression than zlib, but with about the same compression ratio.

Zstandard is vendored into erts, allowing us to also use it inside the VM for future optimizations.

The API is still a bit of a work in progress, but any opinions about what things should be named are very welcome. I think that I may rename zstd:chunk/2 to zstd:stream/2 and also possible break zstd:contex/1,2 into separate functions for compression and decompression context, but I've not decided yet.

This PR depends on #9315 for the doctests support.

@garazdawi garazdawi added team:VM Assigned to OTP team VM feature labels Jan 20, 2025
@garazdawi garazdawi added this to the OTP-28.0 milestone Jan 20, 2025
@garazdawi garazdawi requested a review from sverker January 20, 2025 11:53
@garazdawi garazdawi self-assigned this Jan 20, 2025
Copy link
Contributor

github-actions bot commented Jan 20, 2025

CT Test Results

    4 files    227 suites   1h 54m 56s ⏱️
3 659 tests 3 565 ✅  93 💤 1 ❌
4 753 runs  4 636 ✅ 116 💤 1 ❌

For more details on these failures, see this check.

Results for commit 71de6c2.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi garazdawi force-pushed the lukas/stdlib/zstd branch 2 times, most recently from fff3661 to c1feba5 Compare January 20, 2025 15:05
@essen
Copy link
Contributor

essen commented Jan 21, 2025

The API is still a bit of a work in progress, but any opinions about what things should be named are very welcome. I think that I may rename zstd:chunk/2 to zstd:stream/2 and also possible break zstd:contex/1,2 into separate functions for compression and decompression context, but I've not decided yet.

Please keep the naming close to the original API (zstd.h and friends), it's easier to translate other examples to Erlang that way, plus all the documentation from zstd.h becomes usable. For example if I search zstd.h for "chunk" I find nothing relevant, since it's called "stream" there.

@crertel
Copy link

crertel commented Jan 21, 2025

Just wanted to say thanks for this!

@garazdawi
Copy link
Contributor Author

Updated the API to be zstd:stream/2 and zstd:dict/2. You can view the docs here: https://erlang.github.io/prs/9316/lib/stdlib-6.2/doc/html/zstd.html

@essen
Copy link
Contributor

essen commented Jan 22, 2025

Only one tiny comment following a quick look at the docs: all functions are verbs except context and dict. That's probably fine, although in my mind if there's no verb the function isn't going to do much work (like a getter), which definitely isn't the case for dict.

@williamthome williamthome mentioned this pull request Jan 22, 2025
@garazdawi garazdawi force-pushed the lukas/stdlib/zstd branch 3 times, most recently from 680c6f7 to 8bf306e Compare January 30, 2025 09:24
@garazdawi
Copy link
Contributor Author

@essen John mentioned that in the past you have used zlib:deflate/3 from multiple processes, are you still doing that and if so, do you imagine you would like to do the same with zstd?

@essen
Copy link
Contributor

essen commented Feb 5, 2025

@essen John mentioned that in the past you have used zlib:deflate/3 from multiple processes, are you still doing that and if so, do you imagine you would like to do the same with zstd?

Yes the deflateInit is done in one process and the actual deflate in another for Websocket. It's still like this today. Cowboy creates the contexts and then calls zlib:set_controlling_process.

Websocket has no zstd compression extension RFC/draft so today this isn't necessary for zstd. I guess for deflate the init could be deferred as well, but it's just more convenient to fully prepare deflate while doing the Websocket negotiation.

@garazdawi
Copy link
Contributor Author

Allright, then I will keep the contexts process bound for now and if the need arises later we can add that functionality. Thanks for the prompt response!

@garazdawi garazdawi force-pushed the lukas/stdlib/zstd branch 2 times, most recently from 71de6c2 to ecb4afc Compare February 7, 2025 08:44
rickard-green
rickard-green previously approved these changes Feb 7, 2025
@garazdawi garazdawi merged commit a3571a9 into erlang:master Feb 7, 2025
7 of 9 checks passed
@garazdawi garazdawi deleted the lukas/stdlib/zstd branch February 7, 2025 10:52
@essen
Copy link
Contributor

essen commented Feb 7, 2025

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants