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

zlib: add zstd support #52100

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

zlib: add zstd support #52100

wants to merge 3 commits into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Mar 15, 2024

Adds ZstdCompress and ZsdDecompress to the zlib module which can be used to compress/decompress with the Zstandard ("zstd") algorithm.

Notable omissions:

  • Providing dictionaries isn't implemented.
  • The docs in zlib.md don't call out any params beyond the basic compression level.

The code follows similar patterns to the PR that added Brotli support. Just that instead of brotli, it adds the equivalent zstd APIs. Just like Brotli, this required separate compression/decompression context objects.

Zstd itself has been around and stable for multiple years but this PR is early in terms of web support: It only just starts shipping by default in Chrome 123. On the other hand, by shipping in Chrome it will soon be supported quite widely on the web. Firefox also signaled support (https://bugzilla.mozilla.org/show_bug.cgi?id=1301878#c65). Zstd is now enabled on the web and supported by both Chrome and Firefox: https://caniuse.com/zstd

Official support in node.js would allow passing additional WPTs around fetch (nodejs/undici#2847).

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Mar 15, 2024
@jkrems jkrems force-pushed the zstd branch 4 times, most recently from ec19b6e to 28914c7 Compare March 17, 2024 00:07
@jkrems
Copy link
Contributor Author

jkrems commented Mar 17, 2024

Alright, got up to making the basics (seemingly) work:

$ out/Debug/node -p 'zlib.zstdDecompressSync(zlib.zstdCompressSync("Hello World")).toString()'
Hello World

@jkrems jkrems force-pushed the zstd branch 6 times, most recently from 383ff46 to 47c7ab8 Compare March 17, 2024 20:59
@jkrems jkrems marked this pull request as ready for review March 17, 2024 21:00
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
jkrems added a commit to jkrems/node that referenced this pull request Mar 17, 2024
@KhafraDev
Copy link
Member

cc @nodejs/zlib @nodejs/security-wg

@avivkeller avivkeller added the review wanted PRs that need reviews. label Sep 10, 2024
jkrems added a commit to jkrems/node that referenced this pull request Sep 10, 2024
jkrems added a commit to jkrems/node that referenced this pull request Sep 10, 2024
jkrems added a commit to jkrems/node that referenced this pull request Sep 10, 2024
jkrems added a commit to jkrems/node that referenced this pull request Sep 10, 2024
@jkrems
Copy link
Contributor Author

jkrems commented Sep 10, 2024

Rebased & updated zstd to the latest release (1.5.5 --> 1.5.6).

lib/zlib.js Outdated Show resolved Hide resolved

CompressionError ZstdCompressContext::Init(uint64_t pledged_src_size) {
pledged_src_size_ = pledged_src_size;
cctx_.reset(ZSTD_createCCtx());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cctx?

Suggested change
cctx_.reset(ZSTD_createCCtx());
ctx.reset(ZSTD_createCCtx());

Copy link
Contributor Author

@jkrems jkrems Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The c is for compression (cctx vs dctx). Given that we only ever deal with one or the other, we could drop that marker from the fields. But it seems worthwhile to match the naming used by zstd itself.

src/node_zlib.cc Outdated Show resolved Hide resolved
src/node_zlib.cc Outdated Show resolved Hide resolved
src/node_zlib.cc Outdated Show resolved Hide resolved
src/node_zlib.cc Outdated Show resolved Hide resolved
src/node_zlib.cc Outdated Show resolved Hide resolved
src/node_zlib.cc Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Sep 10, 2024

Can you also update license_builder?

@anonrig anonrig requested a review from jasnell September 10, 2024 16:03
@jkrems
Copy link
Contributor Author

jkrems commented Sep 10, 2024

Can you also update license_builder?

The changes to the top-level LICENSE file were generated by running ./tools/license-builder.sh. It's at the very end of the file changes in the Github UI.

@jkrems
Copy link
Contributor Author

jkrems commented Sep 10, 2024

Moved the init errors into C++ where we can add more context to them.

@jkrems jkrems force-pushed the zstd branch 2 times, most recently from 720bf6c to 7d18a7c Compare September 10, 2024 18:47
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (22ea302) to head (e9e1f0b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #52100    +/-   ##
========================================
  Coverage   87.89%   87.89%            
========================================
  Files         651      651            
  Lines      183364   183680   +316     
  Branches    35714    35756    +42     
========================================
+ Hits       161171   161453   +282     
- Misses      15465    15494    +29     
- Partials     6728     6733     +5     
Files with missing lines Coverage Δ
lib/internal/errors.js 94.92% <ø> (+<0.01%) ⬆️
lib/zlib.js 97.98% <ø> (-0.21%) ⬇️
src/node_errors.h 84.21% <ø> (ø)
src/node_metadata.cc 91.80% <ø> (+0.13%) ⬆️
src/node_zlib.cc 78.74% <ø> (+0.25%) ⬆️

... and 29 files with indirect coverage changes

@maxpain
Copy link

maxpain commented Nov 5, 2024

Hello. Any ETA for this?

@jasnell
Copy link
Member

jasnell commented Nov 5, 2024

@jkrems ... can you give this a rebase?

@jkrems
Copy link
Contributor Author

jkrems commented Nov 5, 2024

@jasnell I can but at this point I have spend quite bit of time on this PR without a clear signal on whether this would be likely to land (in this form or another). So it would be easier to justify if there was a tentative "yep, the high level approach looks good". By high level I mean something like "roughly following brotli integration in terms of API, using the facebook zstandard native implementation as the backing".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants