TextEncoderStream native-sink: write straight to the sink's write_latin1/write_utf16 - #36877
TextEncoderStream native-sink: write straight to the sink's write_latin1/write_utf16#36877robobun wants to merge 2 commits into
Conversation
…/write_utf16 When a native JSSink is attached, TextEncoderStreamEncoder__encodeIntoSink now passes the chunk's WTFStringImpl bytes straight to the sink via the sink's own write_latin1/write_utf16 instead of first transcoding into a per-encoder scratch Vec<u8> and then calling write_bytes. Every sink's write_latin1 already has an is_all_ascii fast path that forwards the input slice verbatim, so an all-ASCII 8-bit chunk is now zero-copy from the JSString all the way to the sink buffer (or the socket, for HTTPServerWritable with no buffered data). For 16-bit chunks the encoder trims a trailing lone lead surrogate into pending_lead_surrogate and hands the remainder to write_utf16; every sink's write_utf16 emits U+FFFD for mid-chunk lone surrogates exactly as the encoder did. Only a chunk that follows a carried lead surrogate still goes through the scratch buffer, so the prefix and body produce one Writable. SinkHandle grows write_latin1/write_utf16 that dispatch to each sink's implementation (RewriterPipe gains &self inherent methods so the shared-BackRef HTMLRewriter arm can reach them). The coerced JSString is now rooted across the sink write (to_js_string + EnsureStillAlive, as in JSSink::js_write), since HTMLRewriter content handlers and ArrayBufferSink's onReady are GC points. Tests: native-sink coverage for 8-bit (ASCII + Latin-1 non-ASCII) and 16-bit (mid-chunk + cross-chunk surrogate) chunks, an HTMLRewriter sink with ToString-coerced chunks and Bun.gc(true) inside the handler, and readable-side + native-sink backpressure tests for TextEncoderStream and readable-side backpressure tests for TextDecoderStream.
|
Verified locally: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesNative sinks now support direct Latin-1 and UTF-16 writes. Native encoding sink writes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/encoding/text-encoder-stream.test.ts`:
- Around line 361-375: Update waitUntilStable to use a named maximum
event-loop-turn limit instead of an unbounded loop with arbitrary Bun.sleep
timing, and return whether stability was reached before the limit. At the call
sites around the stalled-client checks, assert the helper reports success before
recording pullsWhileStalled, while preserving the existing total threshold
behavior.
- Around line 336-358: Extend the 8-bit coercion case in the test
"TextEncoderStream -> HTMLRewriter native sink: non-string chunks + handler that
allocates" with a non-ASCII Latin-1 character, and update the expected output
assertion to include that character unchanged. Keep the existing ASCII and
UTF-16 coercion cases and element-count assertion intact.
- Around line 425-428: Update the tests at
test/js/web/encoding/text-encoder-stream.test.ts:425-428 and
test/js/web/encoding/text-decoder-stream.test.ts:285-288 to store each
writer.close() promise, drain the reader as currently done, then await the
stored promise so close rejections fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c77d68f8-9315-4a61-8041-452dfbcc658b
📒 Files selected for processing (5)
src/runtime/api/html_rewriter.rssrc/runtime/webcore.rssrc/runtime/webcore/TextEncoderStreamEncoder.rstest/js/web/encoding/text-decoder-stream.test.tstest/js/web/encoding/text-encoder-stream.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the trailing-lone-lead trim in the new 16-bit fast path is correct for the [lead], [lead, lead], and [..., lead, trail] edge cases (u16_is_lead is ICU U16_IS_LEAD, so a last-position lead is always unpaired); and the new SinkHandle::write_latin1/write_utf16 match SinkHandle::write arm-for-arm. Verifiers also examined whether the new 16-bit surrogate test's ref compares encodeIntoSink against itself and concluded it does not — and the WPT testCases block anchors the surrogate semantics against hardcoded bytes via the non-sink encodeForStream path regardless.
Extended reasoning...
This is a ~200-line native hot-path change that restructures an extern "C" entry point, adds new unsafe dispatch arms on SinkHandle, changes the GC-rooting strategy (get_zig_string → to_js_string + EnsureStillAlive), and relies on a cross-sink semantic-equivalence claim (every sink's write_utf16 replaces lone surrogates identically to the encoder). It looks correct and is well-tested, but it is not a mechanical change — deferring to human review.
…ressure, fix SAFETY comment - Parameterize the native-sink round-trip tests (8-bit, 16-bit, HTMLRewriter) over two source shapes: start() enqueues everything up front, and async pull with a macrotask yield so each chunk goes through encodeIntoSink while the response is streaming. - Replace the time-polled native-sink backpressure test with a writer-driven one: write 256 KiB chunks until a write stays pending across a macrotask (Bun.peek.status), then drain the client and assert the parked write resolves and the byte count matches. The previous version also had a writer<->fetch deadlock because fetch() waits for the first body bytes. - Readable-side backpressure tests (encoder + decoder) now use Bun.peek.status(second) instead of racing against Bun.sleep(0). - Await writer.close() after draining so a close rejection fails the test. - Add a non-ASCII Latin-1 chunk to the HTMLRewriter rooting test so the RewriterPipe write_latin1 Vec-copy branch is also covered. - Reword the SAFETY comment on the encoder &*this borrow: the sink write can run user JS; the soundness argument is shared-borrow + Cell/RefCell, not absence of user JS.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Chain benchmarksRelease build, 5-run median, 200 x 64 KiB chunks x 30 iterations per run,
The HTTP/rewriter/gzip stages dominate these numbers; the actual change is one scratch allocation + one memcpy eliminated per chunk (or two memcpys for the ascii case when The |
|
Note on fail-before: this change moves which sink entrypoint the encoder calls ( |
There was a problem hiding this comment.
No issues found on the updated commit; deferring so Jarred can confirm the reworked tests and the four open comment-cop notes on TextEncoderStreamEncoder.rs.
What was reviewed:
- Trailing-lead-surrogate trim before
write_utf16: verified it matchesencode_utf16_into's carry semantics for[lead],[…, lead, lead],[…, lead, trail], and[trail]; the 16-bit sink test diffs against the non-sink reference path. to_js_string+EnsureStillAliverooting across sink writes that re-enter user JS (HTMLRewriter handler +Bun.gc(true)test covers the ToString-coerced case).SinkHandle::write_latin1/write_utf16arms mirrorSinkHandle::writeexactly (same backref/SAFETY discipline;ServerResponseunreachable viasink_handle_from_id).- Reworked backpressure tests:
Bun.peek.statuson the parked write, bounded loop,writer.close()awaited — no time-based polling remains.
Extended reasoning...
Overview
Follow-up perf change to #36695: TextEncoderStreamEncoder__encodeIntoSink now hands the coerced JSString's bytes directly to the sink via new SinkHandle::write_latin1/write_utf16 methods (which fan out to each sink's existing JsSinkType impl) instead of transcoding into a scratch Vec<u8> first. The scratch path is retained only for the carried-lead-surrogate case. RewriterPipe grows &self inherent write_latin1/write_utf16 so the shared-BackRef arm can call them. The coerced string is now explicitly rooted (to_js_string + EnsureStillAlive) across the sink write. Tests add describe.each over start/async-pull sources, an HTMLRewriter GC-stress case, and Bun.peek.status-based backpressure assertions.
Security risks
None user-facing. The memory-safety surface is the interesting part: (1) the WTFStringImpl slice is now borrowed across a sink write that can run user JS (HTMLRewriter content handlers, ArrayBufferSink onReady) — the new EnsureStillAlive guard is what makes this sound, and the HTMLRewriter test with Bun.gc(true) inside the handler exercises it; (2) the unsafe { &*this } shared borrow is held across that same re-entry — the SAFETY comment was reworded per my earlier note to state the actual invariant (Cell/RefCell-only struct, owning cell on the C++ caller's stack).
Level of scrutiny
High. This is hot-path native code with three of REVIEW.md's most-blocked categories in play: GC rooting across user-JS re-entry, unsafe pointer deref with a hand-written SAFETY argument, and encoding correctness (surrogate carry now split between the encoder and each sink's write_utf16). The maintainer already engaged with review and requested specific test changes; those were applied in 7447496 but not yet re-reviewed.
Other factors
- Four comment-cop bot comments on the new multi-line inline comments (lines ~278/291/327/346) are still open. The comments in question document GC-rooting rationale and surrogate-carry semantics rather than justifying workarounds, but per CLAUDE.md rule 13 a maintainer should decide whether to trim them.
- I checked that the trailing-lead trim is equivalent to what
to_utf8_list_with_type_bun::<true>produces on the slow path for the edge shapes the test enumerates, and thatSinkHandle::write_latin1/write_utf16mirror the existingwritedispatch arm-for-arm. - Given Jarred was mid-review and the PR restructures a native fast path, auto-approval isn't appropriate here.
2 participants
Follow-up to #36695.
What
When a native JSSink is attached (
m_nativeSinkPtr),TextEncoderStreamEncoder__encodeIntoSinknow hands the chunk'sWTFStringImplbytes straight to the sink viaSinkHandle::write_latin1/SinkHandle::write_utf16instead of first transcoding into a per-encoder scratchVec<u8>and then callingwrite_bytes.Why
Every sink's
write_latin1already has anis_all_asciifast path that forwards the input slice verbatim (andHTTPServerWritablesends it straight to the socket when the buffer is empty and the chunk exceeds the high-water mark), so an all-ASCII 8-bit chunk is now zero-copy from theJSStringto the sink. Non-ASCII Latin-1 and UTF-16 chunks are transcoded once, directly into the sink's own buffer, instead of once into the scratchVecand then memcpy'd into the sink. This was the open follow-up noted in #36695 ("TextEncoderStream pure-ASCII zero-copy path (GCOwned<StringView>→Temporarystraight to the sink)").How
SinkHandlegrowswrite_latin1/write_utf16that dispatch to each sink's existingJsSinkTypeimplementation.RewriterPipegains&selfinherentwrite_latin1/write_utf16(theJsSinkTypeimpl now delegates) so the shared-BackRefHTMLRewriterarm can call them.handle.write_latin1(string_bytes)directly.pending_lead_surrogate, thenhandle.write_utf16(remainder)directly. Every sink'swrite_utf16(simdutf fast path,decode_utf16_with_fffdfallback) emits U+FFFD for mid-chunk lone surrogates exactly as the encoder did; the trailing-lead position is the one place the encoder differs, and that is handled by the trim.Writable(one backpressure signal).JSStringis now rooted across the sink write (to_js_string+EnsureStillAlive, the same pattern asJSSink::js_write). This matters becauseHTMLRewriter'swriteruns user JS content handlers while the input slice is still being parsed, andArrayBufferSink'ssource.readyis also a GC point; whenchunkwas not already a string,toStringOrNullcreates a freshJSStringthatget_zig_stringdid not root.Tests
text-encoder-stream.test.ts: native-sink round-trips for 8-bit (ASCII + Latin-1 non-ASCII) and 16-bit (mid-chunk and cross-chunk surrogate) chunks againstBun.serve; anHTMLRewritersink with ToString-coerced chunks andBun.gc(true)inside the element handler; native-sink backpressure to a stalled client; readable-side backpressure (HWM=1, second write parks until drained).text-decoder-stream.test.ts: readable-side backpressure for both the utf-8 fast path and thefatal:truedecoder path.Benchmark
Release build,
ReadableStream<string>(256 KiB all-ASCII, 2000 chunks) →TextEncoderStream→Bun.serve→fetch().arrayBuffer(), 5-run median on a shared-tenant container:59cab0e97The HTTP stack still dominates; the win is one scratch allocation + one memcpy eliminated per chunk on the native-sink path.