Repository navigation
perf(net): drop a finished stream's reader instead of releasing its lock - #6479
Conversation
Quest-Claim: 251688a7054d1526
Releasing the reader lock of a stream that already ended replaces its closed promise with a rejected one, which builds a TypeError for every group stream. In Chromium that cost about 5 us per stream, which put the js/net frame benchmark 1.2 to 1.5x behind release. Dropping the reference is enough to keep a later stop from cancelling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Outcome: I traced the remaining browser gap to (Written by Claude Opus 5.5) |
Record the maintainer's decision to accept browser parity in the epic. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fce081e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reader kept its ReadableStream only to call getReader(), so after FIN the still-locked stream held the dropped reader alive for as long as the Reader was reachable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Merging. Summary:
(Written by Claude Opus 5.5) |
|
To use Codex here, create an environment for this repo. |
Finishes quest/m1/release-perf/js-frames.md.
Summary
The remaining js/net frame gap against
releasecame from one line. Since #4857,Reader.#fillcallsreleaseLock()once a stream reports done. On a stream that has already ended, releasing the lock replaces the reader'sclosedpromise with a new rejected one. That builds aTypeErrorwith a stack for every group stream. In Chromium this costs about 5 us per stream. Spread over the bench's 50 frames per group, it accounted for the 1.2 to 1.5x on batched lite rows and most of the IETF gap.The reader is now dropped, not released. #4857 needed the drop: a later
stop()still sends nothing and builds no error. Nothing reads the stream again after FIN, so the lock never needs releasing.A regression test reads 32 streams to FIN and asserts no
TypeErrororErroris retained. It fails with 64 on the old code.frames.tsalready shows the cost in Chromium, so no benchmark changes.The quest file is deleted. The epic records the result, and its final step now re-runs the JS frame path in Chromium. That step replaces this quest's "re-run after the other children land" item: both siblings are Rust-only.
Attribution
All runs used headless Chromium from the Nix shell, on a shared 24-core host with load average 19 to 35.
The whole-run harness (
bench/browser/micro.ts, one variant per page, three rounds) gave A/A ratios from 0.36 to 1.55 here, too wide to rank anything. So variants were bundled into one page and interleaved per row, with the starting variant rotated each repetition. The tables report the median of per-repetition ratios, with the IQR in brackets.Component split at main (21 reps, ns per frame):
So the decode path was at parity per frame and the gap was per stream. Removing
releaseLockalone brought decode with 1 frame per stream from 8225 to 2500 ns against release's 2575.frames.ts, release vs this branch (lite 15 reps at 12% of the frames, IETF 15 reps at 25%):Before the fix, the same harness put main at 1.23 to 1.48 on batched lite rows and 1.13 to 1.57 on IETF rows.
Tried and dropped
Group-path changes were tried and measured, then dropped: skipping the compression
WeakMaplookup per write, and skipping the per-read frame copy that #6230 added for plain frames. Neither moved the group benches outside noise. The group path's 3 to 12% sits on an A/A of 3 to 8%, which this host can't separate.Public API and wire impact
None.
Checks
just check: pass.bun test src/stream.test.ts: 66 pass. The new test fails on the old code.bench/browser/micro.ts --bench frames(whole-run harness): two of three rounds finished. In round 3 the existingmaxBatchedRatioassertion tripped on a 136 us/frame host stall in an IETF row. That assertion predates this PR.Decisions
Decided 2026-10-23 (maintainer): browser parity accepted on this evidence. Every
frames.tsrow is within the A/A spread of release, and the group path's 3 to 12% sits on an A/A of 3 to 8%. The epic records this, and its final step re-runs the JS frame path in Chromium.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code