Conversation
|
I'm Bonk, and I've done a quick review of your PR. Bounds compression-stream output delivery and releases codec buffers incrementally.
|
a775d55 to
12d1c95
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7425 +/- ##
==========================================
+ Coverage 37.52% 37.54% +0.02%
==========================================
Files 836 837 +1
Lines 258430 258549 +119
Branches 23747 23785 +38
==========================================
+ Hits 96963 97084 +121
+ Misses 148229 148203 -26
- Partials 13238 13262 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
guybedford
left a comment
There was a problem hiding this comment.
Reviewed this layer (ts-streams-body-consumption-oom...ts-streams-decompression-delivery). Ran the compression suite (all variants) plus the new kj_test locally, all green, and probed the delivery state machine with a few extra cases (stale demandUnmet after releaseLock, BYOB min across writes, two BYOB reads pending at close, slow reader over many small writes) - no loss, duplication or ordering issues. The pull-driven delivery, BYOB fill-in-place and bounded default pieces all look right.
One design concern with where the bytes wait, detailed inline on compression.c++: this layer moves the readable's write-ahead backlog out of ArrayBuffer backing stores (inside the V8 sandbox, accounted as external memory) into plain C++ heap. The delivery mechanics can stay as they are; the storage should stay in the sandbox. The remaining comments are nits.
…pulled The CodecStage behind CompressionStream/DecompressionStream (both implementations) kept its output in a kj::Vector that doubled as it grew and never gave capacity back: decompressing a 256 MiB member held 384 MiB during the last doubling and 256 MiB of capacity for the rest of the stream's life, however long ago the output was pulled. The output now lives in blocks, one per pump iteration (at most the 16 KiB scratch), consumed from the front and freed as they are pulled. A burst of output costs its own size, with no growth copy, and is given back as it is consumed; the block ring itself is replaced once it has grown and emptied. For the C++ pair, decompressing 256 MiB of zeros peaks at 385 MiB RSS, down from 462 MiB, and takes 5.3 s instead of 6.4 s. CompressionCodec gains clear(), so the TypeScript pair can drop the buffered output when the stream is cancelled, aborted or errored, as the C++ frontend's cancelInternal does. compression-test.c++ drives CodecStage without an isolate: pulls of assorted sizes across block boundaries, output buffered across pushes and the flush tail, and clear().
…eces The TypeScript CompressionStream/DecompressionStream pair drained the codec's whole output into the readable after every write, as one chunk: 16 MiB of zeros gzipped to 16 KB came out as a single 16,777,216-byte chunk (C++: 4,096 chunks of 4 KiB), and every write's output was held twice at its peak, in the stage buffer and in the JS copy. Decompressing a 256 MiB member peaked at 661 MiB RSS against 462 MiB for C++, which is what a decompression bomb costs against the isolate's memory. The output now waits in the stage buffer and moves into the readable one read at a time: pull() fills a BYOB request's view in place, or enqueues a piece of at most 64 KiB for a default read. Nothing is queued ahead of demand (highWaterMark 0), so a BYOB view is filled to its size as in C++, rather than from a piece already queued. A pull that finds no output leaves its demand recorded, and the sink serves it as soon as a push or the flush tail produces some, so a read parked before the first write is still served by that write. close() closes the readable once the codec has ended and the stage is drained; cancel, abort and errors clear the stage. The codec still runs eagerly inside write(), so the error timing is unchanged; on a codec error one piece of the output produced before the error point goes to a waiting read before the pair errors, the order WPT decompression-extra-input pins. The 256 MiB decompression now peaks at 350 MiB RSS (1.1 s instead of 1.7 s), and no chunk exceeds 64 KiB through tee(), pipeThrough() or a Response body. Every measured shape is faster without the large allocations: arrayBuffer() of a 64 MiB output 494 -> 243 ms, a JS pipe 279 -> 202 ms, a body pumped by C++ 324 -> 259 ms. Pinned in the compression suite: delivery-shape.js (a 4 MiB single-write output read in bounded, byte-exact pieces; BYOB views filled to their size; two pending reads take consecutive pieces; tee branches get bounded pieces; trailing junk after a large output delivers one piece then errors, where C++ rejects the waiting read: ledger #16, #17) and draining-reader.js (a 1 MiB backlog is taken in 64 KiB pieces, the last with done).
12d1c95 to
8d99027
Compare
The TypeScript CompressionStream/DecompressionStream pair kept each write's output in the C++ stage buffer until reads pulled it out piece by piece. That put the readable's write-ahead backlog, unbounded by design, in plain C++ heap: outside the V8 sandbox, and with no external-memory adjustment on the stage's blocks, so it no longer counted against the isolate at all, where the ArrayBuffers the pair had queued before were accounted. The sink now drains the stage into the readable's queue before each write or close settles, as Uint8Arrays of at most 64 KiB, so the backlog waits in ArrayBuffers V8 accounts for and the stage holds output only within a single push or end. Chunks stay bounded, and a write's output is still never held whole in JS beside the stage's copy: the stage releases each block as it is pulled, so the peak is the output plus one piece. The readable needs neither a pull hook nor BYOB respond plumbing; the controller fills a BYOB read from the queued pieces. Each enqueue serves one waiting read, so every read waiting when a codec error strikes gets a piece of the output produced before it, which is what the trailing-junk test and ledger #17 now say. CompressionCodec loses clear(): the stage is empty between sink steps, and cancelling or erroring the readable drops the queued output. RingBuffer gains capacity() and shrinkToInitial(), which OutputBuffer uses in place of inferring growth from the element count. delivery-shape.js pins that two reads waiting at a trailing-junk error each get a consecutive piece, and states the intent of its piece-count check as sizes.length * sizes[0] == size (the 4 MiB output is an exact multiple of every implementation's piece); draining-reader.js pins that one draining read sweeps the whole 1 MiB backlog as its sixteen 64 KiB pieces.
| // either right after a codec step (stream readable) or is unreachable | ||
| // once the pair has failed or been canceled (the errored/canceled | ||
| // writable rejects writes before the sink hooks run). | ||
| const drainStage = (): void => { |
There was a problem hiding this comment.
My clanker review found something but this too esoteric for me to assess; lmk if this is plausible:
The new loop assumes that the readable cannot become canceled or errored between successive enqueues. That assumption is false in the actual stream implementation:
- byteControllerEnqueue() synchronously notifies the queue’s cursors.
- QueueCursor.notify() resolves a pending read () with an ordinary { value, done } object.
workerd/src/per_isolate/webstreams/queue.ts
Lines 770 to 790 in cc10f59
- Promise resolution synchronously looks up that object’s then property. An Object.prototype.then getter can call reader.cancel().
- Cancellation marks the controller done and invokes the compression pair’s cancel callback.
- drainStage() continues, pulls another piece, then attempts to enqueue into the canceled readable. That enqueue throws.
For output larger than the pieces already pulled, the remainder stays in the native stage. The cancel callback only clears input snapshots and errors the writable; the JS codec interface has no clear() operation. The in-flight write also receives an incidental enqueue TypeError. If this happens inside failCodec(), it skips failBoth(e) and replaces the original codec exception.
This is a specific reentrancy edge, not ordinary cancellation from a promise continuation. The repository already explicitly recognizes read-result then-getter reentrancy.
| output.truncate(validSize); | ||
| void CodecStage::OutputBuffer::write(kj::ArrayPtr<const kj::byte> chunk) { | ||
| if (chunk.size() == 0) return; | ||
| blocks.push_back(kj::heapArray(chunk)); |
There was a problem hiding this comment.
If there are lots of very tiny writes this could be inefficient. Clanker makes a big deal of this, but I suspect it's not worth trying to optimize for this.
compression.ts drained the codec's entire output into the readable after every write as one Uint8Array, so each write's output was held twice at its peak (C++ stage buffer + JS copy), and chunks were unbounded. The shared C++ CodecStage also kept its output in a doubling kj::Vector that never released capacity. Reproduced at 79bfba9: a 16 MiB member came out as one chunk (through tee, pipeThrough, bodies); 256 MiB peaked at 661 MiB RSS (C++ 462).