TS-backed compression streams and compression context consolidation - #7075
TS-backed compression streams and compression context consolidation#7075jasnell wants to merge 13 commits into
Conversation
jasnell
changed the title
911a2b4 to
0412dfd
Compare
|
@jasnell Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bonk roast this pr |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| // not granted here; WPT pins the rejection). Captured getters are used for | ||
| // the view's buffer — prototype accessors are user-patchable. | ||
| function isValidChunk(chunk: unknown): boolean { | ||
| if (isArrayBuffer(chunk)) return true; |
There was a problem hiding this comment.
[P1] isArrayBuffer() accepts detached buffers (and a detached view gets through the backing-buffer check). CompressionCodec::push() then calls JsBufferSource::asArrayPtr(), which returns an empty {nullptr, 0} span for detached storage, so writer.write(detachedBuffer) succeeds as an empty chunk instead of rejecting the pair with TypeError. Check ArrayBufferPrototypeDetachedGet for both direct ArrayBuffers and the extracted view backing buffer, and add a regression test that transfers the buffer before writer.write().
|
Posted 1 inline P1 finding. Targeted tests could not build: local Clang lacks standard-library headers and Time for a pun! This stream is a little too detached from its BufferSource. |
0412dfd to
6e4a5e9
Compare
Merging this PR will improve performance by 13.23%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
6e4a5e9 to
de681c0
Compare
de681c0 to
4121e9a
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Reviewed commit-by-commit, scoped to the new work (the stacked #7073 commits here are byte-identical to what I approved there, modulo the additive AGENTS.md/BUILD rows). The consolidation shape is right: ZlibStream core + CodecStage (spec policy, eager pacing) + the module-private CompressionCodec handle, with node:zlib's ZlibContext re-hosted on the core and its Node-fidelity surface kept local, reaching the structure through core.raw() where no helper applies. I traced the zlib-util refactor hunk-by-hunk and it is faithful — including the resetStream consolidation, which is safe since initializeZlib() precedes every core.reset() so the core's initialized assert is unreachable. The TS pair's deliberate semantics (transform-time error timing, legacy-parity settlement, BYOB-capable readable) are clearly documented and pinned by both the wd-test and the WPT registration, and the GetApiSymbol liftKj fix removes a real process-crash edge.
One intent question and a few nits, inline.
This review was written with AI assistance and may contain mistakes; treat each finding on its merits.
| // Deliver output the codec produced before the error point (e.g. the | ||
| // final valid bytes preceding trailing junk) to any pending read, then | ||
| // error. The WPT-pinned order: output first, error on later reads. | ||
| drainStage(); |
There was a problem hiding this comment.
[QUESTION] Error-time output delivery diverges from the legacy frontend here: legacy's cancelInternal → stage.clear() drops all buffered output and rejects pending reads, while this catch delivers the pre-error bytes to any pending read (drainStage() fulfills it synchronously via the enqueue) before erroring. Only observable with a read pending at the error moment — but the comment's "WPT-pinned order" justification isn't fully CI-pinned: the WPT config runs without strict_compression_checks, so the strict-path variant of this ordering has no WPT coverage. Deliberate divergence, or should the TS pair match legacy clear-on-error?
| wpt_directory = "@wpt//:streams@module", | ||
| ) | ||
|
|
||
| # TODO(streams-ts): registration pending an investigation: the suite's large-file |
There was a problem hiding this comment.
[NIT] This TODO reads as "registration pending an investigation" — but the registration is right below it and CI-green. If start_server = True was the fix for the /media/ fetch failures, the note is stale and should be dropped (or reworded to describe whatever actually remains under investigation).
|
|
||
| kj::ArrayPtr<kj::byte> CodecStage::LazyBuffer::take(size_t readSize) { | ||
| KJ_ASSERT(readSize <= validSize); | ||
| kj::ArrayPtr<kj::byte> chunk = kj::arrayPtr(&output[output.size() - validSize], readSize); |
There was a problem hiding this comment.
[NIT] take(0) computes &output[output.size() - validSize], which indexes one past the end of the vector when the valid region is empty. Safe today only because pull() guards n == 0 one caller up — the bounds-safety belongs in take() itself (an early if (readSize == 0) return nullptr;) so future callers can't reintroduce the OOB index.
| } | ||
|
|
||
| } // namespace workerd::api | ||
| } // namespace workerd::api No newline at end of file |
There was a problem hiding this comment.
[NIT] Lost the trailing newline at EOF.
4121e9a to
dd5c64c
Compare
workerd has three JavaScript-facing compression surfaces -- the web
CompressionStream/DecompressionStream pair, node:zlib, and (upcoming)
the TypeScript streams implementation's codec -- whose shared machinery
is being consolidated under api/compression.{h,c++}. Start with the one
piece the first two already share: CompressionAllocator moves out of
streams/compression.h (which node included for it) into the new
library, unchanged.
ZlibStream owns the z_stream structure, its init/reset/end lifecycle, the input/output buffer plumbing, and the raw deflate()/inflate() step, plus the shared helpers both existing consumers had duplicated: the zlib error-code name table and the web-format windowBits mapping. Mechanism only -- consumers interpret the returned codes under their own policies, and consumer-specific zlib features (dictionaries, deflateParams) reach the structure through raw() until they grow shared consumers. The web pair's Context becomes that policy layer: the spec-pinned TypeErrors and strict_compression_checks handling over the shared core, with its z_stream lifecycle and windowBits table deleted.
Ported from the streams pipeline-optimization branch (slice C of the
compression design): the canonical unit of the web compression pairs is
a synchronous codec stage -- eager push accumulating output in the
stage's own buffer, Z_FINISH plus the strict end checks on end(),
pull/available/clear for the drain side -- with ALL asynchrony owned by
whichever frontend wraps it. Eager pacing is load-bearing: the spec
runs the codec inside transform()/flush(), so corrupt input must reject
the write and a strict-mode incomplete stream must reject the close,
timing that is WPT-pinned on every frontend where writes settle.
The stage lives in api/compression.{h,c++} because it is about to gain
a second frontend (the TypeScript streams implementation's pair); the
web policy layer (Context) and the LazyBuffer move inside it as private
details. The legacy frontend collapses from the templated
CompressionStreamBase/Impl/Adapter trio into one runtime-moded
CompressionStreamImpl -- a thin promise adapter (pending-read ring,
canceler, lifecycle state machine) -- plus the de-templated adapter,
with codec exceptions funneled through one runCodec() helper preserving
the historical teardown-then-rethrow path.
Fixes the custom-arrayptr-first-copyfrom clang-tidy finding: the ported code predates the lint (ArrayPtr::write was adopted on the source branch separately).
ZlibContext keeps everything Node-specific -- the mode bookkeeping (including the UNZIP gzip-sniffing mode reassignment and the multi-member gunzip loop), dictionaries, deflateParams, the lazy-init discipline, and the CompressionError surface -- while the z_stream ownership, init/reset/end lifecycle, buffer plumbing, and codec step move to the shared core, reached through core.raw() where no helper applies. The duplicated error-code name table (ZlibStrerror) is replaced by the core's, and the unused getStream() accessor is dropped. The historical windowBits mode adjustments (+16 gzip, +32 unzip, negated raw) stay here: they are mode policy, applied before handing the final value to the core.
The ZlibMode enum, CompressionError, and the BrotliContext and
ZstdContext families (with their encoder/decoder subclasses and JSG
options structs) move from node:zlib into api/compression.{h,c++},
unchanged. They follow Node.js' structure -- node:zlib is their only
consumer today -- but they are compression machinery, not Node
bindings: CompressionStream/DecompressionStream are anticipated to grow
brotli and zstd format support, at which point those formats gain web
frontends over these same contexts.
Ported from the streams pipeline-optimization branch (slice E1's exposure decision): CompressionCodecHandle is a SelfConvertible value type whose jsgWrap builds a plain object of four jsg::Functions (push/end/pullInto/available) sharing one refcounted CodecStage box -- no isolate-type registration, no new global, GC lifetime and external-memory accounting through normal jsg machinery. All methods are synchronous and IoContext-free: compression is pure CPU, so the TypeScript pair's waiting is entirely V8 promise machinery, and global-scope construction is legal. The factory is a static on the existing CompressionStream class, registered only under typescript_implemented_streams: the per-isolate bootstrap captures it at module load, before main.ts replaces the global with the TypeScript class, so user code never observes it. (Adapted from the branch: jsg::BufferSource is jsg::JsBufferSource here.)
The codec factory is bootstrap plumbing, not API surface: rather than a flag-gated static smuggled onto the standard CompressionStream class (the ported design), inject it as utils.newCompressionCodec alongside getApiSymbol and the type checks. The handle becomes a proper internal JSG resource type (CompressionCodec, excluded from generated types and registered as neither a global nor a nested type, so instances are unreachable except by the bootstrap module holding them), allocated from the raw utils callback via the type-handler lookup -- legal at call time, when the isolate is fully set up. The callback body lives in api/compression.c++ so the compression knowledge stays with the machinery; the bootstrap wires only the name. The raw callback runs under jsg::liftKj, which converts the validation TypeErrors into JS exceptions and sets the returned wrapper as the callback result.
Ported from the streams pipeline-optimization branch and adapted to this substrate: the pair is a JS writable sink feeding the synchronous codec handle (utils.newCompressionCodec) plus a queued byte-capable readable that the sink's drains enqueue into -- the branch hosts that half on its native stage-source kit, which arrives with the future fusion work. Semantics carried forward: eager push (corrupt input rejects the write, strict-mode incomplete streams reject the close -- the spec's transform/flush error timing), legacy-parity settlement (writes never wait for reads), byte-capable BYOB readable, and both-sides error propagation mirroring the legacy cancelInternal. main.ts replaces the globals under typescript_implemented_streams. The behavioral test file (also ported, plus new large multi-pump roundtrip coverage) pins the semantics. The compression WPT config for the TypeScript pair is included but its registration is held back: the suite's large-file fetches fail under the flag with a connection loss in the fetch/serving path -- the same payload sizes round-trip in-worker, so this is a pre-existing streams issue to run down separately, not a codec one.
The raw v8 callback threw its validation TypeError as a kj exception with no liftKj to convert it, so a non-string argument would have taken down the process rather than throwing. The failure path was never exercised (bootstrap-internal callers always pass literals), but the newCompressionCodec callback hit the identical latent pattern the moment it gained reachable validation, so fix this one the same way.
The held-back registration's failures were self-inflicted: the entry was missing start_server (the suite's large-file fetches are served by the WPT sidecar over real HTTP, so without it they died with connection refused, surfacing as 'Network connection lost') and the Windows-incompatible select, both present on the legacy entry it was copied from. There was never a streams issue: the same payload sizes round-trip in-worker, and with the sidecar running the whole suite passes. One expectation delta from the legacy configuration, in the TypeScript pair's favor: the idlharness interface-prototype subtests that fail against the legacy classes pass against the TypeScript pair, whose prototype property attributes follow the IDL rules.
… pair Three fixes and one root-caused documentation correction: SharedArrayBuffer chunks: the pair's write validation admitted SAB-backed views; it now rejects SharedArrayBuffers and views over them (captured buffer getters, mirroring the identity transform's validation), erroring both sides with the BufferSource TypeError. Trailing-junk output delivery: WPT pins that the final decompressed bytes reach an already-pending read before the trailing-data error surfaces. The pump iteration that observes trailing junk is the same one that produces the stream's final bytes, and the strict check threw before the stage buffered them. CodecStage now buffers each iteration's output before applying the strict checks (split out of pumpOnce as enforceStrictChecks), and the pair's sink drains the stage before erroring the readable in its catch paths. The legacy frontend's behavior is unchanged: its error path tears down the stage and pending reads, so its expectations stay as they were. compression-bad-chunks: enabled for the TypeScript pair (it was disabled wholesale against the legacy classes); only the brotli-constructor cases remain expected failures until brotli format support lands. decompression-with-detach: root-caused as environmental, in both configs' comments now: the compression variant of the test runs first in the same isolate and installs its Object.prototype.then trap without configurable, so this test's identical defineProperty throws before the body runs. Browsers give each test file a fresh global; the shared-isolate harness cannot, and the non-configurable leftover cannot be deleted between files.
dd5c64c to
2f1e828
Compare
2 participants
Consolidation of compression state internals and implementation of TS-backed CompressionStream.
Best to review commit-by-commit. Draft while I perform another review pass myself.
Stacks on #7073