Repository navigation
perf(net): park memory-transport reads and writes on the close only when they wait - #7262
Conversation
Quest-Claim: b3a59157ae1bbdfc
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
kixelated
changed the title
…hen they wait Each kio::wait future holds its own waiter since #7042, so registering on the session close before every channel read allocated a fresh shared waker per frame and grew the close list. Check the close only when the read or write parks; a ready one peeks or skips it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 33dab5994c4ab91e6b5dd3be3fd9b1194c3d0e93.
No actionable bugs found. The memory-transport optimization is appropriately local and simpler than restoring per-stream waiter state. Channel registration remains atomic with the readiness check, and the second read check preserves queued-data precedence when closing races an empty read. The write path retains its close check without registering ready writes. No public API or wire changes.
Inspected the changed transport, surrounding close/stream lifecycle, kio waiter/notification semantics, and regression test, with an independent static review. Tests and benchmarks were not run; the reported performance gains were not independently reproduced.
(Written by OpenAI)
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Memory transport read fast path —
Chunks::poll_read_chunknow polls the channel first and only registers on the session close when the read parks, then re-polls the channel so a message queued before the close still drains. - Memory transport write fast path —
SendStream::poll_write_bufpeeks the close (closed(), a read lock) instead of registering, and only callspoll_closewhen the write parks on a full channel. Channel::readable()helper — factors the read predicate (!messages.is_empty() || end.is_some()), replacing the inline closure.- New test
only_parked_streams_wait_on_the_close— a counting waker verifies a ready read/write leaves no close registration behind while a parked one is woken by the close. - Quest cleanup — deletes
quest/m1/ietf22-fetch-poll-cost.mdand itsquest/m1/README.mdrow.
I traced the read/write precedence against the previous close-first ordering. A session close only sets the session flag; it never ends stream channels, so dropping the ready-path registration changes nothing observable. Queued messages still drain before the close, a write after close still fails, and WaiterList::register is idempotent per list while the list holds the waiter, so the read path's two channel polls with one Waiter do not stack entries. No blocking concerns.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: memory-transport reads and writes register for close notifications only when they wait. |
| Description check | |
The description directly explains the memory-transport fix, its performance impact, behavior guarantees, benchmark results, and validation. It is fully related to the changeset. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 1 files.
Finishing Touches
-
Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
|
Landing summary: the memory transport now registers on the session close only when a read or write must park, so a ready read/write no longer allocates a waiter. This removes the draft-18/22 FETCH regression from moq-dev/moq#7042 (-12.5% instructions per 256-frame round vs main, below the pre-#7042 parent). Public API and wire format are unchanged.
(Written by Claude Opus 5.5) |
Finishes quest/m1/ietf22-fetch-poll-cost.md.
Problem
moq-dev/moq#7141 reported that moq-dev/moq#7042 (
6eff65506e, transport polls take kio waiters through&self) adds +12.8% instructions per round to moq-transport-22 FETCH of 256-frame groups.Reproduced. The model bench's
fetch_framescase with draft 22, 1 publisher, 1 viewer and 256 frames costs 8.03M user instructions per round at the parent (4fdbad719e), 9.13M at6eff65506e(+13.6%), and 8.98M on main2fff7ea7ae(+11.8%). Allocations went from 8228 to 11285 per round (10256 on main), about 12 more per frame.Cause
The in-memory transport, which both session benches run on, polled the session close before every channel read and every write. That registers the poll's waiter on the session-wide close list even when the read returns immediately.
Before #7042, each stream kept one
kio::Park, so the same waiter re-registered as a no-op. Since #7042, everykio::waitfuture holds its own waiter, and draft 18/22 FETCH reads each frame header and payload through a fresh one. Each read therefore allocated the waiter's sharedArc<Waker>and left anotherWeakin the close list for the list to collect.The QUIC adapters (moq-tokio, moq-uring) don't poll the session close on each read, so this cost belongs to the model transport. Lite's allocations don't change with the fix, so its re-registration was already a no-op. It saves only the per-read close lock and registration check.
Fix
rs/moq-net/src/transport/memory.rs: a read or write registers on the close only when it parks.A new test,
only_parked_streams_wait_on_the_close, checks that a read or write that returns immediately is not woken by the close, and that a parked one is. It fails on the previous code.Measurements
User-space instructions per steady round on a performance core (
cpu_core, pinned to one CPU) viaperf_event_open. They exclude the cache tick and setup, and the counter was never multiplexed. Each value is the median of Criterion's 10 samples. Every revision was built from its owngit archiveexport into its own target directory, with a bench-only patch that adds draft 18 toVERSIONSand the counter. Same-build reruns of main agree within ±0.15% everywhere, so wall-clock noise from host load doesn't enter.Draft 22, model bench (
rust/net/session_delivery_fetch_frames):4fdbad719e6eff65506eAllocations per 256-frame round drop from 10256 to 7180 (parent 8228).
The full sweep covers lite-06, lite-07-wip, draft 18 and draft 22, publishers 1/4, viewers 1/16, and 4/256 frames, on both the model and relay benches. Every case improves against main, and every model case is at or below the parent. Draft 18 and 22 drop 12.2-13.1% at 256 frames and 2.6-3.8% at 4 frames. Lite drops 2.2-2.3% at 256 frames and 0.4-0.6% at 4 frames, with unchanged allocations. The relay bench matches the model bench to within 0.6 points.
Full sweep (main measured twice from the same build)
Impact
&selfwaiter API from refactor(net)!: transport polls take kio waiters through &self #7042 is unchanged.Choices made without asking
kio::wait. Reusing one waiter across a stream's futures would have restored the old no-op re-registration, but it would bring back per-stream parking state that refactor(net)!: transport polls take kio waiters through &self #7042 removed. Registering only when parking removes the cost on the ready path altogether, so the result is below the parent.VERSIONS. The sweep added it only in the measurement exports. Draft 18 and 22 measure within 0.2% of each other everywhere, and a fourth version would add a third to every session delivery group's Nightly time.Follow-ups
kio::waitfuture a fresh waiter, so a read that parks allocates its shared waker each time, where a per-streamParkreused one before refactor(net)!: transport polls take kio waiters through &self #7042. The memory-transport benches can't see this (their reads rarely park). It may deserve a measurement on moq-tokio/moq-uring session benches. Suggested for/quest-plan, not filed.Validation
quest gate nix develop --command just ci origin/main: PASS (9218 tests, plus lints andquest check).cargo test -p moq-net --lib transport::memory: PASS. The new test fails against the old implementation.Review
gpt-6.1-sol, medium effort, head33dab5994c4ab91e6b5dd3be3fd9b1194c3d0e93againstorigin/main89575b519c. 0 findings.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code