Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9499 +/- ##
=======================================
Coverage 87.71% 87.72%
=======================================
Files 436 436
Lines 30822 30837 +15
=======================================
+ Hits 27036 27052 +16
+ Misses 3785 3784 -1
Partials 1 1
🚀 New features to boost your workflow:
|
|
How did you run into this? Is any user/customer seeing this being a problem in practice? |
|
Hi Easwar, I was testing another controlbuf improvement (#9444, now merged), which I expected to improve QPS by only a couple of percent, since controlbuf wasn't prominent in the flame graphs. It turned out to have a much larger impact: roughly 10–15% higher QPS in local benchmarks and about 5% in the k8s benchmarks.
That suggested controlbuf is a bigger bottleneck than the profiles show, and that optimizing it pays off more than expected. So I used Claude Opus to analyze the profiles from a local benchmark run alongside the transport code, looking for other areas to improve. One of its suggestions was to batch controlbuf drain operations, so that their cost is amortized instead of scaling with the number of streams. Based on the profiles, it estimated a 1–2% QPS gain. Once again, the measured improvement was much larger: around 8% on top of #9444.
I'm not aware of a specific user report. Since this overhead doesn't stand out in profiles, it's unlikely that a user could trace a performance problem back to this code path. Still, users regularly benchmark gRPC Go against other RPC frameworks and against REST clients when picking a library for their services. Combined with the already-merged #9444, this change brings a roughly 10–20% improvement in gRPC Go unary performance in a single release, which directly helps how gRPC Go compares in such evaluations. |

Today, loopy acquires
controlBuffer.muonce for every control frame it reads. Stream goroutines acquire the same mutex input()for every frame they queue, so the lock is constantly handed back and forth between loopy and the producers, and both sides frequently find it held.This PR makes loopy take all the queued frames in a single critical section. When its local batch is empty,
get()swapscontrolBuffer.listwith an empty, loopy-ownedconsumerList(no allocation, since the two ring buffers just trade places) and then returns frames fromconsumerListone at a time without holding the lock. Loopy's lock acquisitions drop from one per frame to one per batch.Loopy still calls
get()for each frame and callsprocessData()after handling it, so writes remain round-robin across streams.Throttling
The number of queued transport response frames is used to throttle the reader (
throttle()). With batching, frames leave the control buffer's list in bulk, but should only count as consumed once loopy dequeues them from its batch.transportResponseFramesis now anatomic.Int64counting the frames in bothlistandconsumerList. Producers increment it while holdingmu; loopy decrements it without holdingmu.updateThrottleLocked(), which is called while holdingmuwhenever the count crossesmaxQueuedTransportResponseFrames. Loopy only acquires the lock for a downward crossing, which is rare.updateThrottleLocked()acts on the current count rather than on the direction of the crossing that triggered it. Because the consumer decrements without holding the lock, the count may have crossed the threshold again by the time the update runs. A naive edge-triggered version (create on the upward crossing, close on the downward crossing) can replace a channel that a reader is blocked on without closing it, leaving the reader stuck inthrottle(). Being idempotent and serialized bymu, the last update always runs after the last crossing and leaves the channel in the correct state.Shutdown
On the server,
finish()can be called fromClose()on a different goroutine than loopy, so it cannot touch loopy'sconsumerList. Loopy cleans up any unhandled frames in its batch viadiscardConsumerList()after callingfinish(), using the same cleanup logic (orphaningclientHeaders, freeing data of unprocesseddataFrames).closedis now anatomic.Bool(still only written undermu) so thatget()can stop handing out frames from the batch once the control buffer is finished, matching the existing behavior.Benchmarks
benchmainwith-bufconn -networkMode=Local -compression=off -benchtime=60s,GOMAXPROCS=4. Each round runsmasterand this PR at the same time, pinned to separate sets of 4 physical cores (usingtaskset), alternating core sets between rounds.Unary, 100 concurrent calls, 0-byte payloads
Streaming, 200 concurrent calls, 100-byte payloads
Bytes/op and allocs/op are unchanged (within ±0.05%) in all rounds.
RELEASE NOTES: