perf(autopipeline): better pool utilization on stranglers - *Client - #3962
perf(autopipeline): better pool utilization on stranglers - *Client#3962ndyakov wants to merge 10 commits into
Conversation
…as a free connection The async autopipeline held 1-3 queued "straggler" commands until the in-flight batch's reply landed (~1 RTT) whenever any batch was executing, so on a slow link an uncached command that enqueued behind an in-flight batch paid ~2x RTT. A phase trace on a deterministic 50ms link pinned it: service (round trip) was clean at 1 RTT and the concurrency permits were never exhausted, but the straggler-hold re-armed for a full RTT (uncached p95 stuck at 107ms / 2x RTT at low-to-mid concurrency). Bound that hold to a few silence gaps (stragglerHoldGaps * gap, which tracks the round trip) ONLY when the pipeline pool has a connection to spare (an idle conn, or room to dial — the pipeline pool runs MinIdleConns=0). When the pool is saturated, keep the original long hold: flushing tiny batches into a full pool thrashes it because they cannot coalesce into the deep pipelines the scarce connections need (an unconditional few-ms bound cut throughput ~7x at a squeezed pool). The 30s autoPipelinePermitBackstop remains the flush-path safety ceiling. The pipeline pool is captured once at construction via an in-package assertion and is nil for clients without one (e.g. *ClusterClient), which then keep the prior long hold. Measured (50ms proxy, churn, inflight 1): default pool uncached p95/p99 111->65ms; squeezed pool (pipeline-pool 2, uncached-heavy) no throughput regression; real WAN uncached p99 314->177ms (residual is link jitter, present on the untouched cached path too).
ndyakov
changed the title
There was a problem hiding this comment.
Pull request overview
Improves async autopipelining latency on “straggler” commands by shortening the coalescing hold time when the pipeline pool has spare connection capacity, while retaining the longer hold under pool saturation to avoid throughput collapse.
Changes:
- Captures (intended) pipeline connection pool on
AutoPipelinerconstruction to infer pool pressure during straggler coalescing. - Adds
pipelineHasFreeConn()andstragglerHoldGapsto bound the straggler-hold to a few silence gaps when a connection is idle or dial-able. - Updates
awaitExpectedArrivalsto choose between the bounded hold and the original long hold based on pool pressure.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: dddab0267f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…e is live The straggler-hold pool gate reaches the pipeline pool via an in-package interface assertion (getPipelinePool() pool.Pooler) in newAutoPipeliner. On master that accessor does not exist, so the assertion fails silently: ap.pipelinePool stays nil, pipelineHasFreeConn() always returns false, and the gate degrades to the conservative long hold — the fix compiles and passes CI while doing nothing. Add the accessor (returns the baseClient pipeline pool) so the assertion succeeds and the gate actually engages. Tests assert the AutoPipeliner captures a non-nil pool (gate live) and the safe nil fallback when no pipeline pool is configured. Note: PR #3959 introduces its own getPipelinePool over a pipelinePoolRef refactor; when both land keep one definition (the #3959 ref-based form) and drop this plain accessor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e9112a36e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ofekshenawa
left a comment
ofekshenawa
left a comment
There was a problem hiding this comment.
Looks great, see the codex comments, especially https://github.com/redis/go-redis/pull/3962/changes#r3764957653 .
Rather than that, all looks good!
pipelineHasFreeConn() used `IdleLen()>0 || Len()<Size()`, which ignores MaxActiveConns: a pipeline pool with MaxActiveConns < PoolSize and no idle connection reported free even though the flush's Get would hit ErrPoolExhausted, so the straggler-hold shortened its hold into a pool it could not actually acquire from (ofek's named change-request blocker; codex #3962). Add ConnPool.HasFreeCapacity(): it keeps the existing idle / dial-able terms and adds the one the heuristic missed -- MaxActiveConns hard-caps dials on poolSize (newConn returns ErrPoolExhausted at poolSize >= MaxActiveConns). The gate prefers this probe (every *ConnPool implements it) and keeps the old heuristic as a fallback for any other Pooler, so with MaxActiveConns == 0 the result is unchanged. Adds internal/pool tests for the max-active-saturated and no-max-active branches.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bbd0fa2a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pipeline_pool_getter_test.go:50
- This test also hard-codes ":6379"; prefer internalTestRedisAddr() for consistency with the autopipeline internal tests (autopipeline_internal_test.go:25-30) and to respect REDIS_PORT.
c := NewClient(&Options{Addr: ":6379"}) // no pipeline buffers => no pipeline pool
pipeline_pool_getter_test.go:19
- This test hard-codes ":6379" even though the existing autopipeline internal tests use internalTestRedisAddr() (autopipeline_internal_test.go:25-30) to honor REDIS_PORT. Using the shared helper keeps this test consistent with the suite and resilient to non-default ports and future changes that might dial during AsyncAutoPipeline setup.
This issue also appears on line 50 of the same file.
c := NewClient(&Options{
Addr: ":6379",
PipelineReadBufferSize: 64 * 1024,
PipelineWriteBufferSize: 64 * 1024,
PipelinePoolSize: 4,
})
- HasFreeCapacity now counts only USABLE idle conns (usableIdleLen) and requires a free pool turn (new FastSemaphore.Available(), which accounts for in-flight dials) before declaring capacity. Adds an unusable-idle test. - The solo-straggler flush now dispatches via processPipeline (one-command pipeline) on the pipeline pool, so it uses the same pool the gate probes; processPipeline falls back to the main pool when none exists (no-op for default clients). The solo goroutine dispatch (the 2xRTT phase-lock fix) is unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b48e459216
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/pool/pool.go:1714
ConnPool.HasFreeCapacityis documented as a non-blocking probe for whether aGetcan proceed without blocking, but it doesn't considerMaxConcurrentDials(thedialsInProgresssemaphore). When there are no usable idle conns and the pool needs to dial,queuedNewConncan still block ondialsInProgresseven thoughsemaphore.Available()>0andpoolSize < PoolSize, so this probe can return a false positive and shorten the straggler hold in precisely the situation where the flush will block on dialing.
if p.semaphore.Available() <= 0 {
return false
}
size := p.poolSize.Load()
if size >= p.cfg.PoolSize {
autopipeline.go:2411
- The singleton fast-path now dispatches via
processPipeline(ctx, []Cmder{cmd})instead ofprocess(ctx, cmd). That changes behavior for CSC-enabled clients:processcan take theprocessCachedpath for cacheable commands, but the pipeline path only runscscCommandErrorand always goes to the server. If autopipelining is intended to remain a drop-in for clients using client-side caching, consider keepingprocessfor cacheable/CSC-enabled singletons or adding a pipeline-pool-aware single-command processor that still uses theprocessCommand/processCachedlogic while borrowing from the pipeline pool.
err := ap.pipeliner.withProcessHook(context.Background(), solo, func(ctx context.Context, cmd Cmder) error {
// Dispatch on the PIPELINE pool (as a one-command pipeline), not the
// main pool, so the flush uses the same pool the straggler-hold gate
// probes (codex #3962). processPipeline falls back to the main pool
// when no dedicated pipeline pool exists, so this is a no-op for
// default clients and only unifies the pools when one is configured.
// The solo goroutine dispatch (which is what avoids the 2xRTT
// flusher phase-lock) is unchanged.
return ap.pipeliner.processPipeline(ctx, []Cmder{cmd})
})
- HasFreeCapacity: when a dial is required, also require a free dial slot (MaxConcurrentDials below PoolSize can leave all slots busy while a turn is free); and usableIdleLen excludes idle conns marked ShouldHandoff (an OnGet hook would divert them). Adds TestHasFreeCapacityExcludesHandoffIdle. - Solo autopipeline flush routes a cacheable command through the single-command Process path so client-side caching is honored (processPipeline bypassed the processCached branch, dropping CSC hits for one-command flushes); non-cacheable solos still dispatch on the pipeline pool.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1773fa2adf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A cacheable solo straggler was routed through Process (main pool) whenever the command type was cacheable, even with client-side caching off (the default) — so it ignored the dedicated pipeline pool the straggler gate probed and could contend on a saturated main pool while pipeline capacity sat free. Gate that routing on cscEnabled (captured at construction): only a CSC client sends a cacheable solo through the cache-honoring path; otherwise it dispatches on the pipeline pool.
setup-go's "1.26.x" resolved to go1.26.5, which govulncheck flags for two standard-library vulnerabilities fixed in go1.26.6: GO-2026-6090 (crypto/tls) and GO-2026-5972 (encoding/asn1). Track the latest stable toolchain so future security patches are picked up automatically instead of pinning a patch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091b606183
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment-only: HasFreeCapacity/usableIdleLen docs reduced to their check invariants; the cacheable-solo CSC gate comments deduplicated into the cscEnabled field doc; test headers condensed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc4d3fed0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- HasFreeCapacity reports no capacity while the dial circuit breaker is open (dialErrorsNum >= PoolSize, dialConn's fail-fast condition): room under PoolSize is not dialable capacity when every dial errors immediately, so the straggler gate keeps the conservative hold instead of flushing into a failing dial path. - The cacheable-solo CSC gate is live (the client's autopipelineCSCActive func, consulted per dispatch) instead of a construction-time bool: when CSC disables itself mid-life, cacheable solos return to the pipeline pool. The routing test pins the mid-life flip.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 11f1607. Configure here.
Get acquires a turn before popIdle, so with the semaphore exhausted even a usable idle conn is not immediately servable — and putConnWithoutTurn can re-pool a conn while its turn stays held, so a positive idle count does not imply a free turn. The turn check now runs first; the idle short-circuit only applies when a turn is available.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63bef1d28b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The async autopipeline held 1-3 queued "straggler" commands until the in-flight batch's reply landed (~1 RTT) whenever any batch was executing, so on a slow link an uncached command that enqueued behind an in-flight batch paid ~2x RTT. A phase trace on a deterministic 50ms link pinned it: service (round trip) was clean at 1 RTT and the concurrency permits were never exhausted, but the straggler-hold re-armed for a full RTT (uncached p95 stuck at 107ms / 2x RTT at low-to-mid concurrency).
Bound that hold to a few silence gaps (stragglerHoldGaps * gap, which tracks the round trip) ONLY when the pipeline pool has a connection to spare (an idle conn, or room to dial — the pipeline pool runs MinIdleConns=0). When the pool is saturated, keep the original long hold: flushing tiny batches into a full pool thrashes it because they cannot coalesce into the deep pipelines the scarce connections need (an unconditional few-ms bound cut throughput ~7x at a squeezed pool). The 30s autoPipelinePermitBackstop remains the flush-path safety ceiling.
The pipeline pool is captured once at construction via an in-package assertion and is nil for clients without one (e.g. *ClusterClient), which then keep the prior long hold.
Measured (50ms proxy, churn, inflight 1): default pool uncached p95/p99 111->65ms; squeezed pool (pipeline-pool 2, uncached-heavy) no throughput regression; real WAN uncached p99 314->177ms (residual is link jitter, present on the untouched cached path too).
Review refinements
HasFreeCapacitywas tightened from the review: when a dial would be needed italso requires a free dial slot, and idle connections that are unusable or marked
for handoff do not count as capacity. The solo-straggler dispatch routes a
cacheable command through the CSC-honoring Process path ONLY when client-side
caching is actually active — otherwise it stays on the pipeline pool the
straggler gate probed.
Note
Medium Risk
Changes hot-path autopipeline flush timing and solo dispatch routing; behavior is gated on pool probes and live CSC state, with conservative fallbacks when the pipeline pool is unknown or saturated.
Overview
Improves async autopipeline latency when 1–3 commands queue behind an in-flight batch: instead of always waiting ~1 RTT for the prior batch to finish, the straggler hold is capped at a few RTT-scaled silence gaps only when the dedicated pipeline pool has spare capacity (
stragglerHoldGaps); if the pool is saturated, the old long hold stays to avoid tiny flushes that thrash a tight pool.The pipeliner now captures the pipeline pool at construction (
getPipelinePool) and probes occupancy via newConnPool.HasFreeCapacity()(turns,MaxActiveConns, dial slots, usable idle conns — not handoff/unusable entries), withFastSemaphore.Available()supporting the turn check.Single-command (solo) stragglers dispatch through
processPipelineon the pipeline pool by default; cacheable solos useProcess(CSC path) only whenautopipelineCSCActive()is true at dispatch time, so inactive or mid-life-disabled CSC does not steal main-pool connections.CI: govulncheck uses Go
stableinstead of1.26.x. Regression tests cover pool capture,HasFreeCapacity, and CSC solo routing (#3962).Reviewed by Cursor Bugbot for commit 63bef1d. Bugbot is set up for automated code reviews on this repo. Configure here.