perf(uring): reuse the worker turn timestamp - #3136
Conversation
ad6d23f to
29b0204
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad6d23f4eb
ℹ️ 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".
| let waker = self.shared.unpark.waker(); | ||
| loop { | ||
| let now = Instant::now(); | ||
| self.shared.clock.begin(now); |
There was a problem hiding this comment.
Reset the turn clock during unwinding
When the future passed to block_on panics and the caller catches the unwind, neither normal clock.end() call executes, so Clock::turn remains true indefinitely. The retained Handle then returns the stale turn snapshot even outside the worker, and a relative deadline created before the next block_on can be shortened or fire immediately. Use an unwind-safe scope guard to clear the active snapshot whenever the polling turn exits.
Useful? React with 👍 / 👎.
Sample the monotonic clock once at the start of each io_uring worker turn and share that instant with the timer sweep, `Timer::poll`, and keep-alive re-arming, so a turn that polls a deadline per connection pays for one vDSO `clock_gettime` instead of one per poll. The snapshot lives behind an RAII guard: `Clock::turn(now)` freezes the clock and dropping the guard, including on unwind, thaws it, so reads before the first turn or while the worker is parked still hit the real clock and a stale instant is never served past the turn that took it. `Handle` publishes it as `moq_net::Timers::now`, which is the clock the trait already tells callers arming a relative deadline to read. Fixes #3122 Co-Authored-By: Codex <codex@openai.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
29b0204 to
b44d6a1
Compare
Summary
Sample the monotonic clock once at the start of each io_uring worker turn and share that instant with the timer sweep,
Timer::poll, and keep-alive re-arming.Root cause:
block_onreadInstant::now()for its sweep, and then every deadline polled in that same turn read it again independently.Timer::pollis the volume: the connection driver polls its quiche timeout and its keep-alive on every pass, so a busy turn paid a vDSOclock_gettimeper connection per poll to learn a time the loop had just measured. #3122 measured[vdso]at 2.95% of relay CPU on the io_uring path against 0.72% on tokio.The snapshot lives behind an RAII guard.
Clock::turn(now)freezes the clock and dropping the guard, including on unwind, thaws it, so reads before the first turn or while the worker is parked still hit the real clock and a stale instant is never served past the turn that took it.Handlepublishes it asmoq_net::Timers::now, which is the clock the trait doc already tells callers arming a relative deadline (now + interval) to read, so the keep-alive re-arm goes through the trait rather than a bespoke accessor.One deliberate behavior change: a timer that comes due during a turn's polling now fires on the next turn rather than in that turn's sweep. That is what keeps the sweep and the eager
Timer::pollpath agreeing on the time instead of one calling a deadline due while the other calls it pending. It costs at most one extra loop iteration, sincemaybe_parkhands an already-past deadline toio_uring_enter, which returns immediately rather than blocking.What this does not cover
quiche reads the clock itself, once inside
recvand once insidesend(quiche/src/lib.rs:2963and:3968), and again inon_timeout. Those are per packet batch and are not reachable from here, so this recovers the timer and keep-alive share of that 2.95%, not all of it. Filed as a follow-up rather than claimed here.Public API changes
Timergains a private field;Timers::nowis a defaulted trait method now overridden forHandle.Wire behavior changes
Test plan
moq-uring is
cfg(linux)-gated, so it was verified in a container on real io_uring (kernel 6.19), not just cross-compiled:cargo clippy -p moq-uring --all-targets -- -D warnings: clean.cargo fmt -p moq-uring -- --check: clean. macOScargo fmtskips this crate entirely, so it has to run on Linux.cargo test -p moq-uring: 47 tests pass (21 lib + echo/session/endpoint/workers/web), noskipping io_uringlines, so these ran against the real ring.cargo clippy -p moq-uring --target x86_64-unknown-linux-gnu --all-targets -- -D warningsfrom macOS: clean.Both new tests were mutation-verified in isolation:
worker::the_clock_is_frozen_for_a_turnassertsHandle::now()is stable across a 5ms sleep insideblock_onand moves outside it. Removing theTimers::nowoverride fails it.shared::the_turn_freezes_and_the_guard_thawsasserts the guard restores the real clock. MakingTurn::dropa no-op fails it.timer::poll_uses_the_worker_turn_clockarms a deadline a second out in real time, so itsReadycan only come from the turn's instant.Fixes #3122
(Written by Claude Opus 5, taking over Codex's original patch)