Repository navigation
Conversation
…NG_ADAPT) The resident ring is a single global today (--kv-resident), and the KV pool is sized by it (qsa_state_bytes -> kv_pool_bytes sizes the pool by slots). On a 24 GB card a ring big enough for depth costs shallow runs their configuration: measured at 1K the ring's growth to 49,152 cells cost 40 % of the MTP path (207/207 -> 149-156/215 draft acceptance), while it was worth +11.7 % @32k and +21.6 % @128k. This sizes the ring from the cells the run needs instead: clamp(prompt + max_new rounded to a page, 32,768, --kv-resident). The floor is the engine's historical default, so a shallow run keeps exactly the ring it has today, and only a deeper one grows into the cap. Opt-in (STRATA_KV_RING_ADAPT), default behavior unchanged; the hook runs before the session arena is sized and the graphs are captured. Measured on gfx1100 (7900 XTX, ROCm 10.2, IQ3_S, 256 generated, interleaved pairs): 1K -0.2 %, 32K +12.9 %, 128K +6.4 %, prefill and TTFT unchanged. With a lower floor (20,480) the same patch measured -7.3 % at 1K, which is why the floor is the default ring. Scope: the generate path; serve needs the same hook at its session-creation site.
rahul05ranjan
left a comment
There was a problem hiding this comment.
The sizing logic reads clean: round cells_needed up to the page, floor at the historical 32,768 ring (max with qsa_kv_resident_min(), which is 20,480 today), cap at the configured --kv-resident, and the placement before session_bytes/cudaMalloc in generate.cpp matches the comment about the arena sizing. The measured numbers in the floor comment are a good touch.
Two doc/behavior nits:
- The header comment says "Returns cap unchanged when cap <= 0" but the code returns 0 for any cap <= 0, including negatives (qsa_kv_ring_adapt in src/core/layer.cpp). The caller guards with o.kv_resident > 0 so there's no runtime effect — either the comment should say "returns 0" or the code should
return cap. - The getenv() presence check means an empty value, "0", or "false" all enable the adaptive path. Fine as a pure on-switch, but a "1" check would let scripts disable it without unsetting the variable.
|
Scope correction after reading the serve path: this PR is Serve keeps one main session (created before the request loop) plus parked snapshots in the conversation cache ( So the follow-up is either a startup heuristic for serve or per-conversation pools, not "the same hook at the session-creation site" as I first wrote. I will keep this PR scoped to the one-shot path and will not claim serve coverage. |
|
Re-test on the base this PR targets, v0.2.0-beta, and it is not a confirmation: the depth gain holds at 128K, the 32K tier reverses, and the 0.1.45 numbers in the description do not transfer. Same binary both arms (built from
The 1K column is the acceptance state again (control cells carry 197 and 207 accepted drafts; the adaptive arm sat at 207 throughout), so I would not read a gain from it — what matters is that 1K does not regress, which is the floor doing its job. But the 32K reversal is real enough to state plainly: on 0.2.0 the 32K point no longer benefits from a ring just past the prompt boundary (0.2.0's control is already ~82 tok/s there, against ~74 on 0.1.45), and one depth tier regressing fails the rule I pre-registered for this change. So: still opt-in and default-off, still a 128K win, but not adoptable as written on 0.2.0. I am keeping this as a draft-grade patch rather than asking for a merge until the 32K tier is understood — likely a 0.2.0 change in the pool/ring path, which I have not diagnosed. Happy to run whatever measurement would help, or to withdraw it. |
|
Follow-up to the 0.2.0 re-test: the 32K reversal is mostly the drafter, with a small main-path cost — it is not the ring planning (that code is unchanged between 0.1.45 and 0.2.0; the 32K on
So without speculation the bigger ring costs −1.7 % at 32K, against −9.1 % with MTP on the same tier. Most of the reversal is the draft path (the same mechanism as 1K), and a small real main-path cost remains — which also fits the base being faster at 32K on 0.2.0 (control ~82 vs ~74 tok/s on 0.1.45): the boundary gain shrank while the drafter-side cost did not. Practical read for this patch: it needs a drafter-aware cap, not a depth-only one — do not raise the ring past what the drafter tolerates at the tier being served. I am not proposing that here; I am leaving the patch as the depth-only experiment and will not ask for a merge. If a drafter-aware version is wanted, it needs its own pre-registered cells against this table. |
|
Thanks for the pull request. We're resetting the tracker for 0.2.0-beta. There are more than 1,300 open issues and pull requests, too many for us to review properly, so we're closing everything that isn't being worked on right now. Closed here does not mean rejected. To reopen it takes about two minutes:
The rules are in CONTRIBUTING.md. |
|
Closed by the tracker reset rather than a review, so noting where it stands and leaving the call with you. The branch has moved: New numbers, all on 0.2.0-beta with the same binary both arms (interleaved pairs, 256 generated):
Against the live 0.1.45 binary: decode +7.5 % @32k, +9.0 % @64k, +8.6 % @128k for prefill -0.9/-1.4/-2.4 % and TTFT +0.7/+1.3/+2.5 % (5 pairs at 128K). The PR body in the branch now matches; the 1K column is a same-ring noise comparison and is stated as such, not claimed. If the maintainers want it, a reopen is all that is needed and I will refresh the body against the current main. If the intent is to keep the tree clean during the reset, leaving it closed is fine with me — the measurements are in the thread and the branch. |
|
Rebased onto
/reopen |
Problem and result
The resident K/V ring is one global value (
--kv-resident), and because the KV pool is sized by it (the pool is the ring), a ring chosen for depth is paid by every run. A flat raise is a measured loss at shallow depth — on 0.2.0, ring 32,768 → 49,152 at 1K costs −40 % of the MTP path (acceptance 207/207 → 149–156/215) and at 32K −9.1 % with MTP against −1.7 % without: the draft path pays for a ring it cannot use, not attention.This PR sizes the ring from the run's depth, and only grows it when the ring can cover the whole context:
Opt-in (
STRATA_KV_RING_ADAPT), default behaviour unchanged. A shallow run keeps exactly today's ring; a deep one grows into the cap.Related discussion
Approach
qsa_kv_ring_adapt(g, cells_needed, cap)insrc/core/layer.cpp(+ declaration inlayer.hpp): rounds the needed cells up to a page, floors atmax(qsa_kv_resident_min(), 32768), and returns the floor unlessneeded >= cap.src/program/generate.cppruns before the session arena is sized and the graphs are captured, becauseqsa_state_bytes→kv_pool_bytessizes the pool by slots.Validation
Passed (0.2.0-beta base, same binary both arms, interleaved pairs, 256 generated tokens):
KV ring adaptive: 49152 -> 32768).Failed: none observed.
Not run:
--servepath: serve keeps one main session plus parked snapshots, so there is no per-request session to size a ring for; per-request sizing there needs per-conversation KV pools, noted as a follow-up, not this patch.Limits
generate-path only.Measurement notes
One process a cell, one-shot greedy, 256 generated tokens; the intervals are the MTP acceptance state (cells moved ±8 %), so only paired medians are used. Raw rows: 32K control 74.0/82.4/…, 128K control 70.7/64.5/…, cap arm 81.5/84.2/… — the full tables are in the campaign records; I can attach a sanitized dump if useful.