Skip to content

[Engine] kv: size the resident ring from the run's depth (opt-in, STRATA_KV_RING_ADAPT) - #3168

Closed
xyzzing wants to merge 1 commit into
Niko1221:mainfrom
xyzzing:v4-kv-ring-adapt
Closed

xyzzing wants to merge 1 commit into
Niko1221:mainfrom
xyzzing:v4-kv-ring-adapt

Conversation

@xyzzing

@xyzzing xyzzing commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

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:

needed = prompt + max_new + one page
ring   = (needed >= cap) ? cap : 32768     // 32768 is the engine's historical default
cap    = --kv-resident

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) in src/core/layer.cpp (+ declaration in layer.hpp): rounds the needed cells up to a page, floors at max(qsa_kv_resident_min(), 32768), and returns the floor unless needed >= cap.
  • The hook in src/program/generate.cpp runs before the session arena is sized and the graphs are captured, because qsa_state_bytes → kv_pool_bytes sizes the pool by slots.
  • The floor is the engine's historical default ring on purpose: with the allocator minimum (20,480) an earlier version measured −7.3 % at 1K.

Validation

Passed (0.2.0-beta base, same binary both arms, interleaved pairs, 256 generated tokens):

  • 32K: control 82.2 → 82.3 tok/s (+0.1 %) — the depth-only rule had lost 9.1 % here; the ring stays at 32,768, which the arm's own log line confirms (KV ring adaptive: 49152 -> 32768).
  • 128K: 70.7 → 81.5 tok/s (+15.3 %), prefill 2,391/2,375 and TTFT 55.1/55.4 s unchanged.
  • Against the live v0.1.45 binary: decode +7.5 % @32k, +9.0 % @64k, +8.6 % @128k (5 pairs at 128K, tight: prefill 2,445–2,459 vs 2,382–2,400; TTFT 53.6–53.9 vs 54.9–55.3 s), prefill −0.9/−1.4/−2.4 %, TTFT +0.7/+1.3/+2.5 %. This is a trade the maintainers may or may not want; it is stated rather than hidden.
  • 1K: −7.7 % with the cap. This is a same-ring comparison — at 1K both arms print 32,768 — and the difference tracks the MTP acceptance state (207 vs 197 accepted). I do not claim it as a gain or a regression.

Failed: none observed.

Not run:

  • CUDA and SYCL builds (no such toolkit on this machine). The patch is backend-neutral core code, but I have not compiled it for those backends.
  • The --serve path: 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

  • Opt-in and generate-path only.
  • The 32,768 floor is a constant (the engine's default ring), not a configuration knob.
  • Because the pool is the ring, the ring cannot be resized within a session — only chosen when the session's arena is built.
  • The 64K point used a workload derived as the first 65,536 ids of a synthetic 128K prompt; provenance is in the measurement record.

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.

…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 rahul05ranjan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xyzzing

xyzzing commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Scope correction after reading the serve path: this PR is generate-only, and --serve cannot take the same hook today.

Serve keeps one main session (created before the request loop) plus parked snapshots in the conversation cache (conv_cache / --conversation-cache-*); requests restore into that one session rather than creating one per request. The ring is therefore fixed at engine start, and since the KV pool is the ring, a per-request ring would need per-conversation KV pools — larger than this patch.

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.

@xyzzing xyzzing changed the title kv: size the resident ring from the run's depth (opt-in, STRATA_KV_RING_ADAPT) [Engine] kv: size the resident ring from the run's depth (opt-in, STRATA_KV_RING_ADAPT) Oct 19, 2026
@xyzzing

xyzzing commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

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 3d1fb480 + this patch), interleaved pairs, 256 generated tokens, --kv-resident 49152 as the cap:

tier control (no env) adaptive delta prefill / TTFT
1K 132.2 144.0 +9.0 % 811/842, 1.5/1.4 s
32K 82.2 74.7 −9.1 % 2354/2308, 14.2/14.5 s
128K 69.8 81.7 +16.9 % 2353/2362, 55.9/55.8 s

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.

@xyzzing

xyzzing commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

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 layer.cpp diff has no ring hunk).

32K on v0.2.0-beta, both arms the same binary, MTP off (--mtp omitted), 3 seeds an arm:

arm ring decode tok/s (median, cells)
AN 32,768 39.22 (39.33 / 39.10 / 39.22)
BN 49,152 38.54 (38.89 / 38.54 / 38.43)

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.

@Niko1221

Copy link
Copy Markdown
Owner

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:

  1. Rebase on main if it no longer applies, and re-run your checks on 0.2.0-beta.
  2. Start the title with its area: [Engine], [GUI], [TUI], [Benchmark], [Docs] or [Other].
  3. Fill in the pull request template: the problem and result, and the validation (what you ran, on which revision, what you saw).
  4. Comment /reopen. A check runs and either reopens the pull request or lists what is still missing.

The rules are in CONTRIBUTING.md.

@Niko1221 Niko1221 closed this Oct 19, 2026
@xyzzing

xyzzing commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Closed by the tracker reset rather than a review, so noting where it stands and leaving the call with you.

The branch has moved: v4-kv-ring-adapt is now at 930f9dde, and the rule changed from "grow to need" to "grow only when the ring can cover the context" (needed >= cap, otherwise the live 32,768). The depth-only version this PR was opened with lost 9.1 % at 32K on 0.2.0 with MTP (-1.7 % without) while 128K gained 15.3 % — the draft path pays for a ring it cannot use.

New numbers, all on 0.2.0-beta with the same binary both arms (interleaved pairs, 256 generated):

tier control drafter-aware delta
32K 82.2 82.3 +0.1 % (the ring stays at 32,768; the arm's own log line confirms it)
128K 70.7 81.5 +15.3 %

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.

@xyzzing

xyzzing commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and re-validated on 0.2.0-beta.

  • Rebase: the branch is now two commits on upstream/main (21e8c5ad): the opt-in ring sizing, then the drafter-aware rule. It applies cleanly; the 0.2.0-beta measurements below were taken with this code, built against the v0.2.0-beta tag (3d1fb480).
  • Title: [Engine] kv: size the resident ring from the run's depth (opt-in, STRATA_KV_RING_ADAPT).
  • Problem and result: a single global ring, sized for depth, is paid by every run. The depth-only version lost 9.1 % at 32K on 0.2.0 with MTP (-1.7 % without it) while gaining 15.3 % at 128K — the draft path pays for a ring it cannot use. The rule now grows to --kv-resident only when the ring can cover the whole context (needed >= cap), keeping the engine's default 32,768 otherwise. Opt-in, default behaviour unchanged.
  • Validation, on 0.2.0-beta with the same binary both arms (interleaved pairs, 256 generated tokens): 32K 82.2 -> 82.3 tok/s (+0.1 %; the arm's own log line prints KV ring adaptive: 49152 -> 32768), 128K 70.7 -> 81.5 (+15.3 %), prefill and TTFT unchanged. 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 interleaved pairs at 128K: prefill 2,445-2,459 vs 2,382-2,400, TTFT 53.6-53.9 vs 54.9-55.3 s). The 1K column is a same-ring comparison (both arms print 32,768) differing only by the MTP acceptance state, and is not claimed either way.
  • Not run, stated: CUDA/SYCL builds (no toolkit here), a compile against main after this rebase (the checks above are on the v0.2.0-beta tag base), and the --serve path (one main session plus parked snapshots - a per-request ring there needs per-conversation pools).

/reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants