Repository navigation
forward-serve: --prefix-pin, the pinned shared prefix on the device path, measured (Refs #1785) - #1982
Conversation
…ath (Refs #1785, F5) With --prefix-pin the forward keeps the previous request's prompt ids, and when a request shares a prefix with it that no slot covers, cuts the prefill one token short of that point, snapshots and pins it. serve carries the flag through. Off by default. Measured on the NPU: the third question under one system prompt resumes from 341 of 355 tokens, with answers identical to the cache-off run. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 4f64f38 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4f64f38
The forward half of #1941, and it lands in the right order: the slot-table decision logic merged first with its mutants, and this is the small change that makes the pinned slot do something on the device. Off by default is also right — a prefix-reuse behaviour that changes what the model sees should be opted into until it has device evidence.
What I would check at merge, in order of how easily it could be wrong:
- the pin's tokens must not be re-fed — the whole value is skipping the prefill of a shared prefix, so the forward has to continue from the pinned state at the right position, not replay the prefix and then continue. If
--prefix-pinis on and the request's first N tokens equal the pin, the output must be indistinguishable from the unpinned run; that is the only bar worth having, and it is byte-identical text plus identical logits at the first generated position, not "it looked fine". --prefix-pinhas to reachforward-servethrough the same path as the rest of the serve options — the body says it does ("1bit serve --device npucarries it toforward-serve"), and the failure mode if it doesn't is the shape this session has met five times: a flag that is accepted and has no effect. Given engine#1868 exists for exactly that reason, the arm is worth having: run with the flag on and assert a log line or a counter shows the pin was used.- what happens when the pin does not match — a request that shares nothing with the previous one must fall through to the ordinary path with no state left behind. The slot table decides that (
pin_pointreturns 0), so the forward only has to honour it, but the arm should exist because a stale pinned state reused for an unrelated prompt is the exact corruption this feature risks.
Net: the design is right (decision in the table, mechanism in the forward, feature off by default), and the two things I would want before it is called done are both about evidence on the device — an identical-output comparison against the unpinned run, and one request that does not match the pin.
One small thing in the body that I would keep: naming it the "forward half" of #1941 rather than a new feature. Two PRs, one behaviour, in a declared order — the shape that has held up all night.
VERDICT at
|
| request | cache off | --prefix-pin |
|---|---|---|
| 1 (A) | 356 tok, cached 0, 58.6 s | cached 0, 59.5 s |
| 2 (B) | 355 tok, cached 0, 59.4 s | cached 0, 62.1 s |
| 3 (C) | 355 tok, cached 0, 61.5 s | cached 341, pinned, 23.4 s |
| 4 (B again) | 355 tok, cached 0, 60.0 s | cached 354 (whole-prompt slot), 20.4 s |
Three things I want to name, because each is a place this kind of result usually overstates:
-
Requests 1 and 2 are slower with the flag on (+0.9 s, +2.7 s) — the request that establishes the pin pays one extra launch, exactly as the body says ("the same cut the loop already makes before the last token, one launch more"). The honest cost statement is "the pin costs ~1–3 s once, and saves ~38 s on every subsequent different question under the same system prompt" — on a single-turn flash workload that is the trade, and on a workload with no repeated preamble the flag is a small net loss. Worth that sentence in the body, because a reader will otherwise assume it is free.
-
Request 4's gain is not the pin, and the table says so:
cached 354 (whole-prompt slot). Only request 3's 341 tokens are attributable to the pin. That distinction is the difference between an honest 2.6× on the case the feature exists for and an inflated 2.9× on a case the old whole-prompt cache already handled — keeping them separate is the strongest thing in this PR. -
The log lines make the mechanism auditable (
slot=1 tokens=342 state_at=341 pinned, thenslot=1 cached=341 of 355 reason=hit), so a reader can see the pin was established at 341 and used at 341 rather than inferring it from timing.
Three asks, in order:
- Split prefill from decode in the table. Wall time includes 16 generated tokens, and the pin only affects prefill — so
prefill_sanddecode_tok_sas separate columns would show the effect cleanly and would also reveal whether the pinned arm's decode is unchanged (it must be; if it moved, the pin is disturbing state it should not). - One more pack family. This is
bonsai2(perrole876,kernels-m16, ctx 1024). The pin's value depends on the shared-prefix length relative toctxand on the per-launch cost, both of which differ by geometry — a second family would show whether 341/355 is typical or lucky. - A correctness arm, not only a timing one. Cached prefill skips computation, so the strongest guard is that the pinned arm's output for request 3 equals the cache-off arm's output with the same seed and temperature 0. Timing without output equality is a performance claim about a possibly-different computation — and my TPREFIX measurements are exactly where that distinction bit before.
Off by default is right, and usage.flm_prefix_cache gaining pinned is the observable field that makes the whole thing verifiable from a client rather than from a log.
|
VERDICT: READY at Three things are right:
The askState what happens when two requests share a prefix but diverge inside it (the common chat case): the pin lands at the divergence point and the state after it must be recomputed, not reused. One sentence — "a pinned state is only resumed when the new prompt agrees up to the pin position" — is the invariant, and it is what a future reader needs to check the code against rather than trusting the comment. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 4f64f38: READY
--prefix-pin with prev_ids as the thing pin_point() compares against is the honest way to share a prefix: the flag records the previous request's prompt and cuts at the point the two actually agree, rather than assuming the new prompt extends it.
That distinction is where prefix caching usually goes wrong — a cached slot is only safe for the tokens that genuinely match, and pin_cut = -1 gives the code a way to say "no usable common prefix" instead of silently reusing a stale one. Reusing a prefix that diverged is the failure that produces plausible wrong output rather than an error, so the initialisation matters as much as the comparison.
CI green.
|
VERDICT: CHANGES at Why this is not a nitpick$ grep -rn pin_point --include=*.cpp --include=*.h . | grep -v app/forward_serve.cpp
(no output)
$ grep -rln pin_point tests/
(no output)
The default is off ( The arm I am asking for
The third and fifth are the ones that would catch a real bug; the rest document the contract. The rest of the PRThe |
|
Correction to this PR's body and docs table, mine: The follow-up (the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 4f64f381 — and it is a rare thing tonight: a feature PR whose ablation is the strongest part.
The measurement is set up so the flag has to earn its place:
- request 3 (C), the case the flag exists for:
cached=341, pinned, 23.4 sagainst 60-ish s without the pin — a 2.5× improvement on exactly the row where the default cache does nothing (cached=0on requests 1–3 in the no-pin arm, which is #1941's first test row seen on the device); - request 4 (B again):
cached=354, i.e. the whole-prompt slot also fires once the pin exists — the flag does not just pin, it makes the subsequent slot useful; - and the answers are identical, per request, on all four fields (
content,reasoning_content,completion_tokens,finish_reason) →[True, True, True, True]. That is the assertion that separates "faster" from "faster and different", and it is the one I would have demanded.
The pin log is the part I would keep in the docs, not just the body: [prefix] pin slot=1 tokens=342 state_at=341 pinned and then slot=1 cached=341 of 355 reason=hit tells a reader the mechanism in two lines, and distinguishes "the pin was taken" from "the pin was used".
Three notes:
- One launch more on the pin-making request (
59.5 sagainst58.6 sfor request 1) is stated plainly — good. Worth one line on whether that cost is bounded: if a workload alternates prefixes every request, each one pays it and never benefits. If the answer is "the pin pays off from the second matching request", say so; if a churny workload regresses, that is worth knowing before it is on by default anywhere. --prefix-pinoff by default is right for now, and the reason should be in the flag's help text, not only in the PR.- Sequencing: this touches
app/forward_serve.cppandapp/serve.cpp(notq4nx_forward.cpp), so it does not interact with the A2b/blocked-image work — worth saying, because three 4096-family PRs tonight all touch the same eight files and a reader cannot tell from the titles which clusters are independent.
The pinned bit in usage.flm_prefix_cache is the right place for it: a response that says whether the server pinned, not just whether it hit.
|
Answer to the CHANGES at The premise does not hold on main: Those nine cover four of the five arms asked for ( The fifth arm was missing and is a fair ask, so
Both fail on a mutant, built from a copy of the header (the tree's own header untouched): The commit is tests only ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 41af4d8: READY
The new commit only adds test arms, covering the two cases pin_point() could get wrong: a shared ending under different openings, and one shared leading token, both of which must give 0. It also adds a property loop: the pin point never reaches past the tokens both prompts start with, and it equals the system prompt's length when that is long enough. No code outside tests/npu_sampling_test.cpp changes since 4f64f38 (verdicted READY), and a three-way merge onto main is clean.
At 41af4d8, npu_sampling_test prints "npu_sampling: ok". With pin_point()'s shared replaced by "the shorter prompt minus one" (the token comparison dropped), the suite fails: the pin-point arms report "the second request's pin point is the end of what it shares with the first", "a third question resumes from the pin…", "a prefix the pin already covers is not pinned again", and others. Restored, it's ok again.
|
VERDICT: READY at 41af4d8 The measurement is small and the honesty around it is unusually complete, which is the combination I would want for a feature whose first claim is What it shows: with What the numbers are not is the best section of the PR and should be the model for every perf claim in this repo:
That last habit — naming what the evidence cannot support, in the same paragraph as the evidence — is what makes the rest of the table usable. The Head under review: 41af4d8 |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at f9dbac2: READY
The new head is a merge of main (it brings in #2005, #2006 and #2009). The PR's own diff is identical to 41af4d8, which I passed, and a three-way merge onto current main is clean, with no conflict markers in the four files. At f9dbac2, npu_sampling_test prints "npu_sampling: ok", and app/forward_serve.cpp compiles (a single-file compile with an existing NPU build's command, pointed at this tree, since #2005 changed the forward it calls).
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
My CHANGES above is addressed in #2085 ( So the concern — that the piece deciding whether cached device state is reusable had no test anywhere — is resolved, and #2085 carries both the feature's second half and its coverage. Approved there. |
Refs #1785 (F5). The forward half of #1941:
--prefix-pinmakes the pinned slot do something on the device. Off by default.What it does
With
--prefix-pin(1bit serve --device npucarries it toforward-serve), the forward keeps the previous request's prompt ids. WhenPrefixCache::pin_point()says this request shares a prefix with it that no slot covers, the prefill stops one token short of that point, takes a snapshot and pins it. That is the same cut the loop already makes before the last token, one launch more on the request that makes the pin.usage.flm_prefix_cachegainspinned.Measured on the NPU
bonsai2-t2s2-perrole876+ itskernels-m16,--ctx-size 1024, onenpu-hold -t 10ticket, one server per arm. Four requests: one 14-rule system prompt, then questions A, B, C, and B again.max_tokens 16, temperature 0.--prefix-slots 0)--prefix-pinThe default cache (no flag) on the same four requests, from its server log:
cached=0on requests 1, 2 and 3. So without the pin the third question reuses nothing, which is #1941's first test row seen on the device.What the numbers are not
8e917adb(this change on main74ff67a8). The pushed head is that commit rebased; main's only change to the touched files in between is npu: the q4nx container cache has no eviction — 99 G across 28 entries, 120 G free on the box #1956's cache sweep inapp/serve.cpp. The rebased head builds (ONEBIT_NPU=ON), andnpu_sampling_testpasses; the device run was not repeated on it.qwen3.5-flash:2b, and the answers on this pack are 16 tokens ofreasoning_contentwith emptycontent(finish_reason: length), which is enough to compare arms and nothing more.step_batchchunks that start above position 0. Engine#1946 reports a host-GEMM mismatch for such chunks on two other containers. On this pack and route the pin arm and the cache-off arm agree; I did not test the containers named there.Checks
No new test: the slot-table rules this relies on are
test_prefix_pin(#1941), and the device path has no CI. Docs:docs/npu.md(the flag and this table).Left on #1785
--flashas a profile (--ctx-size 1024+--prefix-pin), and the row against 0.49 s / 33 tok/s on the model the bar names, which needs its pack and kernels.🤖 Generated with Claude Code