perf(http): cut per-request syscalls from 83 to 14 — cached addresses + no redundant setsockopt (#1719)

[paul-hammant]

Four data-driven steps at the LB performance gap (#1719). +4.9%, controls flat, no new test failures.

@nicolas-maman — flagging you for a must-review: change 2 touches the parking path and makes one of your comments true that wasn't. Your measurements hold up; it's the change-check the comment describes that was missing. No question for you — I had one about your parking poll and withdrew it once I split the measurement by caller; see the bottom.

Result

Same box, same harness, only the Aether binary changed. nginx and haproxy are measured in the same runs as controls:

LB	before	after	change
aether	48,073	50,445	+4.9%
nginx (control)	67,723	67,721	−0.003%
haproxy (control)	63,151	62,969	−0.29%

Both controls held under 0.3% while Aether gained 4.9%, so the change is the cause rather than box drift. Gap closes 71% → 74.5% of nginx.

Syscalls per request, strace -c -f over 20,000 requests:

	before	after
total syscalls/request	83.0	14.0
setsockopt	202,552 calls	72
getpeername	1.0/req	0
getsockname	1.0/req	50 total (one per connection)

Change 1 — addresses resolved per connection, not per request

getpeername/getsockname + 2 inet_ntop + 2 strdup ran on every request. Neither address can change while a socket is open. Now cached on HttpConn.

Cheap now in a way it wasn't when written: connection parking (#1663 / #1684) made HttpConn heap-allocated and connection-lifetime, so there is somewhere to put them. The request still owns its copies — http_request_free frees both — so only the syscalls go, not the allocations.

Change 2 — stop re-applying socket timeouts that are already set

This is the one #1719 couldn't pin down. The issue said:

"setsockopt ×2 — I could not pin this to a single site with certainty … Someone who knows the upstream path should confirm whether that runs per request or per pooled connection."

Answer, from a raw strace: per request, at two sites, both on the keep-alive path.

a) Upstream pooled connections (aether_http.c:1663). Every reuse re-applied SO_RCVTIMEO+SO_SNDTIMEO to the same value the previous request left there. Here is request 3 reusing pooled fd 6 — no connect, yet both options set again:

setsockopt(5, SOL_SOCKET, SO_RCVTIMEO_OLD, "\36\0\0\0...", 16) = 0
setsockopt(6, SOL_SOCKET, SO_RCVTIMEO_OLD, "\36\0\0\0...", 16) = 0
setsockopt(6, SOL_SOCKET, SO_SNDTIMEO_OLD, "\36\0\0\0...", 16) = 0

A Transport travels with its connection into the idle pool, so it can remember what it applied. The unguarded form stays on the dial path, where the socket is new and its option state genuinely unknown.

b) The client connection — and this is the part that needs your eyes. conn_serve applies the idle timeout on entry, and with parking a kept-alive connection re-enters conn_serve once per request. Your comment at aether_http_server.c:3725 (from fbc111c, PR #1684) states:

"The window is only re-applied when it changes, so a steady connection pays one setsockopt, not one per request."

That guard did not exist. In fbc111c, conn_apply_recv_timeout took a bare fd, kept no previous value anywhere, and called setsockopt unconditionally — the helper's own header comment says it is "applied whenever a worker is about to block on this connection", which is the accurate description. The parking comment read as though a change-check was in place; the trace above is fd 5 getting the call on every request.

Rather than rewrite your comment I've added the guard so it's now literally true — but say if you'd rather it read differently. Easy thing to miss: without parking, "whenever a worker blocks" was once per connection, and it's parking that quietly turned it into once per request.

Both sentinels are -1, not 0, because 0 is a legitimate timeout meaning "block indefinitely" and must not read as "never configured". HttpConn is calloc'd, so its field is set explicitly after allocation.

Change 3 — the idle pool stops walking its list every request

http_pool_take and http_pool_put each swept the whole idle connection list under the global pool mutex, on every request. With the default 15s idle window and a proxy reusing upstreams continuously, that sweep frees nothing almost every time — a list walk per request to find there is nothing to do. It now records when the earliest connection becomes eligible and skips the walk until then. The per-key cap check also stops at the cap rather than walking to the end.

This one produced no measurable win — 48,854 → 48,870 rps over three alternating rounds, with the baseline ahead in two of them. http_pool_expire_locked showed as 0.52% of cycles, but that is inclusive of the transport_close/free a real expiry still performs, so skipping the walk recovers much less than the profile implied. I've kept it because it is strictly less work under a global mutex — which should matter with more upstreams than this two-backend benchmark has — but it is not a performance claim, and both the commit message and CHANGELOG say so.

Worth flagging how that null result nearly got misread: the first A/B run showed +26%, because the benchmark backends had died and the LB was returning errors quickly instead of proxying. The harness now refuses to report a number unless the LB answers 200 first.

Correctness is the real content of this change. The watermark is invalidated by five different paths; all are handled, and the subtle one is client_pool_configure — shortening the idle window leaves a deadline further out than the new setting allows, which would delay every eviction. That forces a sweep.

Change 4 — resolve the backend only when about to dial

getaddrinfo ran unconditionally at the top of the request path, above the pool lookup (aether_http.c:1710 vs the pool take at :1737), so every request resolved the backend host and discarded the answer on a pooled hit. Only http_dial uses the address, and the pool key is built from dial_host/dial_port rather than the resolved result, so nothing in between needed it.

Resolution now goes through resolve_dial_addr, which resolves at most once per request behind a once-flag, at all four dial sites: the initial dial plus three pooled-connection retry paths.

+0.56% (47,887 → 48,156 rps, new ahead in two rounds and tied in the third). Modest here because this benchmark's backends are numeric IPs, which getaddrinfo short-circuits without touching files or the network — against named upstreams it removes real work.

A bug I introduced and caught, worth recording: the first version of this change routed only one of the four dial sites through the resolver. The other three are pooled-connection retries — precisely the paths where the first resolve was skipped — so they passed an uninitialised sockaddr_in to connect(). Undefined behaviour. Every client test still passed; http_reverse_proxy T10 caught it, as a 504 that came back 000. The commit was amended rather than fixed-forward, and I added a mechanical check that every http_dial(req, &serv_addr, …) has a resolve_dial_addr above it.

One deliberate behaviour change: a request hitting a live pooled connection now succeeds even if the host has since stopped resolving, where before it failed with "could not resolve host". An open connection does not need DNS, and that is what nginx does. A request that has to dial fails exactly as before.

Correction to #1719

Two numbers in that issue were wrong, both mine:

• "27% of cycles in malloc/free" — overstated. The 27% is libc total; the named allocators are malloc 1.52% + cfree 1.39% + __strdup 0.51% ≈ 3.4%. The rest is unresolved syscall stubs. There is no allocation hot spot to attack, and I no longer think an arena-per-request is the next move.
• "setsockopt 2.0/request" — undercounted. It was ~10/request; the earlier profile was taken under conditions that undersampled it.

The corrected picture: our own code is ~10% of cycles, the largest single function 1.84%, and the top twelve sum to ~6%. There is no hot spot in Aether's C. The gap was kernel time from syscalls we didn't need to make — which is why this PR is syscall elimination and not code tuning.

Constraint: no new test failures

• HTTP/proxy/TLS integration sweep: 41 pass, 2 fail
• http_client_forward_proxy — a leaked test binary squatting port 18120; passes once killed. Environmental.
• http_server_h2 — fails identically on the baseline. Verified by reverting both files, rebuilding, re-running. Pre-existing h2 framing bug, not this PR.
• Full CI: 26/26 green, including all three native Windows legs and both macOS Clang legs.

No question for you after all — I withdrew the one that was here

An earlier revision of this description asked whether your 2 ms grace poll in conn_next_request_imminent was still earning its place, on the grounds that poll is now the largest single syscall cost (36.6% of syscall time, ahead of sendto at 22.4% and recvfrom at 17.9%).

That question was misdirected, and I've removed it rather than leave you to answer it. Splitting poll by caller over 20,000 requests:

poll site	calls	share
transport_is_live — pooled-connection liveness probe, timeout 0	39,713	99.2%
conn_next_request_imminent — your parking grace, timeout 2	265	0.66%
accept loop, timeout 1000	34	0.08%
connect wait, POLLOUT 30000	24	0.06%

Your grace poll fires roughly once per 75 requests, not once per request — http_pool_has_spare_worker() gates it to 0 ms whenever workers are busy, which under this benchmark is nearly always. Even deleting it outright caps out around a third of one percent of syscall time. It is not where the cost is, and your measurements stand.

I got there from the #1719 table, which reported "poll 2.0/request" without separating the callers; I attributed it to the parking path because that is the poll carrying a comment. The ~2 per request is transport_is_live, on the client side — my code, unrelated to parking.

So: nothing to answer here. The review ask is just changes 1–3 above, and change 2 in particular since it edits your comment.

What I'm taking next (no input needed)

Recording it so it isn't rediscovered: the upstream reuse path has the remaining per-request work, and both items are mine.

1. transport_is_live polls every pooled connection before use — ~2 syscalls/request, 99% of all polls. Defensible as written (a stray byte would be read as the head of the next response), but nginx doesn't pay it: it lets the read fail and retries. Removing it changes an error-handling contract, so it needs its own PR and its own argument.
2. getaddrinfo runs before the pool lookup — aether_http.c:1710 resolves the backend host unconditionally, and the pool take is at :1737. On a pooled hit the resolved address is discarded. It profiled at 0.44% and takes a lock. This one looks like a straightforward ordering bug.

🤖 Generated with Claude Code