From e5db4d948744c5cc76a57c0e63c257cfc6cb8289 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 01:32:21 -0700 Subject: [PATCH 1/5] chore(quest): claim quest/m1/release-perf/gc-idle.md Quest-Claim: 3a1cc0ceaf501d06 From 0974c5a8706208cc2e6318d14ab64903cfd33d68 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 01:48:22 -0700 Subject: [PATCH 2/5] perf(net): skip empty cache deadline clock reads Co-Authored-By: GPT-6 --- quest/m1/release-perf/gc-idle.md | 41 ++++++++++++++ rs/moq-net/benches/track.rs | 19 ++++++- rs/moq-net/src/model/cache.rs | 94 +++++++++++++++++++++++++------- 3 files changed, 132 insertions(+), 22 deletions(-) diff --git a/quest/m1/release-perf/gc-idle.md b/quest/m1/release-perf/gc-idle.md index 0e4e2d5e7ad..e315780ea9d 100644 --- a/quest/m1/release-perf/gc-idle.md +++ b/quest/m1/release-perf/gc-idle.md @@ -24,3 +24,44 @@ measure what remains. Fix: keep the earliest wall deadline in an atomic, as `expiry.rs` already does with `earliest`. Read `now_wall()` and take the lock only when that deadline has passed. Time decisions use `max(wall, pts)`; keep that behavior. + + +### Progress + +The draft implementation in [moq-dev/moq#6291](https://github.com/moq-dev/moq/pull/6291) +hoists the benchmark waiters before measuring either idle collector. It also +measures advancing supplied instants, alongside the existing sweeps over track +counts and wall-deadline track/group counts. + +The private collector now skips the ambient clock read when the wall index is +empty. It keeps one lazy wall sample for the whole release pass when the index +is nonempty. The sampled clock's `fetch_max` became a store: its only writer +already holds the clock mutex and clamps supplied time against the last sample. +The wall index still registers every driver under its existing lock, including a +replacement driver arriving while no deadline is armed. No public API, wire, +aging, or ownership changes are proposed. + +Reused-waiter baseline on 2026-10-22: `track_gc/idle` is 36.7-36.9 ns over +1/64/1,024/16,384 tracks. Initial private-change measurements are 19.5-19.9 ns; +wall-index idle remains flat in both track and cached-group counts. The added +simulated-time test covers ambient time leading a stale driver, supplied future +time leading the ambient clock, and a backward replacement sample. Another test +covers replacement-driver registration and rearming after a deadline drains. + +### Decisions needed + +The original no-lock/no-registration goal is not met. Recommend accepting the +remaining flat cost while preserving the current `gc(now, waiter)` contract: + +- An empty wall index still needs to register a new driver, so it hears the first + deadline armed after it parks. An earliest-deadline atomic alone cannot prove + that this particular waiter remains registered after another driver drops or a + previous wake drains the list. +- A nonempty wall index still needs the ambient clock even if supplied `now` is + before its earliest deadline. A lagging driver must release deadlines reached + by ambient time; comparing only supplied time would change aging semantics. + +Alternatively, design driver registration ownership and a reusable lock-free +notification primitive before changing the collector's contract. That public +API decision is intentionally left to the maintainer. The quest remains active +until its stronger goal is revised or implemented. diff --git a/rs/moq-net/benches/track.rs b/rs/moq-net/benches/track.rs index f471d193563..7e392110cc3 100644 --- a/rs/moq-net/benches/track.rs +++ b/rs/moq-net/benches/track.rs @@ -431,7 +431,8 @@ fn bench_wall(c: &mut Criterion) { group.throughput(Throughput::Elements(1)); group.bench_function(BenchmarkId::new("idle", &id), |b| { let aged = Aged::new(tracks, groups); - b.iter(|| black_box(aged.pool.gc(aged.now, &kio::Waiter::noop()))); + let waiter = kio::Waiter::noop(); + b.iter(|| black_box(aged.pool.gc(aged.now, &waiter))); }); // Re-polling one parked read re-arms its deadline. group.bench_function(BenchmarkId::new("repoll", &id), |b| { @@ -470,7 +471,18 @@ fn bench_gc(c: &mut Criterion) { group.throughput(Throughput::Elements(1)); group.bench_with_input(BenchmarkId::new("idle", tracks), &tracks, |b, &tracks| { let swept = Swept::new(tracks); - b.iter(|| black_box(swept.pool.gc(swept.now, &kio::Waiter::noop()))); + let waiter = kio::Waiter::noop(); + b.iter(|| black_box(swept.pool.gc(swept.now, &waiter))); + }); + // Also advance supplied time, so reusing an identical instant cannot hide clock work. + group.bench_with_input(BenchmarkId::new("advancing", tracks), &tracks, |b, &tracks| { + let swept = Swept::new(tracks); + let mut now = swept.now; + let waiter = kio::Waiter::noop(); + b.iter(|| { + now += Duration::from_nanos(1); + black_box(swept.pool.gc(now, &waiter)) + }); }); // A due pass, stepping one sweep interval per call. group.throughput(Throughput::Elements(tracks as u64)); @@ -478,9 +490,10 @@ fn bench_gc(c: &mut Criterion) { let swept = Swept::new(tracks); let interval = cache::DEFAULT_EXPIRY / 2; let mut now = swept.now; + let waiter = kio::Waiter::noop(); b.iter(|| { now += interval; - black_box(swept.pool.gc(now, &kio::Waiter::noop())) + black_box(swept.pool.gc(now, &waiter)) }); }); } diff --git a/rs/moq-net/src/model/cache.rs b/rs/moq-net/src/model/cache.rs index e3a7cd3a076..48ecdde5fb2 100644 --- a/rs/moq-net/src/model/cache.rs +++ b/rs/moq-net/src/model/cache.rs @@ -391,7 +391,8 @@ impl Pool { first = self.inner.wall_epoch.get().is_none(); let epoch = *self.inner.wall_epoch.get_or_init(|| now); let micros = u64::try_from(now.duration_since(epoch).as_micros()).unwrap_or(u64::MAX); - self.inner.wall_micros.fetch_max(micros, Ordering::Relaxed); + // The clock lock already orders drivers and clamps their supplied time. + self.inner.wall_micros.store(micros, Ordering::Relaxed); if self.sweep_interval().is_none() { clock.sweep = None; } else if sweep && clock.sweep.is_none_or(|at| at <= now) { @@ -404,36 +405,45 @@ impl Pool { let mut undriven = self.inner.wall.lock().undriven.take(); undriven.wake(); } - let wall = self.release_due(self.now_wall(), driver); + let wall = self.release_due(driver); next.into_iter().chain(wall).min() } - /// Wake every read parked on a wall-clock deadline at or before `now`, returning the + /// Wake every read whose wall-clock deadline has passed, returning the /// earliest left and registering `driver` to hear of an earlier one. - fn release_due(&self, now: crate::time::Instant, driver: Option<&kio::Waiter>) -> Option { + fn release_due(&self, driver: Option<&kio::Waiter>) -> Option { + let mut wall = self.inner.wall.lock(); + if wall.due.is_empty() { + if let Some(driver) = driver { + driver.register(&mut wall.drivers); + } + return None; + } + // With no parked read there is nothing to age. Otherwise the ambient clock + // still matters, even when a caller supplies an instant behind its deadline. + // Keep one sample across tracks that rearm while this pass releases them. + let now = self.now_wall(); loop { - let due: Vec<_> = { - let mut wall = self.inner.wall.lock(); - let mut due = Vec::new(); - while let Some(entry) = wall.due.first_entry() - && entry.key().0 <= now - { - due.push(entry.remove()); - } - if due.is_empty() { - if let Some(driver) = driver { - driver.register(&mut wall.drivers); - } - return wall.due.first_key_value().map(|((at, _), _)| *at); + let mut due = Vec::new(); + while let Some(entry) = wall.due.first_entry() + && entry.key().0 <= now + { + due.push(entry.remove()); + } + if due.is_empty() { + if let Some(driver) = driver { + driver.register(&mut wall.drivers); } - due - }; + return wall.due.first_key_value().map(|((at, _), _)| *at); + } + drop(wall); // Outside the lock: a track re-arms its next deadline here. for track in due.iter().filter_map(Weak::upgrade) { if let Some(next) = track.wakes.release_wall(now) { self.schedule(next, None, &track); } } + wall = self.inner.wall.lock(); } } @@ -1328,6 +1338,52 @@ mod test { assert!(read.0.load(Ordering::SeqCst), "the read was never released"); } + /// A fresh driver must hear deadlines armed after an empty collection, including + /// after the previous driver was dropped or a prior deadline drained its wake list. + #[test] + fn replacement_drivers_hear_new_deadlines() { + let pool = Pool::unbounded(); + let now = super::super::clock::wall(); + pool.gc(now, &kio::Waiter::noop()); + let track = Track::new(pool.clone(), kio::Weak::new()); + let (read, reading) = flag(); + let (driver, driving) = flag(); + assert_eq!(pool.gc(now, &driving), None); + let at = now + Duration::from_secs(1); + track.watch_wall(at, &reading); + assert!(driver.0.swap(false, Ordering::SeqCst)); + assert_eq!(pool.gc(at, &driving), None); + assert!(read.0.swap(false, Ordering::SeqCst)); + track.watch_wall(at + Duration::from_secs(1), &reading); + assert!(driver.0.load(Ordering::SeqCst), "the drained list was never rearmed"); + } + + /// Wall time can lead a lagging driver's sample, while another driver can supply + /// future time. Neither a new driver nor a backward sample may rewind expiration. + #[moq_clock::test] + async fn wall_deadlines_use_the_later_of_ambient_and_driven_time() { + let pool = Pool::unbounded(); + let now = moq_clock::now(); + let waiter = kio::Waiter::noop(); + pool.gc(now, &waiter); + let track = Track::new(pool.clone(), kio::Weak::new()); + let (read, reading) = flag(); + let at = now + Duration::from_secs(1); + track.watch_wall(at, &reading); + moq_clock::sleep(Duration::from_secs(1)).await; + assert_eq!(pool.gc(now, &waiter), None); + assert!(read.0.swap(false, Ordering::SeqCst), "ambient time was ignored"); + + let future = at + Duration::from_secs(1); + track.watch_wall(future, &reading); + assert_eq!(pool.gc(future, &waiter), None); + assert!(read.0.swap(false, Ordering::SeqCst), "driven time was ignored"); + let tick = pool.now(); + pool.gc(now, &waiter); + assert_eq!(pool.now_wall(), future); + assert_eq!(pool.now(), tick); + } + /// A track indexes one wall-clock deadline, its earliest, and takes it along when dropped. #[test] fn a_dropped_track_leaves_the_wall_index() { From 384377d3bc9244475b53b3c5259867c2e87eeb53 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 14:34:37 -0700 Subject: [PATCH 3/5] fix(net): pass the frame budget to Track::new in the wall-index tests Main added a frame budget argument while this branch was open. Co-Authored-By: Claude Opus 5.5 --- rs/moq-net/src/model/cache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rs/moq-net/src/model/cache.rs b/rs/moq-net/src/model/cache.rs index b605073dea9..3fdca9347b6 100644 --- a/rs/moq-net/src/model/cache.rs +++ b/rs/moq-net/src/model/cache.rs @@ -1356,7 +1356,7 @@ mod test { let pool = Pool::unbounded(); let now = super::super::clock::wall(); pool.gc(now, &kio::Waiter::noop()); - let track = Track::new(pool.clone(), kio::Weak::new()); + let track = Track::new(pool.clone(), Default::default(), kio::Weak::new()); let (read, reading) = flag(); let (driver, driving) = flag(); assert_eq!(pool.gc(now, &driving), None); @@ -1377,7 +1377,7 @@ mod test { let now = moq_clock::now(); let waiter = kio::Waiter::noop(); pool.gc(now, &waiter); - let track = Track::new(pool.clone(), kio::Weak::new()); + let track = Track::new(pool.clone(), Default::default(), kio::Weak::new()); let (read, reading) = flag(); let at = now + Duration::from_secs(1); track.watch_wall(at, &reading); From 56943a1dd03043a3609fb6567a20d50fd35a7aec Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 14:34:37 -0700 Subject: [PATCH 4/5] quest: accept the idle GC cost and retire its quest The remaining ~8 ns over release pays for wall-clock aging (#5548). An earliest-deadline atomic cannot prove a replacement driver is registered, and a new notification primitive is not worth ~8 ns. Co-Authored-By: Claude Opus 5.5 --- quest/m1/release-perf/README.md | 11 +++++- quest/m1/release-perf/gc-idle.md | 67 -------------------------------- 2 files changed, 9 insertions(+), 69 deletions(-) delete mode 100644 quest/m1/release-perf/gc-idle.md diff --git a/quest/m1/release-perf/README.md b/quest/m1/release-perf/README.md index a35788fa4bb..3e386465ad7 100644 --- a/quest/m1/release-perf/README.md +++ b/quest/m1/release-perf/README.md @@ -42,6 +42,14 @@ noise. match payload billing per label, such as per signing key. The drain runs once a second, using under 0.01% of a core at 10,000 sessions. Deleting those bytes was rejected. +- `track_gc/idle` is accepted at about 20 ns, flat across track counts, after + [#6291](https://github.com/moq-dev/moq/pull/6291) skipped the clock read + with no wall deadline and reused the bench's waiter (36.7 ns before). The + remaining ~8 ns over release's 11.6 ns is the lock and driver registration + that the wall-clock aging from + [#5548](https://github.com/moq-dev/moq/pull/5548) needs. An earliest-deadline + atomic cannot prove a replacement driver is registered, and a lock-free + notification primitive is not worth ~8 ns. ### Release as a baseline @@ -60,7 +68,7 @@ usually pays for a feature. | `track_gap_flush/512` | 1.8 us | 48 us | 27x | same | | `track_gap_flush/64` | 1.5 us | 6.2 us | 4.1x | same | | `stats/session-report/1000x10` | 11.0 us | 86.8 us | 7.9x | [Stats report](/quest/m1/release-perf/stats-report.md) | -| `track_gc/idle/16384` | 11.6 ns | 73.7 ns | 6.3x | [Idle GC](/quest/m1/release-perf/gc-idle.md) | +| `track_gc/idle/16384` | 11.6 ns | 73.7 ns | 6.3x | accepted (above) | | `origin/resume_overlap/sequence/1r_64g_32c` | 93 us | 426 us | 4.6x | [Resume duplicates](/quest/m1/release-perf/resume-duplicates.md) | | `session_delivery_group_cost/moq-transport-22/.../viewers=64/.../fetch` | 1.19 ms | 3.86 ms | 3.2x | accepted (above) | | `group_write_frames/single/8192` | 424 us | 486 us | 1.17x | [Re-check](/quest/m1/release-perf/recheck.md) | @@ -95,7 +103,6 @@ one. - [FETCH fan-out](/quest/m1/release-perf/fetch-fanout.md) - a relay serves later viewers' lite FETCHes of a group from its cache, so per-viewer cost falls with viewers again - [Sparse gaps](/quest/m1/release-perf/sparse-gaps.md) - an arrival and a gap flush cost O(log gaps), not O(gaps) - [Resume duplicates](/quest/m1/release-perf/resume-duplicates.md) - an ordered resume skips copies of an already-delivered group without consuming them -- [Idle GC](/quest/m1/release-perf/gc-idle.md) - an idle track GC skips the wall-clock pass when nothing is due - [Stats report](/quest/m1/release-perf/stats-report.md) - the stats drain locks each row once and its bench counts the wire walk; per-label bytes stay, at an accepted O(live sessions) - [Re-check](/quest/m1/release-perf/recheck.md) - the screen-only moves are re-measured with instruction counts, and the ones that hold are fixed or accepted - [JS frames](/quest/m1/release-perf/js-frames.md) - the js/net frame path is no slower than release in a browser, or the gap is accepted diff --git a/quest/m1/release-perf/gc-idle.md b/quest/m1/release-perf/gc-idle.md deleted file mode 100644 index e315780ea9d..00000000000 --- a/quest/m1/release-perf/gc-idle.md +++ /dev/null @@ -1,67 +0,0 @@ -# [S] Idle GC - -## Goal - -`track::Pool::advance` with nothing due does no clock read, lock, or waker -registration. `track_gc/idle` returns to a few nanoseconds. - -## Plan - -Found by reading code on 2026-10-21 (11.6 ns to 74 ns, flat across sizes, -confirmed). [#5548](https://github.com/moq-dev/moq/pull/5548) (wall-clock -aging of superseded groups) added the following to every `advance` in -`rs/moq-net/src/model/cache.rs`: - -- the `wall_epoch` `OnceLock` and a `wall_micros.fetch_max`; -- `release_due(self.now_wall(), driver)`, which reads `moq_clock::now()`; -- the `inner.wall` lock and `driver.register`. - -Part of the cost is the bench itself: it builds a fresh `kio::Waiter::noop()` -on every iteration, so each `register` allocates and frees an `Arc`. A -real driver reuses its waiter. Hoist the waiter out of `b.iter` first, and -measure what remains. - -Fix: keep the earliest wall deadline in an atomic, as `expiry.rs` already -does with `earliest`. Read `now_wall()` and take the lock only when that -deadline has passed. Time decisions use `max(wall, pts)`; keep that behavior. - - -### Progress - -The draft implementation in [moq-dev/moq#6291](https://github.com/moq-dev/moq/pull/6291) -hoists the benchmark waiters before measuring either idle collector. It also -measures advancing supplied instants, alongside the existing sweeps over track -counts and wall-deadline track/group counts. - -The private collector now skips the ambient clock read when the wall index is -empty. It keeps one lazy wall sample for the whole release pass when the index -is nonempty. The sampled clock's `fetch_max` became a store: its only writer -already holds the clock mutex and clamps supplied time against the last sample. -The wall index still registers every driver under its existing lock, including a -replacement driver arriving while no deadline is armed. No public API, wire, -aging, or ownership changes are proposed. - -Reused-waiter baseline on 2026-10-22: `track_gc/idle` is 36.7-36.9 ns over -1/64/1,024/16,384 tracks. Initial private-change measurements are 19.5-19.9 ns; -wall-index idle remains flat in both track and cached-group counts. The added -simulated-time test covers ambient time leading a stale driver, supplied future -time leading the ambient clock, and a backward replacement sample. Another test -covers replacement-driver registration and rearming after a deadline drains. - -### Decisions needed - -The original no-lock/no-registration goal is not met. Recommend accepting the -remaining flat cost while preserving the current `gc(now, waiter)` contract: - -- An empty wall index still needs to register a new driver, so it hears the first - deadline armed after it parks. An earliest-deadline atomic alone cannot prove - that this particular waiter remains registered after another driver drops or a - previous wake drains the list. -- A nonempty wall index still needs the ambient clock even if supplied `now` is - before its earliest deadline. A lagging driver must release deadlines reached - by ambient time; comparing only supplied time would change aging semantics. - -Alternatively, design driver registration ownership and a reusable lock-free -notification primitive before changing the collector's contract. That public -API decision is intentionally left to the maintainer. The quest remains active -until its stronger goal is revised or implemented. From ab196cc0f0cd36864df7597a6ab8365d220da6d8 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 16:16:23 -0700 Subject: [PATCH 5/5] fix(quest): point cache-metadata at the merged idle GC PR Co-Authored-By: Claude Opus 5.5 --- quest/m2/performance-survey/cache-metadata.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/quest/m2/performance-survey/cache-metadata.md b/quest/m2/performance-survey/cache-metadata.md index 5d90ec737fc..b08fe5fd250 100644 --- a/quest/m2/performance-survey/cache-metadata.md +++ b/quest/m2/performance-survey/cache-metadata.md @@ -22,8 +22,8 @@ Correct the accounting explanation with the chosen implementation. Coordinate with [group storage](/quest/m2/group-storage-bench.md) and [log adoption](/quest/m2/perf/kio-channel-contention/log-adoption.md) if their -representation has changed. [Idle GC](/quest/m1/release-perf/gc-idle.md) owns -unnecessary clock/lock work, not retained queue backing. +representation has changed. Idle GC clock and lock work was settled by +[moq-dev/moq#6291](https://github.com/moq-dev/moq/pull/6291); this owns retained queue backing. Follow the [survey benchmark contract](/quest/m2/performance-survey/README.md): baseline first, paired evidence, equivalent work and nightly coverage. A