Skip to content

src: refactor compression allocation tracking, enable for zstd#61717

Open
addaleax wants to merge 4 commits intonodejs:mainfrom
addaleax:zstd-allocation-tracking
Open

src: refactor compression allocation tracking, enable for zstd#61717
addaleax wants to merge 4 commits intonodejs:mainfrom
addaleax:zstd-allocation-tracking

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Feb 7, 2026

src: release memory for zstd contexts in Close()

This aligns zstd streams with other compression libraries in this regard,
and enables releasing memory early when the stream ends in JS instead of
waiting for GC to clean up the wrapper object (which is a problem that
is exacerbated in the zstd context because we do not track memory
and report memory pressure to V8 yet).

src: extract zlib allocation tracking into its own class

This makes it a bit easier to separate concerns, and results in
reduced code duplication when compiling since this code does not
depend on template parameters.

src: do not store compression methods on Brotli classes

This addresses a long-standing TODO comment, referencing the fact
that these values are either known at compile time or can be
inferred from the this value in the context class.

src: track allocations made by zstd streams

This is both valuable as a diagnostic tool and as a way to inform
the JS runtime about external allocations.

Currently, this is a feature only enabled in the statically linked
builds of zstd, so with --shared-zstd, we fall back to the
non-tracking variant.

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. review wanted PRs that need reviews. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Feb 7, 2026
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Feb 7, 2026
This aligns zstd streams with other compression libraries in this
regard, and enables releasing memory early when the stream ends in
JS instead of waiting for GC to clean up the wrapper object (which
is a problem that is exacerbated in the zstd context because we
do not track memory and report memory pressure to V8 yet).
This makes it a bit easier to separate concerns, and results in
reduced code duplication when compiling since this code does not
depend on template parameters.
This addresses a long-standing TODO comment, referencing the fact
that these values are either known at compile time or can be
inferred from the `this` value in the context class.
This is both valuable as a diagnostic tool and as a way to inform
the JS runtime about external allocations.

Currently, this is a feature only enabled in the statically linked
builds of zstd, so with `--shared-zstd`, we fall back to the
non-tracking variant.
@addaleax addaleax force-pushed the zstd-allocation-tracking branch from c1e9728 to 5110ea4 Compare February 7, 2026 13:08
@codecov
Copy link

codecov bot commented Feb 7, 2026

Codecov Report

❌ Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.72%. Comparing base (3ab9dd8) to head (5110ea4).

Files with missing lines Patch % Lines
src/node_zlib.cc 92.64% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61717      +/-   ##
==========================================
- Coverage   89.73%   89.72%   -0.01%     
==========================================
  Files         675      675              
  Lines      204502   204516      +14     
  Branches    39304    39303       -1     
==========================================
+ Hits       183502   183510       +8     
- Misses      13283    13292       +9     
+ Partials     7717     7714       -3     
Files with missing lines Coverage Δ
src/node_zlib.cc 78.43% <92.64%> (+0.47%) ⬆️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants