perf(net): seek instead of scan for the next in-range group - #3088
Merged
Conversation
`TrackState::lookup` was a `HashMap<u64, Slot>`, which has no order, so `poll_next_in_range` had to scan every cached slot to find the lowest sequence at or above the subscriber's cursor. `Subscriber::next_group` calls it once per delivery, making a drain of N cached groups O(N * cache_size). That is the path the media consumers use (moq-mux's container and MSF catalog readers, moq-json, moq-ffi, moq-transcode's feed, and resume), and cache depth is the retained group count: a track publishing one group per frame at the default 5s retention holds ~250 groups, so every delivery scanned ~250 entries. Make `lookup` a `BTreeMap` and seek with `range(next_sequence..)`. The `end_sequence` cap becomes a `take_while` now that iteration is ascending, and only aborted slots (awaiting the next eviction scan) are stepped over. Lookups by sequence go from O(1) to O(log n), but those paths touch one entry at a time. Measured with the new `track_recv_groups` bench arm, sweeping cache depth with both delivery orders. Sequence order was 720 Kelem/s at depth 64 and collapsed to 16.7 Kelem/s at 4096; it is now flat at ~1.8 Melem/s across the sweep, matching the arrival-order walk. nextest runs criterion benches in test mode, so the arm is exercised in CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughThe track cache now uses Merge Risk: ⚪ Minimal · up to This changes internal group lookup to seek efficiently in sequence order without changing the public API or intended delivery behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
Merged
kixelated
added a commit
that referenced
this pull request
Aug 28, 2026
Max age judged a group by where it *started*, so a group was convicted for being behind rather than for having nothing left to give. That is the wrong axis. Priority already transmits newer groups first, so an older group consumes only leftover capacity and closes the gap faster than the live edge advances: being behind is survivable, and a receiver that is behind converges without losing content. What cannot be recovered is a group with nothing left worth delivering. Measure a group by how far it could still *reach* instead: the first frame timestamp of its successor, since a group cannot present past where the next group begins. Its own frames prove nothing, because frame durations are not on the wire and a group's last timestamp is where that frame starts, not where the group ends. A group whose successor has not arrived is therefore never expired on timestamp age; the wall-clock measure still backstops it. Reach is an exclusive bound, so the comparison is `>=`: the freshest frame a group could still hold sits strictly below its reach, and an age equal to the budget already puts every frame in it past the budget. That also makes a zero budget fall out of the general rule, so `wall_stale` loses its `budget.is_zero()` special case. This retires all three competing measures at once: the original first-frame one, the reader-position preference from #2890 (a drained-but-open group measured as level with the edge, so nothing could convict a stall), and the newest-frame one from the previous commit (which had the same defect and hung a reader outright). `Edge` carries a suffix minimum over stamped groups, built in the scan `live_edge` already did, so walking a backlog stays linear rather than rescanning per candidate. It is `Clone` rather than `Copy` now, so `is_stale` and `poll_stale` take it by reference. Once `lookup` becomes a `BTreeMap` (it already is on main, via #3088) this collapses to a `range(sequence + 1..)` walk and the table goes away. Six tests move to the new semantics. The shape they share: a two-group track can never expire the older group on timestamp age, because its reach *is* the edge. Convicting it needs a group beyond the successor, which is what each test now sets up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
TrackState::lookupwas aHashMap<u64, Slot>. A hash map has no order, sopoll_next_in_rangehad to scan every cached slot to find the lowest sequence at or above the subscriber's cursor.Subscriber::next_groupcalls it once per delivery, so draining N cached groups was O(N * cache_size).moq-mux's container and MSF catalog readers,moq-json,moq-ffi,moq-transcode's feed, andmoq-net's resume), and cache depth is the retained group count. A track publishing one group per frame (hang audio, anywrite_frametrack) at the default 5s retention holds ~250 groups, so every delivery scanned ~250 entries.lookupaBTreeMapand seek withrange(next_sequence..). Theend_sequencecap becomes atake_while, which is only correct because iteration is now ascending; the old scan had tocontinuepast it. Aborted slots (awaiting the next eviction scan to reclaim them) are still stepped over, and the early return that parks rather than ends the stream whenend < next_sequenceis unchanged, as isfinal_sequencetermination.lookupget/insert/remove go from O(1) to O(log n). Those are the eviction and fetch paths, which touch one entry at a time, so it is a good trade against removing a full scan per delivery. There is a single insert site and it keys ongroup.sequence, so the seek key is exact.Numbers
New
track_recv_groupsbench arm, sweeping cache depth across both delivery orders.arrivalisrecv_group(an arrival-order index walk, already flat);sequenceisnext_group.Flat across a 64x depth increase, ~107x at the top end. The "before" column is from the original report; the "after" column and the
arrivalbaseline are from this branch.Public API changes
None.
TrackStateand itslookupfield arepub(crate); nopubitem inrs/moq-*orjs/*is added, renamed, removed, or resignatured. The only additions are private helpers in the benchmark. Targetsmainaccordingly.Test plan
just fix(no changes beyond the two files here),just check,just test: 2799 tests pass, 1 skipped.track_recv_groupsarm tors/moq-net/benches/group.rsas the regression guard. nextest runs criterion benches in test mode, so the arm is exercised in CI rather than rotting until someone next runscargo bench.cargo bench -p moq-net --bench group -- track_recv_groupsfor the table above.Cross-Package Sync
No row applies: this is an internal data-structure change in
moq-netwith no wire, catalog, config, or CLI surface touched, sojs/netand the drafts are unaffected.Follow-up
#3086 covers making the delivery order a handle (
ordered()) rather than a method choice, and movingmoq-mux's timestamp-based group skipping down intomoq-net. Worth noting for reviewers: this fix removes the performance argument for treating sequence order as the slower opt-in path, but the API argument stands on its own, sincerecv_datagrambumpsnext_sequenceandread_framebumps both cursors, making the two orders quietly interact on oneSubscriber.(Written by Claude Opus 5)