Eliminate multi-assignment churn in the connection pool#1075
Conversation
Assignment does not change an HTTP/1.1 connection's state, so a newly idle connection was handed to every queued request in one pass and re-picked by later passes until the winner sent on it. Every loser woke up, failed with ConnectionNotAvailable, re-entered the queue, and triggered another full O(n) assignment pass - quadratic churn at high queue depth. Drop a connection from the availability snapshot once assigned, and exclude idle connections already reserved by an in-flight request when building the snapshot. 1000 concurrent requests against a local server drop from 5.1s to 0.9s with the default pool, and from 100s to 1.1s with max_connections=1. HTTP/2 multiplexing is unaffected: an active h2 connection is not idle, so it stays available to additional streams.
|
Docs preview: |
Merging this PR will degrade performance by 10.25%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | test_bench_line_decoder |
115.3 µs | 128.5 µs | -10.25% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing pool-single-assignment (0ed00a0) with main (c4e73cf)
Footnotes
-
7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![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: a3e1a2bbc2
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
An idle HTTP/2 connection can serve further requests while reserved, so treating it like HTTP/1.1 could leave a queued burst waiting for the next pool event instead of multiplexing. Add can_multiplex() to the connection interface (False by default, True for established HTTP/2) and only apply the reserved-idle exclusion to connections that cannot multiplex.
11kkw
left a comment
11kkw
left a comment
There was a problem hiding this comment.
The single-assignment fix itself looks sound. I left one follow-up performance concern inline.
Each assignment pass walked every in-flight request even when the pool was saturated, and re-probed reserved idle connections for expiry with an is_readable socket check on every interleaved pass. Break out of the assignment loop once no connection is available and no new one may be created, and skip expiry checks and surplus-keepalive eviction for connections reserved by an assigned request - they were health-checked at assignment time, and evicting them would hand the winning request a closed connection. 1000 unbounded concurrent requests against a local server now complete in 0.41s versus 0.51s sequential, compared to 0.93s before this change and 5.1s before #1075.
|
@11kkw Thanks for the thorough verification. 0ed00a0 implements your point 3 plus reservation-aware sweeping: the assignment loop now breaks once nothing can be assigned, and reserved idle connections skip the |
|
@11kkw ignore the above, it was my coding agent. Sorry. |
Kludex
changed the title
|
@11kkw I'd appreciate a double check from your side here if you have time. 🙏 |
11kkw
left a comment
11kkw
left a comment
There was a problem hiding this comment.
Double-checked the latest changes locally. LGTM, thanks! 👍
2 participants
Summary
Investigating #1074: with many requests queued, throughput collapses. Profiling showed
_assign_requests_to_connectionsat ~70% of runtime, called ~14x per request instead of 2.The cause: assignment does not change an HTTP/1.1 connection's state, so a newly idle connection was handed to every queued request (and re-picked by later passes until the winner sent on it). Every loser woke, failed with
ConnectionNotAvailable, re-queued, and triggered another full O(n) assignment pass - quadratic churn at high queue depth. #974 fixed the per-pass cost but intentionally preserved this behavior; this PR removes it, porting the "Bug 2" half of encode/httpcore#1035.Changes
can_multiplex()to the connection interface (Falseby default,Truefor established HTTP/2) so multiplexing connections are exempt and h2 bursts keep multiplexing.Benchmarks
1000 concurrent
GETs against a local uvicorn server:max_connections=1Parallel is now faster than sequential (0.33s vs 0.55s), confirmed on the discussion author's own benchmark. Incremental reservation bookkeeping to remove the last O(n)-per-pass term follows in #1076.
Verification
PoolTimeout, and threaded syncClientall behave as before.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.