doc: add missing Zstd strategy constants #59312
Conversation
nodejs-github-bot
added
doc
jasnell
added
author ready
doc/api/zlib.md
Outdated
| @@ -750,6 +750,34 @@ The most important options are: | |||
| * `ZSTD_c_compressionLevel` | |||
| * Set compression parameters according to pre-defined cLevel table. Default | |||
| level is ZSTD\_CLEVEL\_DEFAULT==3. | |||
| * `ZSTD_c_strategy` | |||
| * Select the compression strategy. Default is `ZSTD_lazy2`. | |||
There was a problem hiding this comment.
I am just curious to know from where is coming this default strategy.
I have found this: https://github.com/nodejs/node/blob/main/deps/zstd/lib/zstd.h#L355-L364
The compression level (default: 3) dynamically determines compression parameters, including the strategy, unless ZSTD_c_strategy is explicitly set. This implies that the default strategy is tied to the default compression level (ZSTD_CLEVEL_DEFAULT == 3), which typically maps to ZSTD_greedy
There was a problem hiding this comment.
This is what internet 📡 saying:
When using the zstd command, you can specify different compression levels, and zstd automatically selects a strategy based on the level. Levels 1 and 2 use faster strategies, while level 3 and above default to
ZSTD_greedy.
But I cannot find any source nor doc to prove it 🤷
There was a problem hiding this comment.
I guess, I found something -> https://github.com/facebook/zstd/blob/dev/lib/compress/clevels.h 🔍
There was a problem hiding this comment.
It is explained how it works here:
- https://github.com/nodejs/node/blob/main/deps/zstd/lib/zstd.h#L355-L364
- https://facebook.github.io/zstd/doc/api_manual_latest.html#Chapter5
I think this line specifing a default should be omited.
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/59312 ✔ Done loading data for nodejs/node/pull/59312 ----------------------------------- PR info ------------------------------------ Title doc: add missing Zstd strategy constants (#59312) Author RANDRIAMANANTENA Narindra Tiana Annaick <[email protected]> (@Annaick, first-time contributor) Branch Annaick:doc-zstd-strategy-constants -> nodejs:main Labels doc, zlib, author ready Commits 2 - doc: add missing Zstd strategy constants - doc: omited default strategy for zstd_c_strategy Committers 1 - annaick <[email protected]> PR-URL: https://github.com/nodejs/node/pull/59312 Fixes: https://github.com/nodejs/node/issues/59290 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/59312 Fixes: https://github.com/nodejs/node/issues/59290 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - doc: add missing Zstd strategy constants ⚠ - doc: omited default strategy for zstd_c_strategy ℹ This PR was created on Fri, 01 Aug 2025 10:27:31 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/59312#pullrequestreview-3079463740 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/59312#pullrequestreview-3079898823 ✘ Last GitHub CI failed ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/16704072824 |
|
Hope, it can land 🛬 already ⌛ |
|
CQ failed due to the latest commit pushed from the author didn't go through GHA, I have kicked started the GHA run, once its ready it will need another approval and then it can land. |
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in ef58be6 |
PR-URL: #59312 Fixes: #59290 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Documents the ZSTD_c_strategy parameter and its possible values (ZSTD_fast … ZSTD_btultra2) that were already exposed in zlib.constants but missing from the API docs and added an example for reference.
Fixes: #59290