perf(net): reuse cache ticks for settlement - #3296
Conversation
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: d0842b88d7
ℹ️ 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 pool = cache::Pool::new(config); | ||
| let mut group = c.benchmark_group("track_parallel_write"); | ||
| for writers in WRITERS { | ||
| group.throughput(Throughput::Elements(writers as u64)); |
There was a problem hiding this comment.
For every writers > 1 case, parallel_write divides iterations among the threads, so the benchmark still writes exactly iterations groups in total. Declaring writers elements per iteration therefore inflates Criterion's reported throughput by that factor and makes scaling across writer counts appear better than it is. Use Throughput::Elements(1), or have every writer perform all iterations writes if each iteration is intended to represent one write per thread. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed in ccf463b — by dropping the throughput call entirely rather than setting Elements(1).
One criterion iteration here is one frame write wherever it landed, so Elements(1) would just restate what the per-iteration time already says. The bench's purpose is comparing that per-write cost across writer counts, and an elements/sec line on top of it is a second number that means the same thing.
Worth noting this is also why the PR's original benchmark table was uninterpretable across writer counts: at 8 writers the reported rate was 8x the real one.
(written by Claude Opus 5)
Co-Authored-By: OpenAI Codex <codex@openai.com>
Co-Authored-By: OpenAI Codex <codex@openai.com>
Co-Authored-By: OpenAI Codex <codex@openai.com>
Remove the completed quest and its performance index reference. Co-Authored-By: GPT-5 <noreply@openai.com>
d0842b8 to
aaf4892
Compare
Reshapes the previous commit rather than changing what it does.
`Charge::touch` returned a bare `u64` and `0` when the charge is detached,
but `0` is a real tick: it is the pool's own epoch, indistinguishable from
the first 100 ms of its life. `settle_at(0)` would then wedge the expiry
gate shut for the rest of the process, since `now < next` holds for every
deadline once `next_expiry` moves past zero. Unreachable today, because
`group::Producer::new` always installs a real charge, but nothing in the
signature says so.
`touch`, `add`, and `record_write` now return `Option<u64>` and `settle_at`
folds back into `settle(Option<u64>)`. That also restores the lazy clock
read the previous shape gave up: `settle` sampled `Pool::now()` eagerly for
every caller, where `expiry_due` used to return above it on
`expiry == u64::MAX`. The `None` is resolved below that gate again, so a
pool with no expiry window settles without reading the clock, and the two
datagram call sites in `track::Producer` stop paying for a tick they never
use. The two `match now { Some => settle_at, None => settle }` call sites
collapse into one call each.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sharding spike is settled and the duplicate clock read is gone, but the counters themselves are still unbatched, so the quest is not finished and should not be deleted. Rewrite it around that one open item, and record why sharding is not to be retried as specified, so the next person measures batching on its own rather than inheriting the spike's verdict. Batching reduces how often the shared line is touched; sharding kept the frequency and spread the address. They are not the same experiment. `track_parallel_write` drops its `Throughput::Elements(writers)`: one criterion iteration is one frame write wherever it landed, so declaring `writers` elements per iteration inflated the reported rate by that factor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccf463b0c7
ℹ️ 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".
| barrier.wait(); | ||
| let start = Instant::now(); |
There was a problem hiding this comment.
The main thread releases the barrier before recording start, so any worker scheduled first can execute frame writes outside the measured interval. This can omit a material share of short calibration samples and makes results depend on scheduler timing, especially with more writers. Record the timestamp before barrier.wait(), or add a second synchronization step that starts workers only after the timestamp is captured. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
1 participant
Summary
model::clock::now()read on the frame-write path: the tickCharge::touchalready sampled is threaded intoTrack::settleinstead of being read a second time.Spike decision
The proposed 16-way sharding of the process-wide pool counters was implemented and measured, then abandoned. Repeated results were mixed, and one repeat regressed the existing fanout benchmark by 15.6% at 64 readers and 10.5% at 512 readers. Sharding a counter that is already only relaxed
fetch_adds trades one cheap contended line for several lines plus a read-side fold, and the fold is what showed up. None of that experimental code is retained.What is retained
A frame write read the model clock twice for the same tick: once in
Charge::touchviaPool::stamp, and again inTrack::settleviaexpiry_due.Charge::addandrecord_writenow return the tick they stamped, andTrack::settletakes it asOption<u64>. Write paths hand theirs over; everyone else passesNone.Optionrather than a bare tick, because0is a real tick (the pool epoch) and indistinguishable from the pool's first 100 ms. A0sentinel for a detached charge would makesettle(0)wedge the expiry gate shut for the process lifetime, oncenext_expirymoved past it. It is unreachable today, sincegroup::Producer::newalways installs a real charge, but nothing in the signature said so.The
Optionalso keeps the clock read lazy.expiry_dueresolves theNonebelow itsexpiry == u64::MAXgate, so a pool with no expiry window still settles without reading the clock, and the two datagram call sites intrack::Producerdo not start paying for a tick they never use.This does not coarsen the clock, shard counters, delay accounting, or change eviction and staleness bounds. There is no public API or wire-format change.
Benchmarks
Treat these as not establishing a win, and the change as justified by the removed duplicate read rather than by them.
just bench 3227c195ee2ad2110fd3e672f9a14c00af29e32ecompleted against a fixed dev base, reporting -11.3%/-14.7% at 1 shared-pool writer, -2.9% at 2, neutral at 4, and -12.5%/-6.46% at 8, plus 10.0% at one reader and 11.1% at eight on the existing track fanout with 64 and 512 neutral. That pattern is not monotone in either parameter, and a singleInstant::now()is not 10-15% of a frame write, so it reads as machine noise rather than an effect. End-to-end was mixed in both directions: throughput and loss identical, fanout relay CPU -0.02% and RSS -0.41%, video relay CPU +3.57% and RSS +1.03%.track_parallel_writealso drops itsThroughput::Elements(writers). One criterion iteration is one frame write wherever it landed, so declaringwriterselements per iteration inflated the reported rate by that factor.Quest
cache-shard.mdis rewritten rather than deleted. The sharding spike is settled and the duplicate clock read is gone, but the counters themselves are still unbatched, which was the survey's actual proposal and is untouched here. The quest now carries that one open item plus the spike's verdict, so the next person measures batching on its own: batching reduces how often the shared line is touched, where sharding kept the frequency and spread the address.Validation
just fix,just check, andjust testpass locally (3429 tests, 0 failures).expiry_gate_reuses_a_supplied_tickcovers both directions: a supplied tick drives the gate, and aNonefalls back to the pool clock rather than inheriting the caller's.expiry_gate_stays_closed_without_a_windowcovers the short-circuit that keeps the clock read lazy.(written by Claude Opus 5)