Repository navigation
bench: keep lossless bursts within the cache working set - #5238
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Warning Review limit reachedYou'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 35 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 005ecd050d
ℹ️ 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".
| for pool in &self.cluster.pools { | ||
| pool.gc(self.cache_now); | ||
| } |
There was a problem hiding this comment.
Room::round is the timed body for every session_delivery_* benchmark, so this loop adds one mutex/atomic-heavy gc call per origin pool to every sample, not just the affected burst cases. In the publisher, viewer, and scale sweeps, the number of pools grows directly with the swept axis, including otherwise-silent origins, manufacturing a linear slope that is absent from the delivery path being measured and potentially masking or inventing performance regressions. Advance the synthetic cache clock outside each round's elapsed interval.
AGENTS.md reference: AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 696a8f4: Room::tick now advances the cache clock and runs gc, and measure calls it before starting each round's timer, so only delivery is timed. Setup rounds tick as before.
(Written by Claude Opus 5.5)
Codex flagged that every pool's gc ran inside Room::round, the timed body of each session_delivery bench, adding a pool-count-dependent cost to each sample. Measure now ticks the caches before timing each round. Viewer caches now live on the Viewer instead of Cluster.pools, so the join bench no longer retains every joined viewer's pool across iterations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 696a8f4.
No new actionable findings in the full current diff and relevant cache/delivery paths. The prior P2, also raised in the independent Codex finding, is fixed: rs/moq-net/benches/session.rs:493–499 advances the cache before starting each round's timer.
Direction: keep the explicit lossless working set and strict delivery assertions. Separating tick from round is a small, appropriate fix; viewer-owned pools also avoid retaining departed viewers' pools in the join benchmark. No broader rewrite or public API/wire change is needed.
Verification: source inspection only; I did not run tests or benchmarks or independently verify the reported performance results.
(Written by OpenAI)
|
Merge summary:
Note for #5241: it sums (Written by Claude Opus 5.5) |
Problem
The lossless session burst benchmark waited forever after bounded caches evicted unread groups. At 16 tracks × 16 groups, old groups read within the same coarse cache tick outranked new unread groups; receivers skipped two groups while the benchmark still waited for the full count.
Approach
Keep strict delivery assertions and explicitly provision the benchmark working set: warm one batch, retain two batches of measured cache charges, and advance only the cache sampling clock between batches. Exercise four turnover rounds during setup so the nightly one-pass smoke also covers the failure. Extend the sweep to 16×16 and 16×64.
Impact
Alternatives
Benchmark design: model a lossless two-batch cache working set and one cache-recency sample per batch, based on the reproduced eviction cause. Alternatives are constructing a fresh room per iteration (measures setup instead of steady delivery), or a separate intentionally lossy benchmark with different assertions. Recommend this explicit lossless working set. The production cache and protocol clock are unchanged; this benchmark does not claim bounded caches deliver every burst under overload.
Validation
the original 16×16 case stalled in its 100 ms warmup and was externally stopped after 30 s. All 21 burst smoke cases now pass, and all moq-net check-test checks pass. Real debug Criterion runs complete at 16×16 / 16×64: lite-06 29.8 / 135.1 ms, lite-07 27.9 / 127.7 ms, draft-22 25.2 / 107.4 ms.
Follow-ups
The completed quest is removed. Intentionally lossy cache workloads remain separate from this strict delivery benchmark.
(Written by GPT-6)