Add readiness gate to make idle poll_output O(1) - #1011
Open
k0nserv wants to merge 4 commits into
Open
Conversation
Everything below Rtc forms a tree; events and timeouts bubble up. On main every poll_output re-walks the whole tree (the .soonest() timeout fold plus poll_event over all medias/streams), so an idle poll that only returns Output::Timeout is O(number of m-lines). An SFU calls poll_output thousands of times per second per connection, paying that scan O(N) times. Introduce a Wake guard threaded down the tree (src/poll.rs). It defaults to re-polling everything and is narrowed locally by the component that mutated (no_events / no_timeout), folding into an Rtc-owned Readiness on drop. poll_output gates on it: skip the event pipeline unless something queued output, skip the timeout recompute unless a timer moved, otherwise return the cached timeout. Forgetting or over-arming costs a redundant poll, never dropped output. The mutation and handle_input paths pass their own guard to each subsystem so the invalidation decision stays local (session, chan, sctp, Writer, Bwe, Channel). External crates (ice, sctp-proto) can't take a Wake, so those boundaries arm conservatively; SdpApi/DirectApi still arm at the accessor pending threading. tests/poll-scaling.rs measures it: idle poll_output is flat ~27ns from 2 to 50 m-lines here, versus 135ns->1038ns (7.7x) on main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@copilot Review |
The sdp_api()/direct_api() accessors armed the readiness conservatively for the whole tree. Push that decision down: each mutating method on DirectApi and SdpApi calls self.rtc.readiness.wake() itself, so the invalidation lives with the mutation rather than in Rtc. DirectApi arms in every &mut method (ICE/DTLS/SCTP/channel/media/stream mutators); read-only &self getters do not. send_app_specific_feedback narrows to no_timeout (it only queues RTCP feedback). SdpApi arms the methods that touch rtc directly (accept_offer, accept_answer, set_direction, stop_media, add_channel_with_config, apply); add_media, ice_restart and merge only accumulate pending changes that apply()/ accept_*() enact and arm. Drop #[must_use] from Wake so `readiness.wake();` (immediate drop = arm both) reads cleanly without a drop() wrapper. wake_all() now remains only where it must: the two ICE candidate adds (the ice crate can't take a Wake) and start_close (queues the close drain). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The API wrappers (DirectApi, SdpApi, Channel, Bwe) armed the readiness per-method, which a new &mut method could forget, and armed even for read-only (&self) access. Have them hold an RtcMut instead of &mut Rtc. It is change detection on mutable access: Deref reads and never arms; DerefMut arms both halves. Because a &self method can only reach Deref and a mutating &mut self method must reach DerefMut, "mutation re-polls, reads don't" is enforced by the borrow checker rather than remembered per method. For a mutation that only affects one half of the re-poll -- possibly not known until the mutation is underway -- RtcMut::mutate() returns a Mutation guard: it derefs to Rtc, is armed both by default, is narrowed with no_events()/no_timeout() at any point (including conditionally), and folds its own per-operation signal on drop. The signal is private to the one guard, so narrowing can't interfere with another call's decision in any order -- unlike a long-lived shared guard. RtcMut keeps DerefMut (arm both) because methods that return a &mut borrowed from the tree can't route it through a guard that folds at method end. RtcMut has no Drop, so a wrapper can still be held across other Rtc use (Mutation's Drop is method-local). No Inner split needed. Uses: Bwe (timeout-only, no_events) and DirectApi::send_app_specific_feedback (events-only, no_timeout); everything else arms both or, for readers, nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An active probe generates padding in maybe_create_padding_request()
independently of the regular padding bitrate, but LeakyBucketPacer::next_poll
returned early with `if !padding_possible { return None }` before it got to
the probe branch. So when padding_bitrate is zero (e.g. media is keeping up
with the estimate), an active probe was never scheduled through the pacer's
own timeout: it wasn't driven to completion and kept emitting padding,
sending more than the desired bitrate.
Check for an active probe (and return its next_probe_time) before the
padding-rate early-return, and request an immediate timeout when a probe
starts so it is driven from the outset.
This is a pre-existing bug (present on 0.21.0 and main). It was fixed as a
side effect of #1008 and lost when that was reverted; this restores just the
pacer part, independent of the scheduler/readiness work.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Everything below Rtc forms a tree; events and timeouts bubble up. On main every poll_output re-walks the whole tree (the .soonest() timeout fold plus poll_event over all medias/streams), so an idle poll that only returns Output::Timeout is O(number of m-lines). An SFU calls poll_output thousands of times per second per connection, paying that scan O(N) times.
Introduce a Wake guard threaded down the tree (src/poll.rs). It defaults to re-polling everything and is narrowed locally by the component that mutated (no_events / no_timeout), folding into an Rtc-owned Readiness on drop. poll_output gates on it: skip the event pipeline unless something queued output, skip the timeout recompute unless a timer moved, otherwise return the cached timeout. Forgetting or over-arming costs a redundant poll, never dropped output.
The mutation and handle_input paths pass their own guard to each subsystem so the invalidation decision stays local (session, chan, sctp, Writer, Bwe, Channel). External crates (ice, sctp-proto) can't take a Wake, so those boundaries arm conservatively; SdpApi/DirectApi still arm at the accessor pending threading.
tests/poll-scaling.rs measures it: idle poll_output is flat ~27ns from 2 to 50 m-lines here, versus 135ns->1038ns (7.7x) on main.