perf(router): stream the data path instead of buffering it - #199
Draft
Rockdu wants to merge 7 commits into
Draft
Conversation
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.
What
Streams the router's data path. Three commits: submit the parser task off the event loop, strip hop-by-hop headers, and relay response bodies chunk-through instead of buffering.
Draft: stacked on
feat/rollout-request-timing(the instrumentation these commits modify); rebase and mark ready once that branch lands.Why
The router buffered every response twice (httpx
aread()+ the transport's bytearray) and serialised the two hops — nothing reached the client until the worker finished sending. At H3's 1.4 GiB responses that made router egress the largest cost in an admitted request (47.7%, above the denoise's 24.5%) and RSS peaked at 51 GiB.Validation
Isolated harness (stub upstream -> this router -> N clients, 1384 MiB payloads), buffered vs streamed:
In the 17-GPU H3 run (with the sibling PRs): per-request egress median 61.6 s -> 4.5 s, rollout-phase GPU 35% -> 82%,
train_wait1010-1140 s -> 486-499 s.Measured constraint — do not merge for raw float payloads alone. An ablation on the same run with
--rollout-video-dtype keep(1384 MiB bodies, streaming on) REGRESSED: egress median 95.1 s vs 61.6 s buffered,train_wait1260 s vs the 1010-1140 s buffered baseline. Buffering acted as a large elastic buffer between the engine-side and client-side rates; streaming replaces that with end-to-end min-rate flow, and at ~90% utilisation of the path the queueing dominates. The harness's +22%..2x was measured with an idle client loop and does not transfer. Streaming's wins at H3 payloads are memory (router RSS 34 GiB -> 144 MiB) and headroom; the throughput win requires the payload cut (#198's uint8) to land with it.Known issue surfaced by the harness, pre-existing and not introduced here: uvicorn's 5 s keep-alive close races a client reusing a pooled connection (probability grows with N; disappears with keep-alive off). Worth a follow-up on its own.
Checklist
pre-commit run --all-filespasses — run on touched filespytest -xis green — tests/fast on the full stack, 215 passed--helpstill parses — no flag changes