test(flowcontrol): harden and reorganize the flow-control benchmark suite#1638
Conversation
github-actions
Bot
added
size/XL
9831de9 to
636836e
Compare
|
cc: @RishabhSaini doing some re-organizing as our bench setup is getting complex |
|
Note to reviewers: This is a very low-priority PR in a low-churn path. We can wait a bit before merging this |
| // The complication: the processor also runs dispatchCycle on a 1ms ticker when the queue is EMPTY | ||
| // (notably at startup). Those idle grants never dispatch and so are never Released; a naive detector | ||
| // would latch saturated forever and deadlock every request until its TTL. We distinguish leaked | ||
| // idle grants from genuine load saturation with releaseCount: while releases are | ||
| // flowing the saturation is real; if two consecutive saturated reads see zero releases between them | ||
| // the outstanding grants are leaked idle permits, so we reclaim one. The 1.0/0.99 split sits below | ||
| // usagelimits.DefaultPolicy's 1.0 ceiling, so a grant never trips head-of-line gating. | ||
| // | ||
| // stuckReads and lastReleaseCount are touched only by the single-threaded dispatch loop (Saturation) | ||
| // and so need no synchronization; inFlight and releaseCount are atomic (Release runs on workers). |
There was a problem hiding this comment.
+1 Currently the benchmark fails to run precisely due to the above complication.
My question is, what happens in the benchmark when there is true queueing, even if momentary queueing? Shouldn't there be true saturation (i.e. Inflight > level) ?
There was a problem hiding this comment.
Yes, under true queueing the detector does report true saturation: once in-flight (un-Released) grants reach L, every subsequent Saturation read returns 1.0 (covered by TestBenchDetector_SaturatesAtLimit). The reclaim path never fires while that saturation is genuine, because genuine load means dispatched workers are calling Release, and any release observed between two saturated reads resets the stuck counter and keeps returning 1.0.
The reclaim only triggers on consecutive saturated reads with zero releases in between which the signifies the empty-queue idle-tick leak (a grant that never led to a dispatch, so no Release can ever arrive for it). The one edge case is a genuine holder whose worker is descheduled long enough to stall its Release past the threshold; then we reclaim spuriously and transiently admit past L. For a CPU-cost benchmark, briefly loosening L just shifts the coordinate's operating point, while the opposite failure mode (latching saturated) deadlocks the run until request TTL, which is exactly the L=1 hang this PR fixes.
I've expanded the comment on stuckReadThreshold in the latest commit to spell this out.
|
@LukeAVanDrie would you like to address the comments and move this one forward? |
* typo * apply review's suggestion * update * move otel env to helm template * update * skip lint * fix lint * add file header * update README.md * typo * apply review's suggestion
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
|
@LukeAVanDrie is this still relevant or can be closed? |
yes, but low priority and fell through the cracks this is just for maintainability will rebase today, resolve conflicts and address feedback; this isn't critical for the release though |
…uite Test-only changes to pkg/epp/flowcontrol/benchmark; no production code is touched. - Re-scope the full-path benchmark (FullPathStress -> FullPath) to what it uniquely measures: dispatch throughput and allocation overhead through the real producer -> concurrency detector -> controller path. Drop the off-layer producer leak-counter assertion (covered by the inflightload producer's own unit tests), report d/s like the other benchmarks, and fix a b.Fatalf called from inside RunParallel (invalid off the benchmark goroutine). - Fix the mock benchDetector so the PerformanceMatrix L sweep is meaningful: it accumulates in-flight up to the limit and reports saturated once exceeded (real W>L backpressure), while self-healing the empty-queue idle grants the processor's 1ms dispatch ticker would otherwise leak (which previously latched and hung at L=1). Distinguish leaked idle grants from genuine load saturation via a release counter. Add detector unit tests covering saturation-at-limit and self-heal. - Replace fixed bootstrap time.Sleep calls with a warmup request that blocks until dispatch -- deterministic readiness instead of arbitrary timing. - Reorganize into doc.go (two-family overview) + helpers_test.go + benchmark_test.go, and dedup the shared registry/policy setup. - Add a bench-smoke make target and CI step (go test -bench=. -benchtime=1x) so benchmark bodies execute on every PR; go test only compiles them otherwise, letting runtime breakage (panics, deadlocks) go undetected. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
The inline comment claimed the FullPath benchmark sustains W>L queuing, but worker count rounds down to <= L and the real detector's in-flight load is released near-instantly, so dispatch is effectively free-flowing. Reword to match the function godoc: FullPath prices dispatch + the detector's per-cycle DynamicAttribute reads, not sustained backpressure. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
- Use the llm-d Authors copyright header on the new files (doc.go, detector_test.go) per review. - Fail fast if the PerformanceMatrix warmup request is not dispatched, matching the full-path harness, instead of silently timing a dead dispatch loop. - Fail fast if a matrix coordinate dispatches zero requests, so a wedged dispatch path produces a red build instead of plausible zero rows. - Drop the matrix harness TTL from 5m to 30s: nothing in a throughput benchmark legitimately waits minutes, and a tight TTL bounds a wedged coordinate to seconds of CI time instead of minutes per cell. - Document the liveness bias of the benchDetector's stuck-read reclaim heuristic: true queueing still reports saturated while releases flow; a stalled release transiently over-admits rather than latching. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
cca0e9e to
96f8549
Compare
rebased and feedback addressed |
|
/retest |
4 participants
What type of PR is this?
/kind test
/kind cleanup
What this PR does / why we need it:
Hardens and clarifies the flow-control benchmark suite (
pkg/epp/flowcontrol/benchmark). Test-only; no production code changes.FullPathStress→FullPath) to what it uniquely measures: dispatch throughput and allocation overhead through the real producer → concurrency detector → controller path. Drops an off-layer producer leak-counter assertion (covered by the inflightload producer's own unit tests), reportsd/slike the other benchmarks, and fixes ab.Fatalfcalled from insideRunParallel(invalid off the benchmark goroutine).benchDetectorso thePerformanceMatrixL sweep is meaningful: it accumulates in-flight up to the limit and reports saturated once exceeded (real W>L backpressure), while self-healing the empty-queue idle grants the processor's 1 ms dispatch ticker would otherwise leak. Previously this latched and hung at L=1. Adds detector unit tests for saturation-at-limit and self-heal.time.Sleeps with a warmup request that blocks until dispatch for deterministic readiness instead of arbitrary timing.doc.go(two-family overview) +helpers_test.go+benchmark_test.go, and dedup the shared registry/policy setup.bench-smokemake target + CI step (go test -bench=. -benchtime=1x) so benchmark bodies execute on every PR;go testonly compiles them otherwise, letting runtime breakage (panics, deadlocks) go undetected.Which issue(s) this PR fixes:
N/A -- expanding upon the work in #1543
Release note (write
NONEif no user-facing change):