perf(net): seek instead of scan for the next in-range group - #3088
perf(net): seek instead of scan for the next in-range group#3088kixelated wants to merge 1 commit into
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>
|
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 |
1 participant
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)