perf(flowcontrol): index active queues so dispatch scans O(active) flows#2120
perf(flowcontrol): index active queues so dispatch scans O(active) flows#2120LukeAVanDrie wants to merge 6 commits into
Conversation
Every dispatch cycle, the fairness policy's Pick calls IterateQueues,
which snapshotted every registered queue in the band (empty ones
included) into a freshly allocated slice under the registry read lock.
Per-dispatch cost was O(registered flows) in CPU and allocation: 94-98%
of all allocation in the flow-control benchmarks, and superlinear
throughput decay under flow churn as registered flows accumulated
between GC cycles.
Each priority band now maintains a lock-free index of its non-empty
queues, updated on empty<->non-empty transitions detected inside the
queue's existing stats critical section (transitions are serialized per
queue by the queue mutex; the index is a sync.Map, so the update never
touches the registry mutex). IterateQueues ranges over the index
directly: no registry lock, no allocation, cost proportional to flows
that actually hold items.
This narrows IterateQueues to active (non-empty) queues, matching its
documented contract ("each active Flow"). Both production callers are
compatible: globalstrict.Pick already skips empty queues, and the
processor's cleanup/drain sweeps are no-ops on them. The view is
eventually consistent; callbacks must tolerate a visited queue reporting
Len() == 0, which they already did under the snapshot implementation.
Benchmarks (M4 Pro, -benchtime=3s unless noted):
- PerformanceMatrix L=1/P=1/F=5000/W=5000: 30.7k -> 277k d/s (9x),
42.2KB -> 1.8KB per op
- TopologyChurn: 16.7k -> 260k d/s (15x), 83KB -> 1.8KB per op
- FullPath (registry churn): 2.3k -> 118k d/s (52x); per-op cost now
flat with run length (was superlinear: 106us at 24k flows minted ->
439us at 73k)
Signed-off-by: Luke Van Drie <lukevandrie@google.com>
|
/assign @shmuelk After this, it's probably not worth making any more targeted perf improvements as we are definitely not a bottleneck anymore. My main goal was to reduce unnecessary temp allocs which create GC pressure which can bleed out and affect perf of other parts of EPP. |
f11bdd5 to
6144fcb
Compare
…eincarnated flow ID A cleanup-sweep worker can drain a queue through a handle resolved before deleteFlow removed that queue from the registry. If a successor queue was registered under the same flow ID and became active in the interim, the stale drain's empty transition unconditionally deleted the successor's index entry, hiding a live, non-empty queue from IterateQueues (dispatch and future sweeps) until flow GC reaped it. Deactivation now uses CompareAndDelete so it only removes the entry while it still points at the transitioning queue. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
|
Found some issues, converting this back to draft while I rework this. edit: Done and moved back out of DRAFT |
The LAS strategy applied attained-service decay only when Pick visited a program with an empty queue. That coupled decay to dispatch-loop visit cadence, and with PriorityBandAccessor.IterateQueues narrowed to active (non-empty) queues, idle programs were never visited at all: their service froze between bursts and the pending decay was forfeited on the next completion (AddService reset the anchor without applying it). Decay is now folded in lazily from the existing decayAnchor whenever a program's service is read (Pick) or accumulated (OnCompleted), so it is defined purely in wall-clock terms and needs no visits. lasDecayFactor is consequently a per-second rate when lasHalfLifeSeconds is 0, replacing the per-Pick semantics whose effective half-life scaled with dispatch frequency. The default configuration selects a 60s half-life, matching the order of the previous default's effective behavior at typical dispatch rates. Decay accrues continuously, including while requests are in flight; the in-flight pause is gone along with the visit-driven model. Pick no longer touches strategy state for empty entries, so a program being evicted concurrently with a Pick cannot be resurrected into an orphaned state-map entry. Signed-off-by: Luke Van Drie <lukevandrie@google.com>
|
cc: @D-Sai-Venkatesh and @praveingk This perf improvement necessitates changing an aspect of the program aware fairness plugin. Would like your feedback on this to make sure my changes are aligned with your intentions for how decay works. |
|
Hey @LukeAVanDrie, sure will take a look and post our comments here. |
Thanks, no urgency here as this is not a release blocker for v0.10. I just fixed up the go-bench recently, got fresh results, and sent a fix out for the worst cpu and mem bottlenecks. |
|
@LukeAVanDrie Please rebase. It will fix the CI failure |
|
cc: @jtechapps I have my micro-bench framework for the flow control runtime itself, but I was wondering if I am able to use your new perf tooling to investigate overall EPP perf impact before merging (I know it's a nightly). |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Every dispatch cycle, the fairness policy's
PickcallsPriorityBandAccessor.IterateQueues, which snapshotted every registered queue in the band (empty ones included) into a freshly allocated slice under the registry read lock. Per-dispatch cost was O(registered flows) in CPU and allocation (94-98% of all allocation in the flow-control benchmarks) and throughput decayed superlinearly under flow churn as registered-but-idle flows accumulated between GC cycles. The 1 ms dispatch ticker pays this cost even at zero traffic.Each priority band now maintains a lock-free index of its non-empty queues, updated on empty<->non-empty transitions inside the queue's existing stats critical section (transitions are serialized per queue by the queue mutex; the index is a
sync.Map, so the update never touches the registry mutex).IterateQueuesranges the index directly: no registry lock, no allocation, cost proportional to flows that currently hold items.This narrows
IterateQueuesto active (non-empty) queues, matching its documented contract ("each active Flow"). There are three production callers.globalstrict.Pickalready skips empty queues and the processor's cleanup/drain sweeps are no-ops on them. Theprogram-awareLAS policy, however, used empty-queue visits to drive attained-service decay, so the second commit makes decay lazy: it is folded in from the decay anchor whenever a program's service is read or accumulated, independent of visits. This changeslasDecayFactorto a per-second rate (it was per-Pick, so its effective half-life varied with dispatch frequency) and the default config now uses a 60s half-life. Flagging that for LAS reviewers specifically.The view is eventually consistent; callbacks must tolerate a visited queue reporting
Len() == 0, which they already did under the snapshot implementation (a snapshotted queue could drain before the callback ran).Index deactivation uses
CompareAndDeleterather than a plain delete: a cleanup-sweep worker can drain a queue through a handle resolved beforedeleteFlowremoved it, and if a successor queue was registered under the same flow ID in the interim, an unconditional delete would hide the live successor from dispatch and future sweeps. A regression test covers this interleaving.Note for reviewers: iterating under the registry read lock instead was considered and rejected. The cleanup sweep calls
registry.ManagedQueue()(which takes the read lock) from inside the iteration callback, so yielding under the lock is a recursiveRLockthat deadlocks when a writer is queued.Benchmarks (M4 Pro,
-benchtime=3s):Which issue(s) this PR fixes:
Part of #1187
Release note: