perf(connection): pipeline the sync connection handshake - #4203
perf(connection): pipeline the sync connection handshake#4203elena-kolevska wants to merge 2 commits into
Conversation
The sync connection handshake in AbstractConnection.on_connect_check_health issued each setup command as a separate blocking round-trip. For a typical RESP3 connection with a client name and a selected DB this was ~5 sequential round-trips (HELLO/AUTH, optional CLIENT MAINT_NOTIFICATIONS, CLIENT SETNAME, CLIENT SETINFO LIB-NAME, CLIENT SETINFO LIB-VER, SELECT), paid on every connection open, pool growth, and reconnect. Pipeline the tail of the handshake: after HELLO/AUTH (which stays its own round-trip because its reply drives the RESP2->RESP3 parser upgrade, the pre-6.0 AUTH retry, and proto validation), send the remaining commands back-to-back without blocking on a reply between them, then read the replies back in send order. All requests are on the wire before the first read blocks, so the whole tail costs a single round-trip. Each command keeps its original validation / error-swallowing semantics, and per-command check_health preserves the original health-check behavior. This mirrors the async stack (redis/asyncio/connection.py). When maintenance notifications are enabled, the CLIENT MAINT_NOTIFICATIONS command is folded into the same pipelined tail (sent first, read first) instead of costing its own round-trip; the maintenance helpers are refactored (_should_enable_maint_notifications, _maint_notifications_command_args, _send_maint_notifications_command, _add_maint_notifications_to_handshake, _handle_maint_notifications_response) with no change to the behavior of callers that enable notifications outside of on_connect. This cuts the handshake from ~5 round-trips to 2. The saving is ~4 x RTT per connection: measured means at protocol 3 with client_name + db (n>=1000) went 7.64->3.47 ms at 1 ms/RTT and 32.0->13.5 ms at 5 ms/RTT. Add benchmarks/connection_handshake.py to measure connection-handshake latency, with an optional --delay-ms that injects per-round-trip latency so the round-trip reduction is observable on localhost, and a --maint-notifications knob to include the CLIENT MAINT_NOTIFICATIONS command in the measured handshake. Add a regression test asserting the full pipelined tail (maintenance notifications in auto mode with an error reply, plus client_name and db) reads its replies back in order and a following command still succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| self.read_response() | ||
| except ResponseError: | ||
| pass | ||
| def _read_setname_response(): |
There was a problem hiding this comment.
Could we lift these three read handlers out of on_connect_check_health into methods: _read_setname_response, _read_setinfo_response and _read_select_response?
They capture nothing from local scope (only self), so nesting doesn't buy the usual closure benefit, and it re-allocates three function objects on every connect — a path this PR is specifically optimizing.
It'd also match _handle_maint_notifications_response, which is already a proper method; right now the four handlers play the same role in two different shapes.
_read_setname_response and _read_select_response are identical apart from the error string, so they can collapse into one helper:
def _read_ok_or_raise(self, error_message):
if str_if_bytes(self.read_response()) != "OK":
raise ConnectionError(error_message)
with the send site appending functools.partial(self._read_ok_or_raise, "Error setting client name"), plus a small _read_optional_setinfo for the swallow case.
| self.activate_maint_notifications_handling_if_enabled(check_health=check_health) | ||
| # The tail of the handshake (optional CLIENT MAINT_NOTIFICATIONS, then | ||
| # CLIENT SETNAME / SETINFO / SELECT) does not affect control flow -- the replies | ||
| # are only validated or discarded. So we pipeline it: send every command first |
There was a problem hiding this comment.
I think the wording here should be changed a little - we are not really pipelining the commands - maybe it would be better to state that we are optimizing the flow.
In redis-py's context is associated with collecting the commands and sending them all with one write, so this comment might be confusing.
| # are only validated or discarded. So we pipeline it: send every command first | ||
| # (without blocking on a reply between them), then read the replies back in send | ||
| # order. All requests are on the wire before we block on the first read, so the | ||
| # whole tail costs a single round-trip instead of one per command. This mirrors |
There was a problem hiding this comment.
Since not all commands in the async handshake are handled with this approach, a TODO note there would be a nice addition for the commands that are handled completely separately - I think the maintenance notifications flow is added as it was in the sync implementation before this optimisation.
2 participants
Description of change
The sync connection handshake in AbstractConnection.on_connect_check_health issued each setup command as a separate blocking round-trip. For a typical RESP3 connection with a client name and a selected DB this was ~5 sequential round-trips (
HELLO/AUTH, optionalCLIENT MAINT_NOTIFICATIONS,CLIENT SETNAME,CLIENT SETINFO LIB-NAME,CLIENT SETINFO LIB-VER,SELECT), paid on every connection open, pool growth, and reconnect.Pipeline the tail of the handshake: after HELLO/AUTH (which stays its own round-trip because its reply drives the RESP2->RESP3 parser upgrade, the pre-6.0 AUTH retry, and proto validation), send the remaining commands back-to-back without blocking on a reply between them, then read the replies back in send order.
All requests are on the wire before the first read blocks, so the whole tail costs a single round-trip. Each command keeps its original validation / error-swallowing semantics, and per-command check_health preserves the original health-check behavior.
This cuts the handshake from ~5 round-trips to 2. The saving is ~4 x RTT per connection (depending on what's enabled).
Add benchmarks/connection_handshake.py to measure connection-handshake latency, with an optional --delay-ms that injects per-round-trip latency so the round-trip reduction is observable on localhost, and a --maint-notifications knob to include the CLIENT MAINT_NOTIFICATIONS command in the measured handshake.
Add a regression test asserting the full pipelined tail (maintenance notifications in auto mode with an error reply, plus client_name and db) reads its replies back in order and a following command still succeeds.
Pull Request check-list
Note
Medium Risk
Every new sync connection and reconnect goes through the rewritten handshake; incorrect reply ordering would break connects, though behavior is intended to match the prior sequential path and is covered by new tests.
Overview
Sync
Connection.on_connect_check_healthno longer blocks on each reply for setup commands after HELLO/AUTH. It sends the optional tail—CLIENT MAINT_NOTIFICATIONS,CLIENT SETNAME,CLIENT SETINFO, andSELECT—back-to-back, then drains replies in send order via deferred handlers, so the tail costs one round-trip instead of roughly one per command. HELLO/AUTH stays sequential because those replies drive parser upgrades and auth retries.Maintenance notifications are refactored so the command can join that pipelined tail (
_should_enable_maint_notifications, split send/response helpers) instead of a separate post-handshake round-trip when enabled.Adds
benchmarks/connection_handshake.py(optional per–round-trip--delay-msand--maint-notifications) and a test that asserts wire order and that a swallowed MAINT error inautomode does not desync later replies.Reviewed by Cursor Bugbot for commit 234e2be. Bugbot is set up for automated code reviews on this repo. Configure here.