Fix 2 bugs and improve performance from O(N²+NxM) to O(N+M) in _assign_requests_to_connections
Summary
Fixes 2 logic bugs in _assign_requests_to_connections and improves complexity from O(N²+NxM) to O(N+M) where N=connections, M=requests.
Although I attempted a smaller PR with an atomic change, I ultimately decided for a redesign of the method because the algorithm design of the method was fundamentally flawed.
Motivation
_assign_requests_to_connections is in the hot path of this library and the bugs and complexity issue were severly slowing down my application.
Bug 1: max_keepalive_connections is not respected
This causes idle connections to be evicted unnecessarily. This bug is also addressed in this PR (currently approved but not merged).
The bug is on lines 294-295. The expression on line 294 simply returns len(self._connections), ignoring the number of idle connections, and compares this number to self._max_keepalive_connections.
https://github.com/encode/httpcore/blob/5974b03c7df89d3ee4e23779900d5349d550753c/httpcore/_async/connection_pool.py#L294-L295
This test demonstrates the bug. This test fails with the current code on master, but passes with the new implementation of this PR.
Bug 2: multiple requests are assigned to the same HTTP/1.1 connection, leading to ConnectionNotAvailable exceptions
The bug is on lines 322-323. An available connection is assigned to a request, but the status of that connection is not actually changed, resulting in that connection also being assigned to the next request in the queue.
This test demonstrates the bug. This test fails with the current code on master, but passes with the new implementation of this PR.
Perf: Reduce complexity from quadratic to linear
This PR reduces time complexity from O(N²+NxM) to O(N+M) where N=connections, M=requests.
In a typical use case, with for instance 100 connections in the pool and 500 requests in the queue this reduces complexity dramatically, especially since _assign_requests_to_connections is in the hot path of this library because it will be called twice for every request (on start and on finish).
In my use case, this PR reduces the CPU utilization of my application by half. Profiling shows that _assign_requests_to_connections previously accounted for 59% of all samples; after applying this PR, its share dropped to 4%.
The problem: unnecesary nested loops
This snippet contains a nested loop over all connections (O(N²)). This is unnecessary and can be done in a single pass (O(N)).
This snippet contains a nested loops over all requests and connections (O(NxM)). This is unnecessary and can be done in a single pass (O(M)) with early exit if all connections are occupied.
Checklist
- I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
- I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
- I've updated the documentation accordingly.