You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(net): shrink group consumers without extra allocations - #7791
A group::Consumer moved 280 bytes through every layer handing a group to a reader. Completes the Consumer size quest.
Approach
Share original arrival, timescale, and IETF provenance in the allocation already shared for expiry deduplication. Group clones and frame readers still share the same expiry flag, including after route adoption.
Put prefetch bookkeeping beside the frame slots in their existing lazy allocation. Metadata tables stay separately lazy. A reader that never fills a batch still allocates no batch.
Guard the measured layout at compile time on 64-bit targets and report Consumer size through the existing SESSION_ALLOCS=1 benchmark. Remove the completed quest and its links.
Impact
Public API: private layout only, 280 to 184 bytes on x86_64 with async enabled, a 34.3% reduction. No symbol, signature, or behavior change; no migration.
Wire: unchanged.
Allocations: no additional allocation per common-path group. Both boxes replace allocations already made. Smaller session state offsets their larger payloads in the measured workload.
The waiter change in moq-dev/moq#7789 at ea70199e1f was inspected, not imported. Its additional 64 bytes per waiter stay behind Consumer's existing recovery/idle pointers; its boxed pending subscription variants also do not enlarge Consumer. Measurements below use main's eight-record waiter, isolating this layout change.
Performance
just bench-fanout 6647cda31b "model session" "64 512", on one pinned thread over memory transports. Median of three retired-user-instruction counts, subtracting setup, per delivered group. The measured working tree was committed as 9e0e4e05eb; later review edits leave its runtime production code unchanged:
Scenario
Readers
Base
This change
Change
Model
64
10,286.7
9,789.3
-4.83%
Model
512
10,075.8
9,582.9
-4.89%
Session
64
115,085.9
113,039.3
-1.78%
Session
512
113,227.6
111,125.1
-1.86%
bench/fanout.py compare over setup-subtracted callgrind runs at 64 readers shows session memcpy falling from 9,347.6 to 7,217.5 instructions/group (-2,130.1); model memcpy falls from 992.2 to 543.0 (-449.2).
A temporary allocation counter on the exact fanout session harness, removed from the shipped change in favor of the existing benchmark, measured:
Readers
Allocations/group, base
This change
Requested bytes/group, base
This change
64
64.43
64.43
9,589.3
9,508.6
512
63.21
63.21
9,187.5
9,107.5
Validation
just ci on final head 533ceafa0964b5de151032e8f85a5cb98c750792: passed, including 9,756 Rust tests, 112 skipped.
Group model tests: 90 passed.
Final documentation and quest checks: passed (560 quest documents).
Existing SESSION_ALLOCS=1 cargo bench --locked -p moq-net --features fuzz --bench session -- session_delivery, pinned with taskset -c 7: passed on base and branch. All 12 group-delivery cases, spanning 16 publishers/16 viewers and 1 publisher/256 viewers, all three protocols, and paced/unpaced delivery, retain the same allocation count to the report's 0.01/group resolution. For lite-06 at 256 viewers: 96.31 allocations/group on both, 10,899 to 10,819 requested bytes/group.
Unpinned allocation runs refused multiplexed perf counters on both base and branch; the documented CPU pinning was used, with assertions unchanged.
Targeted Miri: all five tests passed (partial-batch destruction, object metadata, segmented batches, refill boundaries, and ranges), using the pinned 2026-10-18 nightly toolchain and the repository's Miri runner.
Alternatives
Boxing the entire Consumer would add an allocation per delivery. Copying the entire track Info into the shared allocation would retain fields these readers never use, so only the queried metadata is stored. Reused the existing session allocation counter instead of adding another maintained counter.
Review
Claude Code, Claude Opus 5.5 (claude-opus-5-5), medium effort, round 2, reviewed head 533ceafa09. No findings.
All earlier findings were fixed: reuse the existing allocation counter, clarify storage/report comments, and guard the measured layout at compile time.
Follow-ups
None. No maintainer decision remains; recommend merge review after this head is ready.
Design change: reuse the existing SESSION_ALLOCS=1 benchmark for allocation validation and report group::Consumer size there. The separate feature-gated fanout counter was redundant and would not be exercised by CI, so it has been removed. The production Consumer and prefetch layout are unchanged.
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found.
Reviewed changes
Shared delivery state: Consumer::track, arrived, and stale_counted collapse into one Arc<Delivery>, keeping clones and frame readers on the same expiry flag while provenance and arrival still survive route adoption (adoption only swaps cursor/expiry).
Lazy prefetch batch: Prefetch becomes a single Option<Box<Batch>>, so the frame slots, cursor, and optional chains/fields/inflated tables share one allocation made by the first push; Batch::drop preserves the old frames[pos..len] teardown.
Benchmark + docs: SESSION_ALLOCS=1 now prints group::Consumer size, and the bench README/module docs describe it.
Quest cleanup: the completed group-consumer-size quest is removed and its README and relay-cpu-bisect links point at this PR.
Semantics check out: clone sharing, adoption, stale-count dedup, and the unsafe MaybeUninit drop ranges match the old behavior, and one Arc allocation still replaces the old one on the common path.
The reason will be displayed to describe this comment to others. Learn more.
Automated review by review (OpenAI)
Reviewed head 533ceafa0964b5de151032e8f85a5cb98c750792 against base 6647cda31ba061562c990eb5e7e2a980c9639eb6.
No actionable correctness findings in this diff. I checked that Delivery retains the original metadata and shared expiry flag through cloning, frame handoff, and route adoption; that lazy Batch allocation preserves refill, metadata, and MaybeUninit destruction invariants; and that the benchmark/reporting and quest changes agree with the implementation. Existing range, mixed segmented-frame, clone, refill-boundary, and partial-batch-drop tests were inspected.
Verification limits: GitHub-only static review. I did not run builds, tests, Miri, or benchmarks, and did not independently reproduce the performance figures. The current GitHub Check run was still in progress when inspected. State, head, prior reviews, and review threads were rechecked immediately before submission.
No actionable comments were generated in the recent review.
Walkthrough
Prefetch now stores frames and optional metadata in a lazily allocated batch. Consumer clones and expiry handlers share delivery metadata, including the flag used to count stale content once. The allocation report now includes the size of moq_net::group::Consumer. Performance documentation removes the completed consumer-size quest and updates related references.
Merge Risk: Minimal · up to 533ce
No actionable merge-blocking issue is established. The storage-layout and reporting changes appear mergeable subject to normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title clearly summarizes the primary change: reducing group::Consumer size without adding allocations.
Description check
The description directly explains the layout reduction, allocation strategy, performance impact, validation, and documentation changes.
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 76.47% which is insufficient. 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 17 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 069a26fe-edb5-4c4d-ac76-6de61bff5bd0
Commits
Reviewing files that changed from the base of the PR and between 6647cda and 533ceaf.
Files selected for processing: 7
bench/README.md
quest/m1/release-perf/README.md
quest/m1/release-perf/group-consumer-size.md
quest/m1/release-perf/relay-cpu-bisect.md
rs/moq-net/benches/session/mod.rs
rs/moq-net/src/model/frame.rs
rs/moq-net/src/model/group.rs
Files with no reviewable changes: 1
quest/m1/release-perf/group-consumer-size.md
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
quest land watch: the merge queue removed this pull request at 533ceaf because its merge_group build failed (failed_checks).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
The queue log reports tests::live_multi_rung panicking at rs/moq-transcode/src/lib.rs:556:44 with Cancel (lines 12237–12254). It passed unchanged in isolation and in the full moq-net/moq-transcode suite: 3509 passed, 13 skipped. The moq-net all-targets build, including the changed session benchmark, also passed. Local builds needed explicit paths to already installed PipeWire development files, libclang and system headers. No repository changes or commits were needed. The intermittent live-rung cancellation needs follow-up; the log and PR diff do not establish that this PR caused it.
quest land watch: re-enqueued this pull request at 533ceaf, at the back of the merge queue, unchanged, since the land fixer judged the failure flaky: moq-transcode tests::live_multi_rung.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kixelated
changed the title
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
kixelated
left a comment
Problem
A
group::Consumermoved 280 bytes through every layer handing a group to a reader. Completes the Consumer size quest.Approach
SESSION_ALLOCS=1benchmark. Remove the completed quest and its links.Impact
The waiter change in moq-dev/moq#7789 at ea70199e1f was inspected, not imported. Its additional 64 bytes per waiter stay behind Consumer's existing recovery/idle pointers; its boxed pending subscription variants also do not enlarge Consumer. Measurements below use main's eight-record waiter, isolating this layout change.
Performance
just bench-fanout 6647cda31b "model session" "64 512", on one pinned thread over memory transports. Median of three retired-user-instruction counts, subtracting setup, per delivered group. The measured working tree was committed as 9e0e4e05eb; later review edits leave its runtime production code unchanged:bench/fanout.py compareover setup-subtracted callgrind runs at 64 readers shows sessionmemcpyfalling from 9,347.6 to 7,217.5 instructions/group (-2,130.1); modelmemcpyfalls from 992.2 to 543.0 (-449.2).A temporary allocation counter on the exact fanout session harness, removed from the shipped change in favor of the existing benchmark, measured:
Validation
just cion final head533ceafa0964b5de151032e8f85a5cb98c750792: passed, including 9,756 Rust tests, 112 skipped.SESSION_ALLOCS=1 cargo bench --locked -p moq-net --features fuzz --bench session -- session_delivery, pinned withtaskset -c 7: passed on base and branch. All 12 group-delivery cases, spanning 16 publishers/16 viewers and 1 publisher/256 viewers, all three protocols, and paced/unpaced delivery, retain the same allocation count to the report's 0.01/group resolution. For lite-06 at 256 viewers: 96.31 allocations/group on both, 10,899 to 10,819 requested bytes/group.Alternatives
Boxing the entire Consumer would add an allocation per delivery. Copying the entire track Info into the shared allocation would retain fields these readers never use, so only the queried metadata is stored. Reused the existing session allocation counter instead of adding another maintained counter.
Review
Claude Code, Claude Opus 5.5 (
claude-opus-5-5), medium effort, round 2, reviewed head 533ceafa09. No findings.All earlier findings were fixed: reuse the existing allocation counter, clarify storage/report comments, and guard the measured layout at compile time.
Follow-ups
None. No maintainer decision remains; recommend merge review after this head is ready.
(Written by GPT-6.1 Sol)