perf(core): reuse a persistent weight-fill pool and tune staging geometry - #763
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a469588457
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a469588 to
58152a0
Compare
|
Re-checking the measurements surfaced a second problem, in the GH200 row of the body. Those figures difference medians of two independent arms across repetitions; only same-repetition pairs are comparable here, because the phases around the load drift with machine state. Read as pairs, that run yields one usable repetition — the baseline arm of the first ran against a cold page cache, since the job had no untimed warm-up, and the second was hit by unrelated load on the node. The surviving pair is 4729 → 4315 ms in phase and 9608 → 9150 ms to ready. The sm_89 row stands: five clean pairs, load phase -8.8% to -11.0%, ready flat. The in-phase effect is real and the two hosts agree on its size; the GH200 ready figure is one repetition, a direction rather than a claim. The twelve-versus-eight comparison is unpaired too, though its ranges do not overlap. A re-run with a warm-up and five repetitions is queued to replace the GH200 row. |
|
The persistent-pool follow-up is folded into this PR, because the grid below shows the geometry change alone is dominated by the pool alone. Two GH200s, Qwen3-32B, tensor parallel size two, 144 cores. Ten arms, five repetitions, arms interleaved inside each repetition, one untimed warm-up discarded. CPU is sampled every 100 ms and read at the weights-ready mark, so it covers the fill rather than the whole startup. Spread is (max-min)/median of the phase across repetitions.
Row one is main; row eight is this PR. The constants alone are not worth shipping. Spawning at 8 workers and 32 MiB, which is what this PR contained before, reaches 8272 ms to ready at 25.01 s of fill CPU; pooling at main's own 4 and 64 MiB reaches 8262 ms at 20.35 s — better on both axes with no constant touched. Per second of startup saved that arm costs 11.9 CPU-seconds, against 4.3 to 4.6 for every arm that includes the pool. CPU is bought by the worker count, not the chunk size. At fixed chunk, four workers to eight costs 14.3% and 17.5% more fill CPU on the pooled arms; at fixed workers, 64 MiB to 32 MiB costs 1.1% and 4.0%. The two were changed together in one step before, which is why the earlier numbers attributed the cost to the wrong variable. The price of this PR is 6.68 additional CPU-seconds once per process start for 1547 ms off HTTP-ready, on a host where the fill already occupies about four cores and now occupies about nine. Cheaper points exist on the frontier if that is the wrong trade: pool at 8 workers and 64 MiB is 7651 ms for 5.76 CPU-seconds and leaves pinned memory at 128 MiB. On a single sm_89 GPU with Qwen3-4B the same comparison is flat to ready across five paired repetitions. That host reaches its H2D roofline of 23.3 GiB/s while four fill threads already sustain 24.6 GiB/s, so a fill-side saving has nowhere to surface. Validation. Open question. |
|
Rebased onto main after #752 merged and re-measured there, so the figures in my earlier comment. What ranOne sm_89 GPU: Two GH200s: Correctness
Measurements
Milliseconds for ready and load, seconds for cpu. Repetition 5 sits apart from the other four in both the branch arm (load 1720 against about 850) and main (ready 8219 against about 7600). Same-repetition pairs, main to this branch: ready -14.4%, -7.3%, -4.4%, -8.0%, +2.2%; load -23.5%, -24.5%, -25.3%, -18.4%, +45.1%. Medians across arms: ready -8.7%, load -24.3%, cpu +34.3%. On the pre-#752 base the same change measured -17.4% to ready, and the CPU delta was of the same size, which puts the cost at 7.7 CPU-seconds per second of startup saved here against 4.3 there. Correction to the grid comment aboveThe Recomputed from CPU read at ready, which is a direct
The statements that column supported come out the same: four workers to eight costs 12.6% and 14.8% more CPU on the pooled arms while 64 MiB to 32 MiB costs 0.3% and 2.2%; spawning at 8 and 32 MiB reaches 8272 ms at 28.84 s against pooling at main's constants reaching 8262 ms at 24.25 s; per second of startup saved that arm costs 12.1 CPU-seconds against 4.3 to 4.9 for the pooled arms. A constant common to every arm cancels in the differences, which is why the percentages shift and the orderings do not. What did not runNo TP4 or TP8 of anything — that is what the No other model line was exercised. That is bounded rather than assumed: Two figures are not comparable across the two campaigns even though both are labelled a load phase: the earlier one came from a tensor-parallel timeline marker covering both ranks and a barrier, this one from the model's own per-rank line. Open question
|
fb18907 to
7e69c6f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e69c6f05e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
7e69c6f to
0552a42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0552a424b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Feathbow <feathbow@gmail.com>
0552a42 to
fee39e9
Compare
xiaguan
left a comment
There was a problem hiding this comment.
Thanks for the careful implementation and measurement work. I rechecked the current head locally and against a clean merge onto current main: the focused release test for the parallel strided-gather path, strict openinfer-core Clippy, formatting/diff checks, and the release Qwen3 server check all pass. The MaybeUninit<u8> boundary, stager-owned Rayon pool, scoped task joins, and event-gated pinned-buffer reuse look sound. I do not see a remaining implementation blocker.
The measured tradeoff is also real, but the PR's public narrative is stale and currently contradicts both the code and the later measurements. Please update these two surfaces before merge:
-
Rewrite the PR description to describe the actual change and current evidence. It still says “This changes only those two constants; no logic moves,” uses the old two-host/three-repetition table, and says the TP2 golden gate has not run. The PR now introduces a persistent fill pool as well as the 32 MiB / 8-worker geometry, and TP2 validation has been completed. On the post-#752 base, the same-repetition paired medians for main → this branch are GPU-model load
-259 ms / -23.5%, HTTP-ready-541 ms / -7.3%, and CPU-to-ready+4.94 s / +32.8%. Please present this explicitly as a one-time startup CPU-for-latency tradeoff rather than retaining the obsolete constant-only description. -
Correct the TP concurrency model and paired statistics in
docs/subsystems/frontend/startup-time.md. Line 43 currently says eight workers per rank become 32/64 concurrent fill workers at TP4/TP8 and recommends dividing a host budget byworld_size. That does not match the Qwen control flow:openinfer-qwen3/src/executor.rs:1368-1383loads TP ranks sequentially, and each rank's stager/private pool is gone before the next rank starts. The exact-head TP4/TP8 experiment confirms the consequence: reducing the active team from 8 to 4/2 workers delayed HTTP readiness by267.7 ms / 4.33%and1118.7 ms / 9.99%, respectively. Please replace the concurrency claim with that verified boundary. Line 45 should likewise report paired deltas (-259 msload,-541 msready,+4.94 sCPU) rather than differences between independent arm medians (1113→843,7682→7010,15.06→20.22).
After the PR body and startup document reflect the implementation and measured data, I do not expect further code changes.
…er sweeps TP ranks load one at a time — the constructor finishes one `Qwen3Model` before starting the next and drops that rank's loader with its fill pool — so eight workers per rank is never 32 or 64 concurrent fillers and there is no host budget to divide by world size; a TP4/TP8 arm confirms the split only shrinks the one team that fills. Other claims in the same section did not hold either. The four spawned workers were justified by single-thread fill falling behind H2D, not by the event-sync amortization that the pegainfer-project#742 sweep dropped. Pooling alone is a real trade against main, so only the geometry-only arm is dominated. GLM5.2's concurrent fillers exist only under `--glm52-weight-staging`, count per local rank rather than per global EP rank, and number 16 in the single-host EP4 case. The post-pegainfer-project#752 load delta is the model's own line, not the sweep's two-rank marker, and is quoted as a paired median. The column-shard ceiling is enforced in `prepare_cols` and binds the supported Qwen3 checkpoints. The `FILL_THREADS` comment now states what the constant bounds instead of asserting a measurement that would drift. Signed-off-by: Feathbow <feathbow@gmail.com>
|
Thanks for the careful review. Correct the startup document to reflect sequential Qwen3 TP rank loading, the TP4/TP8 worker-budget results, and paired rather than independent-arm statistics. |
|
The current head combines a persistent, stager-owned Rayon fill pool with two 32 MiB buffers and up to eight workers per rank. On the post-#752 base, five paired repetitions give:
This is explicitly a one-time CPU-for-startup-latency tradeoff. Qwen3 TP ranks load sequentially, so only one eight-worker fill team is active. Reducing it to four workers at TP4 or two at TP8 delayed readiness by 267.7 ms and 1118.7 ms while saving only 0.31 and 0.78 CPU-s. The implementation therefore remains at eight workers per rank. TP2 |
xiaguan
left a comment
There was a problem hiding this comment.
Rechecked the final head. The PR description now separates the current scope and validation from the superseded initial proposal, and the startup document accurately records the sequential Qwen TP load path, paired performance/CPU tradeoff, TP4/TP8 worker-budget result, and GLM5.2 boundary. The implementation review and local release validation remain clean, all 15 checks pass, and I found no remaining blocker. Approving.
Final scope and validation
The current head combines a persistent, stager-owned Rayon fill pool with 32 MiB staging buffers and eight workers per rank. On the post-#752 base, paired medians are -259 ms GPU-model load, -541 ms HTTP-ready, and +4.94 s CPU-to-ready. The TP2
hf_golden_gatehas passed.Initial proposal (superseded by later measurements)
The pinned staging path shipped with 64 MiB buffers filled by four threads. Both numbers were picked to amortize the per-chunk event sync rather than measured against the fill rate itself, and neither had been swept as a pair. This changes only those two constants; no logic moves.
Measured on two hosts, warm page cache, arms interleaved inside each repetition so a drift in machine state cannot masquerade as an arm effect:
Medians of three repetitions on the first host, five on the second.
The two hosts disagree at HTTP-ready for a reason worth stating: the sm_89 host reaches its H2D roofline of 23.3 GiB/s while four fill threads already sustain 24.6 GiB/s, so the pipeline there is transfer-bound and an in-phase saving cannot surface end to end. The GH200 host is not, so it does. Both hosts agree the load phase itself gets faster, and neither regresses at ready — the sm_89 spread across repetitions of a single arm is 17 ms, wider than the 10 ms difference between arms.
Twelve threads was measured on the GH200 host and is slower than eight (4406 ms against 3937 ms) while costing a third more CPU, so the thread count stays a fixed width rather than a fraction of the core count. It is still capped by
available_parallelismso a small host does not oversubscribe, and logs the effective width when that cap engages.Two consequences beyond throughput. Pinned memory halves to 64 MiB across both buffers. And
upload_colsrejects a column shard whose row width exceeds one staging buffer, so halving the buffer halves that ceiling — from 33.5 M to 16.7 M columns per shard, which no published checkpoint approaches.Validation on the sm_89 host:
openinfer-coreunit tests pass, and a greedy completion is byte-identical before and after the change, with a different prompt producing different output as a control.The TP=2 pass of
hf_golden_gatehas not been run on this branch; it needs two visible CUDA devices and self-skips below that. It is the gate for the column-gathering path, whose chunk count this change alters.Type of Change
Checklist
docs/conventions/coding-style.md).CLAUDE.md).