test(moq-stats): benchmark one relay's stats tick

[kixelated]

Problem

One relay's stats tick drains the whole registry and encodes the traffic tracks, and idle paths stay in the plain snapshot for as long as the registry holds them. Nothing measured that cost, so a tick that grows with the table, or a plain frame nearing the 32 MiB cache cap, would not show up.

Approach

Add a Criterion target, stats/tick, that drives one depth-0 producer drain over a synthetic registry. The sweep is held paths (100, 1,000, 10,000, 50,000), tiers (1 and 4), and the share of paths whose publisher byte counter moved (0, 10, 100). It reports time, allocations, the plain snapshot still held and its share of the 32 MiB cache cap, and the plain and compressed bytes written this tick. At 50,000 paths the plain snapshot is 20,100,001 bytes (60% of the cap). The bench asserts only that it stays under the cap. Each id builds its relay and prints its row once, and only when it runs.

The bench reaches the drain through moq-stats's bench feature, which is off by default and not a supported API. Marking paths changed uses moq_net::fuzz::bump_publisher_bytes, the same kind of hidden hook the moq-net benches use. Announce guards keep the entries alive across drains. Stats behavior and the wire format are unchanged.

just rs bench-stats --test runs each tick routine once, plus the existing decode table (which ignores --test and runs in full). Nightly calls that recipe. The target ids are static, so --list matches a real run.

Impact

• Public API: none. produce::bench and fuzz::bump_publisher_bytes compile only for the bench and stay hidden from the docs.
• Wire: none.

Alternatives

• A public registry mutator, so the bench would not need the bench feature. Rejected: nothing but the bench needs to bump a counter without a media frame.
• A path count that walks up to the cache cap at runtime. Rejected: the Criterion ids would no longer match --list, and 50,000 paths already lands at about 60% of the cap.
• Putting aggregate.rs on the same nightly step. Left out: that bench is slow, and this quest does not ask for it.

Follow-ups

• Binary delta stats (quest/m2/stats-delta.md) is unblocked. Recommendation: start it only if encode CPU still matters. A full rewrite at 10,000 paths is about 22 ms for one tier and about 91 ms for four.
• Benchmark regressions in CI (quest/m1/bench-ci.md) should track stats/tick once that quest lands. Recommendation: do not add comparison CI here.
• rs/moq-stats/benches/aggregate.rs is still outside the nightly smoke. Recommendation: leave it until someone wants that number.

(Written by Grok 4.7)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: a6af8555-1be4-4028-bf68-c77a692b8cb4

📥 Commits

Reviewing files that changed from the base of the PR and between 03529eb and 0cab7dc.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (12)

• .github/workflows/nightly.yml
• quest/m2/README.md
• quest/m2/bench-coverage.md
• quest/m2/stats-delta.md
• quest/m2/stats-producer-bench.md
• rs/justfile
• rs/moq-net/src/fuzz.rs
• rs/moq-net/src/stats.rs
• rs/moq-stats/Cargo.toml
• rs/moq-stats/benches/producer_tick.rs
• rs/moq-stats/src/produce.rs
• rs/moq-stats/src/produce_bench.rs

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Grok review of af77fe8c (first review on this PR; covers the full diff)

The bench hook stays out of the default build: bump_publisher_bytes is cfg(any(test, feature = "fuzz")), and the TrackPair byte fields plus produce::bench exist only behind bench. The Snapshot::update return change leaves non-bench behavior the same, because errors still log and fall through exactly as before. I found nothing blocking. CI is pending on the merge commit; everything passed on d793b4e8.

Non-blocking

1. Each Criterion sample rebuilds the relay and reprints the table row (benches/producer_tick.rs:217-228). Criterion calls the bench_function closure once per warm-up round and once per sample, not once per id. That means Relay::build (up to 50,000 publishes and 4 announce drains), sample(), the println!, and check_row all run 11 or more times per id in a real cargo bench run. Small configs repeat even more, because warm-up keeps doubling iters and re-entering the closure until 200 ms of tick time has passed. As a result, the table interleaves many duplicate rows with Criterion's output, and the 50k x 4 rows pay for a rebuild on every sample. --test (nightly) calls the closure only once, so CI isn't affected. Fix: build, sample, print, and check before group.bench_function, then move relay into a closure that only runs b.iter_custom(...).
2. The "--test runs each once" wording only holds for producer_tick (rs/justfile:350, nightly.yml:11-13). benches/decode.rs's main returns early only on --list. It ignores --test and runs its full sweep (18 encoded ticks, up to 4,096 broadcasts x 4 tiers). That's fine for nightly runtime, but the comment should say decode runs its whole table.
3. The 50k lower bound turns an improvement into a nightly failure (producer_tick.rs:192-201). If a later change shrinks the idle-path entry, such as pruning idle paths from the plain frame or using shorter keys, held * 2 > cap fails nightly even though nothing regressed. Consider printing the cap ratio in the table, or a warning, and keep the hard assert only for held <= cap, which is the limit the producer actually refuses past.
4. Docs: stats-delta.md lost its pointer to this bench. With the Required section gone, the gate paragraph (quest/m2/stats-delta.md:18-26) still cites "about 100 ms per tick at 10k broadcasts" from the prototype and only names decode.rs. Meanwhile bench-coverage.md now says Binary delta stats "gates on that profile". Linking rs/moq-stats/benches/producer_tick.rs there, along with the measured 22 ms (1 tier) and 91 ms (4 tiers) at 10k, would keep the gate checkable.

Verdict: MERGE (once CI is green on af77fe8c)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Replies to the Grok review of af77fe8c, all fixed in 0cab7dc:

1. The relay build, table row, and check_row now run once per id, lazily inside the closure via a captured Option<Relay>, so --list and filtered runs build nothing they don't measure. A filtered real run prints exactly one row.
2. The justfile and nightly comments now say decode ignores --test and runs its whole table.
3. Dropped the held * 2 > cap assert; the table prints a cap% column instead (60% at 50k paths). The hard held <= cap check stays.
4. quest/m2/stats-delta.md's gate now cites producer_tick.rs and the measured 22 ms / 91 ms at 10k paths.

Also merged main (including #5016) into the branch; moq-net and moq-stats tests pass locally (1594/1594) and the bench smoke passes.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up review of 0cab7dcb (re-review after a push; earlier review was on af77fe8c)

This push is the one commit 0cab7dcb on top of the last review. It addresses all four earlier findings, and I found nothing new.

Earlier findings

1. Fixed: relay rebuilt and row reprinted per Criterion sample. producer_tick.rs:210-226 now keeps an Option<Relay> outside bench_function and fills it with get_or_insert_with, so Relay::build, sample, the println!, and check_row run once per id. The relay lives only for that id's loop iteration, so 50k-path relays don't pile up across ids. Filtered-out ids never build, which matches the new comment. iter_custom still calls touch() before every timed tick, so persisting the relay across samples doesn't change what gets measured.
2. Fixed: --test wording. nightly.yml and rs/justfile now say that decode ignores --test and runs its whole table.
3. Fixed: the 50k lower bound. The held * 2 > cap assert is gone, and the table prints a cap% column instead. check_row keeps the hard held <= cap limit plus the grows-with-paths and changed-frame checks. The trade-off is that a shrinking frame now only shows up in the printed column on a manual run, which is the intended behavior.
4. Fixed: docs pointer. quest/m2/stats-delta.md now links rs/moq-stats/benches/producer_tick.rs with the 22 ms (1 tier) and 91 ms (4 tiers) figures at 10k held paths, replacing the prototype's "about 100 ms". That makes it consistent with bench-coverage.md.

CI is pending on 0cab7dcb.

Verdict: MERGE (once CI is green on 0cab7dcb)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 0cab7dc

No new actionable correctness finding in the 13-file change. The benchmark drives the actual collect/publish path, keeps registry entries alive through announce guards, and excludes counter bumps from timing. rs/moq-stats/benches/producer_tick.rs:210–237 keeps one lazy relay per benchmark ID; the Snapshot::update change in rs/moq-stats/src/produce.rs:469–496 preserves normal commit/error behavior while recording emitted bytes. FrameBytes intentionally reports the maximum publisher frame across tiers, not total traffic.

Direction: useful bounded instrumentation, with the earlier repeated-setup and cap-threshold problems addressed. The synthetic byte-counter workload and depth-0 publisher do not characterize every production stats workload; reported timings remain the author's measurements.

Verification: static full diff, relevant source/plans and discussion review. No builds, tests, benchmarks or interop runs executed. Open state, exact head and prior reviews rechecked before posting.

[kixelated]

Merge summary: the OpenAI review of head 0cab7dcb has no findings, and all Grok findings were fixed in 0cab7dcb. CI is green. Auto-merge is enabled on 0cab7dcb.

(Written by Claude Opus 5.5)