Repository navigation
perf: finish the m1 performance audit - #6281
Conversation
Quest-Claim: dad32f0cedc681f4
kixelated
changed the title
kixelated
changed the title
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Checkpoint pushed with the export reap fix and the queued-groups benchmark. The 64-group, 16-frame case uses 63.8% fewer instructions; a repeat differs by 0.11%. Public API and wire changes: none. Keeping this PR draft because the broader performance audit is unfinished. Its quest and (Written by GPT-6) |
# Conflicts: # quest/m1/audit-perf.md
The inline eight-frame batch made every group::Consumer 672 bytes, and each layer that hands out a group (track cursor, subscriber, resume reader, mux consumer) moved it by value. Box the uninitialized storage on the first non-empty fill instead, so a consumer that never reads whole frames still allocates nothing. Callgrind, one iteration (instructions): - annexb-export-queued-groups/64/1: 1,389,531 -> 1,183,215 (-14.8%) - annexb-export-queued-groups/16/1: 443,536 -> 394,905 (-11.0%) - track_recv_groups/arrival/512: 3,274,648 -> 2,775,448 (-15.2%) - group_read_frames/single/4: 1,422 -> 1,430 (+0.6%) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every estimate a container producer published read its rendition's config through a full catalog snapshot, three times per change, so each frame cloned every rendition on the broadcast. VR metadata (#5543) made each VideoConfig clone larger, which is the +3.3% the m0 audit bisected to it. Clone only the entry under the catalog lock, once per estimate. Callgrind, one iteration of catalog_estimate_fanout (instructions): - hang/tracks_1/1: 19,149,482 -> 12,068,527 (-37.0%) - hang/tracks_4/4: 106,157,457 -> 59,002,822 (-44.4%) - hang/tracks_16/16: 1,074,313,398 -> 509,137,148 (-52.6%) - msf_references/tracks_4/4: 110,861,752 -> 58,856,411 (-46.9%) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Record the m1 run's fixes, attributions, and three-night trend, and move the recurring audit to the end of m2's Required behind the release cut, with what to check first: five nights by commit, the per-group write cost, and the resume reader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/audit-ci.md # quest/m2/README.md
kixelated
changed the title
|
@codex review (Written by Claude Opus 5.5) |
|
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 25 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. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # quest/m1/README.md
![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: 8f99d11a2c
ℹ️ 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".
| self.discard(|group| { | ||
| group.sequence > current | ||
| && group.buffered.is_empty() | ||
| && group.fault.is_none() | ||
| && group.poll_aborted(waiter) |
There was a problem hiding this comment.
When an untrusted publisher keeps the current group supplied with frames while repeatedly opening and aborting higher-sequence groups, poll_read_finish adds those groups to pending, but each read returns through the ready-frame arm before this cleanup runs. The aborted groups therefore accumulate without bound until the current group stalls or finishes, allowing a peer to grow the consumer's memory indefinitely; retain bounded or incremental cleanup on the frame-delivery path rather than running it only when delivery blocks.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Summary
Finishes the m1 run of the recurring performance audit: three measured fixes, one commit each, attribution for the remaining regressions, a trend comparison by commit, and the m2 successor.
Instruction counts are one optimized
--test --exactiteration under callgrind 3.27.1 (Rust 1.98.1), collected inside an#[inline(never)]boundary so fixture setup is excluded. Repeat runs agree within about 1%.1.
perf(mux): reap queued aborts only when delivery blocksThe export consumer checked every queued group's abort state for every delivered frame. It now reaps later aborted groups only when delivery blocks, before the existing hole and latency decisions. New
annexb-export-queued-groupsin the nightlycontainersbench sweeps queued groups x frames per group.2.
perf(net): allocate a group's prefetch batch on its first fillgroup::Consumerwas 672 bytes: its eight-frame prefetch batch was inline, and every layer that hands out a group (track cursor, subscriber, resume reader, mux consumer) moves it by value. This is thememcpythe m0 run attributed to moq-dev/moq#6019. The uninitialized storage is now boxed on the first non-empty fill, so a consumer that never reads whole frames still allocates nothing.annexb-export-queued-groups/64/1annexb-export-queued-groups/16/1track_recv_groups/arrival/512track_recv_groups/sequence/512group_read_frames/single/4group_read_frames/single/5123.
perf(mux): read a rendition's config without cloning the catalogEach estimate a container producer published read its rendition through a whole-catalog snapshot three times, so every frame cloned every rendition on the broadcast. Bisecting the m0 run's +4%: moq-dev/moq#6074 +0.4%, moq-dev/moq#5543 +3.3% (a larger
VideoConfigcloned on every estimate). It now clones only the entry under the catalog lock.catalog_estimate_fanouthang/tracks_1/1hang/tracks_4/4hang/tracks_16/16msf_references/tracks_4/4Attributed, not fixed
1tracks-64frames/256: 1.185M vs 1.495M atad87ad34e). A per-export constant remains (+18.6k at one frame): +8.8k catalog decode, +11ktrack::Subscriber::poll_groupthrough the resume reader, +2.6k track resolve. The duplicate-key check (moq-dev/moq#5941) replaced an equal-cost merge check, so it adds nothing. A one-pass strict JSON parse would save about 14k per catalog snapshot but reimplements serde_json's feature-dependentValuevisitor, so it was not landed.track_stamp_groups/append(nightly +29% on all three depths, stepped and stayed): 6,557 instructions per group at Oct 20f13283b302, 7,729 at Oct 21bc64b9c525, 8,944 now. Step one is feat(moq-net): time anchors for subscription ranges #6074's start index (TrackState::index_start, aBTreeMapinsert per group); step two isgroup::Handle::startandTrackState::wake_landedtaking the track lock again on each group's first frame. These are features; the successor checks them first.Rendition::stagestill clones and serializes the whole catalog per estimate to validate it (62% of the catalog bench after fix 3). Validating only the entry would weaken that guarantee for custom configs, so it is recorded for a decision.Trend comparison
Published nightly history has three distinct commits: Oct 20
f13283b302(plus two reruns, treated as one night), Oct 21bc64b9c525, Oct 2233940b6e33. Of 1,756 Rust and FFI cost series common to all three, 13 stayed over 25% worse on both later nights and 8 stayed over 25% better, close to noise. The worse ones arecatalog_estimate_fanout(fixed above),track_stamp_groups/append(attributed above),track_pending_tail/repoll,session_churn_held/moq-transport-22,lite_varint/encode/Datagram,origin/announce/1000p_1000s,kio/contended_repoll(threaded), and Kotlin FFI (swings nightly). No throughput series moved. JS history starts Oct 22.Decisions (picked unattended, recommended option)
stageas the successor's first checks rather than fixing them here: each is a feature cost or needs a decision, with no measured fix in hand.annexb-export-queued-groups.Quest
quest/m1/audit-perf.mdmoves toquest/m2/audit-perf.md, appended to m2'sRequiredbehind the release cut, with this run's results, baselines, and what to check first. Links to it are repointed.Impact
Prefetchis private, and the new catalogProducer::configispub(super).Validation
just rs test -p moq-net -p moq-mux: 4,669 passed, 14 skipped.just rs test -p moq-relay -p hang -p moq-hls -p moq-ffi: 974 passed, 12 skipped.just check: Rust lint, docs, fmt, quest (613 documents), and drill stages pass; the flake stage stops building the unchanged runner image because this host denies/proc/self/uid_map.quest check --base origin/main --branch quest/m1/audit-perfpasses.Follow-ups
None created. Suggested
/quest-plantopics, if the maintainer wants them before the m2 run: the per-group write path's start index and first-frame relock, and whetherRendition::stagemay validate only the edited entry.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)