Repository navigation
fix(protocol): stop the /ws request pipeline from waking its connection on every request - #249
Merged
Merged
Conversation
…ry request The release-checkpoint performance gate fails on main: cheap_query serial p50 is 1.110x the approved baseline e732cd5 on the 32-vCPU runner (115 -> 126 us). Concurrent latency and throughput did not move. Bisected to #214 (per-connection request pipelining), made worse by #222. Cause: the pipeline runs requests in a `FuturesUnordered`. Its `poll_next` wakes its own task and returns `Pending` after any poll that polled every future in the set. With one request in flight, which is every serial client, that happens on each request's first poll. The connection task wakes itself, tokio schedules that as a yield, and a yield wakes an idle worker to take the task. Each request therefore costs one more context switch (4.0 -> 5.0 per query) and a hand-off of the connection task to another core. #222 added no work per query but made the hand-off dearer: user cycles per query rose 15% at the same instruction count. Fix: keep one never-woken future in the set. After the pipeline's first poll it is never polled again, so no poll reaches every future and the forced yield no longer fires. The yield after futures that wake themselves still applies. Pipelining, ordering, deadlines, cancellation and the credential fence are unchanged. Nothing is added per request: no lock, atomic or allocation. Each connection gets one more set node. Bisect (cheap_query, `?edge(1, Y)`, serial). Server counters per query from perf stat. Ratios are against e732cd5, from an interleaved probe with one pinned server per commit, 40 rounds: | commit | PR | user instr/query | ctx switches/query | p50 ratio | |---|---|---|---|---| | e732cd5 | baseline | 1.282M | 4.0 | 1.000 | | d75ce6e | #208 atomic programs | 1.279M | 4.0 | 0.993 | | 4578fe6 | #215 notification stream | 1.271M | 4.0 | 0.991 | | 71a4d24 | #216 online backup | 1.275M | 4.0 | 1.016 | | 2f621c5 | #200 first-update latency | 1.272M | 4.0 | 1.009 | | e96df5c | #207 | 1.279M | 4.0 | 0.998 | | 4dd8fe9 | #209 shard index | 1.272M | 4.0 | 0.987 | | 277c7f0 | #206 | 1.274M | 4.0 | 1.008 | | 3554c98 | #214 pipelining, deadlines | 1.296M | 5.0 | 1.020 | | 3492a59 | #213 | 1.294M | 5.0 | 1.030 | | 2ba9a5a | #212 provenance | 1.297M | 5.0 | 1.029 | | 9ed61b4 | #204 key expiry | 1.297M | 5.0 | 1.027 | | be35ee3 | #221 | 1.297M | 5.0 | 1.013 | | c98a590 | #222 bounded streaming | 1.298M | 5.0 | 1.047 | | a6fd628 | #219 wildcard fix | 1.297M | 5.0 | 1.061 | | b7f7ad6 | #220 WAL durability | 1.300M | 5.0 | 1.056 | | de54807 | #223 shared standing eval | 1.299M | 5.0 | 1.071 | | 55ec34d | #225 checkpoint-1 fixes | 1.300M | 5.0 | 1.045 | | 23712b8 | main | 1.300M | 5.0 | 1.064 | | this change | | 1.300M | 4.0 | 1.033 | Confirming A/Bs with the gate's own fixture (fresh pinned servers each round, 40 rounds, ratio [95% interval]): - #214 vs its parent 277c7f0: 1.039 [1.027..1.049] - #222 vs its parent be35ee3: 1.025 [1.006..1.044] - A/A 3554c98: 1.010 [1.002..1.025] - e732cd5 -> main: 1.048 [1.028..1.066] - main -> this change: 0.978 [0.958..0.989] - e732cd5 -> this change: 1.021 [1.009..1.044], PASS (budget 1.05) User cycles per query (perf stat, same instruction count): e732cd5 958K, be35ee3 1053K, c98a590 1206K, this change 1006K. Full gate, all fixtures, 20 rounds, servers pinned to CPUs 2-15: - main -> this change: no metric worse. Every required metric is PASS, or INCONCLUSIVE with an interval that includes 1.0. cheap_query p50 0.979 [0.953..1.007]. - e732cd5 -> this change: cheap_query p50 1.029 [1.019..1.072], INCONCLUSIVE on a busy host (load average up to 22); bound_query p50 1.022, PASS. The insert and delta metrics improved since the baseline. The shared host is about 4x slower per query than the CI runner, so the same hand-off is a smaller share of each query here than there. The remaining ~1.7% of user instructions per query since #214 is the pipeline's own per-request bookkeeping: the boxed request future (one 4-8 KiB allocation), its set node, the request control and the in-flight maps.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Restore millisecond cheap-query latency on the /ws protocol without giving up anything the request pipeline delivers. The release-checkpoint performance gate (perf-gate cheap_query, serial ?edge(1, Y)) failed on main with p50 1.110x the approved baseline e732cd5 (115 -> 126 us on the 32-vCPU CI runner). The regression bisects to #214 (per-connection request pipelining, deadlines, cancellation): the pipeline's FuturesUnordered forces a yield (self-wake) after polling every future it holds, which with one request in flight happens on every request, so tokio wakes an idle worker and hands the connection task to another core on every query (one extra context switch per query); #222 made that hand-off costlier. The fix must stop waking an idle worker per request in the #214 pipeline, while keeping pipelining, in-order replies, ordering barriers, deadlines, cancellation and the credential fence unchanged, and without adding any lock, atomic or allocation per request (no new locks on hot paths). Other optimisations are out of scope. The PR must include the bisect table (per-commit instructions and context switches per query, p50 ratio vs e732cd5 for every runtime commit since e732cd5) and the before/after measurements (A/B ratios with intervals: #214 vs parent, #222 vs parent, e732cd5 -> main, main -> fix, e732cd5 -> fix, plus the full-gate main -> fix result showing no metric worse).
What Changed
RequestPipeline::new(src/protocol/rest/handlers/ws/pipeline.rs) now puts one future that is never woken (std::future::pending()) into therunningFuturesUnordered. It is polled once, on the pipeline's first poll, and after that no poll ever reaches every future in the set. SoFuturesUnorderedno longer wakes its own task and returnsPendingon each request's first poll. With one request in flight, that self-wake used to make tokio wake an idle worker and move the connection task to another core on every query. Futures that wake themselves still get the yield.shutdownnow drains whilerunning.len() > 1instead of until the set is empty, andnext_replyhas a comment saying itsNonearm can no longer be reached. Pipelining, in-order replies, ordering barriers, deadlines, cancellation and the credential fence are unchanged. Nothing is added per request: no lock, atomic or allocation. Each connection gets one extra set node.a_running_request_wakes_the_connection_only_when_it_finishes. It uses a counting waker to check that starting a request causes no wakes and that a finished request wakes the connection exactly once.Bisect
cheap_query,
?edge(1, Y), serial. Server counters per query come fromperf stat. p50 ratios are against e732cd5, from an interleaved probe with one pinned server per commit, 40 rounds.Before/after measurements
A/B runs with the gate's own fixture: fresh pinned servers each round, 40 rounds, ratio [95% interval].
User cycles per query (
perf stat, same instruction count): e732cd5 958K, be35ee3 1053K, c98a590 1206K, this change 1006K.Full gate, all fixtures, 20 rounds, servers pinned to CPUs 2-15:
These runs were on a shared host that is about 4x slower per query than the CI runner, so the same hand-off is a smaller share of each query here than on CI.
Risk Assessment
✅ Low: The change is one file and narrowly scoped. It adds a single never-woken sentinel future to each connection's FuturesUnordered. I checked it against futures-util 0.3.32 poll_next: with the sentinel in the set,
polled == lenis no longer reached once the first poll is done. The None branch, shutdown, which now loops while len > 1, and every other path keep the pipelining, ordering, deadline, cancellation and credential-fence semantics they had before. Nothing is added per request (no lock, atomic or allocation); the only new cost is one task node per connection. The new test drives the public poll path and would fail without the fix.Testing
I built release servers for main (c649d12) and the fix, and the gate script built e732cd5. I drove all three over the real /ws protocol in three ways. First, a raw-socket WebSocket client ran 6 interleaved rounds of 3000 serial
?edge(1, Y)queries per side and read server context switches from /proc. The count was a steady 5.0 per query on main, 4.0 with the fix, and 4.0 on e732cd5. Second, I ran the repo's own perf-gate on cheap_query only, with servers pinned to CPUs 2-15. Main → fix gave p50 0.975 [0.936..1.027], PASS, plus an earlier noisier run at 1.002. e732cd5 → fix gave p50 1.019 [0.987..1.052], INCONCLUSIVE because the shared host was at load average 20-30; that matches the author's own 1.021-1.029. Third, a burst of 200 pipelined requests came back in order with correct ids and row counts. I also ran the existing integration tests that exercise these features over /ws: ws_pipeline_tests (all 9), ws_cancel_tests (deadlines and cancellation), credential_revocation_tests (fence), plus the ws pipeline/outbound/access unit tests. All pass. I confirmed the new wake-count regression test fails with the fix's runtime lines reverted ("starting a request woke the connection", left 1 vs right 0) and passes with them, then restored the file. That check is a unit test, so its scenario is recorded as untested (not live). I did not re-run the full gate across all fixtures or the per-commit bisect; those figures are the author's own, recorded in the commit message. This change has no UI. All build outputs (target/, 4.8G) were removed and the worktree is clean.cargo test --release --test ws_pipeline_tests: kg_switch_orders_pipelined_writes, session_changes_are_ordered_with_queries, pipelined_subscribe_and_unsubscribe_keep_their_ordercargo test --release --test ws_cancel_tests: 6/6, incl. a_running_query_stops_at_its_deadline, cancel_stops_a_running_query_promptly, a_write_whose_deadline_passes_in_the_queue_never_runscargo test --release --test credential_revocation_tests: 9 pass, incl. revoking_your_own_credential_withholds_the_reply, nothing_committed_after_the_fence_reaches_the_revoked_sessionEvidence: Interleaved A/B main vs fix over /ws: ctx switches/query and latency
main: ctx_switches_per_query 5.001-5.008 (6 rounds); fix: 4.001-4.007 (6 rounds)Evidence: Baseline e732cd5 ctx switches/query
4.076, 4.11Evidence: perf-gate cheap_query main -> fix (30 rounds)
cheap_query.latency_us.p50 0.499 ms -> 0.482 ms, ratio 0.975 [0.936..1.027] PASS; queries_per_sec PASS; concurrent p50 PASSEvidence: perf-gate cheap_query main -> fix (20 rounds, busier host)
Evidence: perf-gate cheap_query e732cd5 -> fix (30 rounds, load avg ~25-30)
cheap_query.latency_us.p50 ratio 1.019 [0.987..1.052] INCONCLUSIVE; p99 PASS; qps PASSEvidence: Pipelined burst in-order check
main: 200 pipelined, 0 out of order/wrong; fix: 200 pipelined, 0 out of order/wrongEvidence: Raw /ws probe client used for the live checks
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
cargo test --release --test ws_pipeline_tests: kg_switch_orders_pipelined_writes, session_changes_are_ordered_with_queries, pipelined_subscribe_and_unsubscribe_keep_their_ordercargo test --release --test ws_cancel_tests: 6/6, incl. a_running_query_stops_at_its_deadline, cancel_stops_a_running_query_promptly, a_write_whose_deadline_passes_in_the_queue_never_runscargo test --release --test credential_revocation_tests: 9 pass, incl. revoking_your_own_credential_withholds_the_reply, nothing_committed_after_the_fence_reaches_the_revoked_sessionpython3 wsprobe.py <server-main|server-fix> 2-15 3000x6 interleaved rounds (raw /ws client: serial ?edge(1, Y), server ctx switches/query from /proc)python3 wsprobe.py <e732cd52 server> 2-15 3000x2 (baseline ctx switches/query)python3 wsprobe.py <server> 2-15 0 --pipelineon main and fix (200 pipelined requests: order, id echo, row counts)scripts/perf-gate.sh --baseline-rev c649d122 --candidate-bin server-fix --fixtures cheap_query --rounds 20/30 --server-cpus 2-15(two runs)scripts/perf-gate.sh --candidate-bin server-fix --fixtures cheap_query --rounds 30 --server-cpus 2-15(approved baseline e732cd52)cargo test --release --test ws_pipeline_tests(9/9)cargo test --release --test ws_cancel_tests(6/6: cancel, deadlines)cargo test --release --test credential_revocation_testsfiltered to revocation/fence (9 pass)cargo test --release --lib ws::pipeline/cancel/deadline/revocation/credential unit tests incl. newa_running_request_wakes_the_connection_only_when_it_finishesRegression check: fix's runtime lines in pipeline.rs reverted temporarily → new test FAILS (left 1, right 0); file restored, git status clean✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.