Repository navigation
perf: stop splitting decompressor input into 16 KiB writes - #30
Merged
Merged
Conversation
The split capped WebKit's output chunks, since WebKit inflates a whole input write into one chunk, at about 16 MiB instead of 64 MiB. That only matters for near-1032:1 data such as zip bombs or long runs of zeros, and the size check already rejects an oversized chunk before it reaches the caller. Meanwhile the extra writes slow decompression of incompressible data by up to 2x in Node, Chromium, Firefox and WebKit, and large realistic entries by about 5-10% end to end. Reads are already at most 64 KiB, the queue's high-water mark, so that remains the bound on each write.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 65.30% 65.45% +0.15%
==========================================
Files 16 16
Lines 709 715 +6
Branches 205 205
==========================================
+ Hits 463 468 +5
- Misses 165 166 +1
Partials 81 81 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Node before 24.19 counts each chunk written to DecompressionStream as size 1 against a 16 KiB high-water mark, so pipeThrough read an entry's entire compressed data ahead of the reader (nodejs/node#62986). A 64 MiB entry held 64 MiB after reading one chunk. Feed the decompressor one awaited write at a time instead of using pipeThrough, which bounds read-ahead to a few chunks on every runtime. The decompress-node contract test now checks that an abort reason reaches the reader as the same object, which the upstream error mapping relies on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Entry data was split into 16 KiB writes before decompression to cap WebKit's output chunks, since WebKit inflates a whole input write into one chunk. That capped a single chunk at about 16 MiB instead of 64 MiB, which only matters for data near deflate's 1032:1 limit such as zip bombs or long runs of zeros, and the size check already rejects an oversized chunk before it reaches the caller. Rather than work around a WebKit limitation, this removes the split. Reads from the source are already at most 64 KiB, the queue's high-water mark, so that remains the bound on each write.
The split had a real cost. Decompressing 10 MB of incompressible data with 16 KiB writes was 1.7–2.2x slower in Chromium, Firefox and WebKit and about 55% slower in Node, and text was up to 23% slower. On the realistic large and medium benchmarks in #29 it is about 5–10% end to end. The earlier benchmarks in #26 didn't show this because their fixture compressed about 300:1, so entries were too small to be split.
The test that bounded the size of a decompressed chunk mostly checked engine behaviour: Node, Chromium and Firefox emit small chunks regardless of what zip-reader does. It is replaced by a test of what zip-reader controls, using a source that records reads: entry data is read in slices of at most 64 KiB, and only a few slices are read ahead of the consumer rather than the whole entry.
The new test failed on Node 22, and on main's code too, which exposed an existing problem: Node before 24.19 counts each chunk written to
DecompressionStreamas size 1 against a 16 KiB high-water mark (nodejs/node#62986), sopipeThroughread a deflated entry's entire compressed data into memory ahead of the reader. Entries are now fed to the decompressor one awaited write at a time, which bounds read-ahead to a few chunks on every runtime with no measurable cost. A comment links the Node fix, so the workaround can go when Node 22 support is dropped.