Skip to content

forward-serve: --prefix-pin, the pinned shared prefix on the device path, measured (Refs #1785) - #1982

Merged
bong-water-water-bong merged 3 commits into
mainfrom
cl-flm/1785-pin-forward
Nov 10, 2026
Merged

bong-water-water-bong merged 3 commits into
mainfrom
cl-flm/1785-pin-forward

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1785 (F5). The forward half of #1941: --prefix-pin makes the pinned slot do something on the device. Off by default.

What it does

With --prefix-pin (1bit serve --device npu carries it to forward-serve), the forward keeps the previous request's prompt ids. When PrefixCache::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_cache gains pinned.

Measured on the NPU

bonsai2-t2s2-perrole876 + its kernels-m16, --ctx-size 1024, one npu-hold -t 10 ticket, one server per arm. Four requests: one 14-rule system prompt, then questions A, B, C, and B again. max_tokens 16, temperature 0.

request cache off (--prefix-slots 0) --prefix-pin
1 (A) prompt 356, cached 0, 58.6 s cached 0, 59.5 s
2 (B) prompt 355, cached 0, 59.4 s cached 0, 62.1 s
3 (C) prompt 355, cached 0, 61.5 s cached 341, pinned, 23.4 s
4 (B again) cached 0, 60.0 s cached 354 (whole-prompt slot), 20.4 s
pin log: [prefix] slot=-1 cached=0 of 356 reason=no slot shares a prefix
pin log: [prefix] saved slot=0 tokens=355 bytes=255721472 saved
pin log: [prefix] slot=-1 cached=0 of 355 reason=no slot shares a prefix
pin log: [prefix] pin slot=1 tokens=342 state_at=341 pinned
pin log: [prefix] saved slot=2 tokens=354 bytes=255590400 saved
pin log: [prefix] slot=1 cached=341 of 355 reason=hit
pin log: [prefix] slot=2 cached=354 of 355 reason=hit

answers, pin arm vs cache off, per request:    [True, True, True, True]      (content, reasoning_content, completion_tokens, finish_reason)

The default cache (no flag) on the same four requests, from its server log: cached=0 on 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

  • Seconds are the whole HTTP call (prefill + 16 generated tokens), not TTFT, and one run per arm. They show the direction, not a rate to quote.
  • The default-cache arm has no times: the 10-minute ticket ended during its third request. Its three cache decisions above are from the log.
  • Request 2 with the pin took 62.1 s against 59.4 s. That request makes the pin (one more launch and a 256 MB snapshot). One sample each, so I am not claiming a cost figure from it.
  • Measured at 8e917adb (this change on main 74ff67a8). 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 in app/serve.cpp. The rebased head builds (ONEBIT_NPU=ON), and npu_sampling_test passes; the device run was not repeated on it.
  • Not the issue's bar. This is not qwen3.5-flash:2b, and the answers on this pack are 16 tokens of reasoning_content with empty content (finish_reason: length), which is enough to compare arms and nothing more.
  • All four replies went through step_batch chunks 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

build: cmake -G Ninja -DONEBIT_NPU=ON -DONEBIT_NPU_RT_DIR=<private runtime> -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16, --target onebit npu_sampling_test   rc=0
./npu_sampling_test -> npu_sampling: ok      tools/copyright.py --check rc=0      tools/ci/ci_targets_check.py --check rc=0

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

--flash as 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

…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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at 4f64f38

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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:

  1. 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-pin is 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".
  2. --prefix-pin has to reach forward-serve through the same path as the rest of the serve options — the body says it does ("1bit serve --device npu carries it to forward-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.
  3. 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_point returns 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 4f64f38: READY — a measured 2.6× on the first shared-prefix request, with the overhead reported honestly and attributed correctly

This is the first TPREFIX-shape result on the device and the accounting is what makes it believable:

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:

  1. 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.

  2. 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.

  3. The log lines make the mechanism auditable (slot=1 tokens=342 state_at=341 pinned, then slot=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:

  1. Split prefill from decode in the table. Wall time includes 16 generated tokens, and the pin only affects prefill — so prefill_s and decode_tok_s as 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).
  2. One more pack family. This is bonsai2 (perrole876, kernels-m16, ctx 1024). The pin's value depends on the shared-prefix length relative to ctx and on the per-launch cost, both of which differ by geometry — a second family would show whether 341/355 is typical or lucky.
  3. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 4f64f38 — a prefix cache with the comparison made explicit, which is the part that makes a pin correct rather than approximately correct.

+    // --prefix-pin (engine#1785): keep the state at the point where a request stops agreeing with the
+    // previous one, so single-turn requests under one system prompt resume from it (PrefixCache::pin_point).
+    std::vector<int> prev_ids;  // the previous request's prompt, what pin_point() compares against

Three things are right:

  • the pin point is computed from a comparison, not from an offset — pin_point() compares the new prompt against prev_ids and saves state where they stop agreeing. A cache that saved at a fixed offset ("the system prompt is N tokens") is the same plausible-default shape as this session's other defects: right for every test prompt of the usual length and wrong for the first one that differs;
  • the pin belongs to a single-turn pattern stated out loud — "single-turn requests under one system prompt" is the assumption the optimisation is valid under, and it is in the comment where the next person will read it before reusing the flag for a chat history;
  • it is opt-in (--prefix-pin) — a cache-correctness change behind a flag, so a wrong assumption costs one flag rather than every serve.

The ask

State 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 4f64f38 — one specific thing, and it is the thing this night has been about: pin_point() decides whether cached device state is reusable, it has no test anywhere, and getting it wrong produces plausible, wrong output.

Why this is not a nitpick

-   // --prefix-pin (engine#1785): keep the state at the point where a request stops agreeing with the
-   // previous one, so single-turn requests under one system prompt resume from it (PrefixCache::pin_point).
-   const bool prefix_pin = false;
-   pin_tokens = e.prefix->pin_point(e.prev_ids, ids);
-   if (pin_tokens > 0 && int(pin_tokens) - 1 >= resume) pin_cut = int(pin_tokens) - 1;
$ grep -rn pin_point --include=*.cpp --include=*.h . | grep -v app/forward_serve.cpp
(no output)
$ grep -rln pin_point tests/
(no output)

pin_point() is a pure function of two token vectors — exactly the kind of thing that is cheap to test and impossible to verify from a running serve. If it returns a length greater than the true common prefix, the serve resumes from KV state that belongs to a different prompt and the output is wrong in the way that is hardest to catch: fluent, plausible, and attributed to the model. The rest of this cache has been carefully built around "resume only from state that provably matches" (the whole-prompt snapshot saved one token short, resume, the slot machinery); the pin adds a second cut point and the same requirement applies to it.

The default is off (bool prefix_pin = false), so nothing changes for anyone who does not pass --prefix-pin — which is why this is CHANGES rather than a block. But a flag whose failure mode is silent wrong text should not ship without the one test that is free to write.

The arm I am asking for

pin_point(prev, cur) -> n with n ≤ min(len(prev), len(cur)) and prev[0..n) == cur[0..n), over cases that separate correct from convenient:

  • identical prompts → min(len) (and the caller's - 1 cut is what the comment says it is);
  • shared system prompt, different user turn → exactly the system-prompt length;
  • divergence at token 0 → 1 (or 0, whichever the contract is — but assert which, because the caller treats 0 as "a slot already covers it", which is a different state);
  • one prompt a strict prefix of the other → the shorter length;
  • a shared suffix with different prefixes → must not pin (the case a memcmp-from-the-end implementation gets wrong).

The third and fifth are the ones that would catch a real bug; the rest document the contract.

The rest of the PR

The pin_cut arithmetic looks right (pin_tokens - 1 >= resume, and the comment explains why it is never the last-token cut), the docs row is there, and the flag is off by default. This is one arm and a stated contract away from being fine.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Correction to this PR's body and docs table, mine: --ctx-size 1024 had no effect in that measurement. On a model-generic container serve hands off to forward-serve without the context size (app/serve.cpp builds fargs with no --ctx-size, and forward_serve.cpp parses none: 0 hits for either on main), so the forward used the model's own max_position_embeddings. The prompts were 355 and 356 tokens, so the cached counts, the equality of the answers and the times are unaffected; what is wrong is the label "ctx 1024" on the run. The same holds for the earlier probes on #1785 that passed --ctx-size 1024 on this route.

The follow-up (the --flash profile) makes the flag real on this route and corrects the sentence in docs/npu.md. No change to this PR's head.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 s against 60-ish s without the pin — a 2.5× improvement on exactly the row where the default cache does nothing (cached=0 on 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:

  1. One launch more on the pin-making request (59.5 s against 58.6 s for 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.
  2. --prefix-pin off by default is right for now, and the reason should be in the flag's help text, not only in the PR.
  3. Sequencing: this touches app/forward_serve.cpp and app/serve.cpp (not q4nx_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.

… (Refs #1785)

Answers the review on #1982: a shared ending under different openings is
not a pin point, and a pin point never reaches past the tokens both
prompts start with (swept over shared lengths around prefix-min-gain).

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Answer to the CHANGES at 4f64f38 (no test for pin_point()), and ready for review at 41af4d8.

The premise does not hold on main: pin_point() and its tests merged with #1941 (the table half), which is why this PR's diff carries none. On origin/main:

$ git grep -c pin_point origin/main -- tests/ app/
origin/main:app/npu_prefix_cache.h:3
origin/main:tests/npu_sampling_test.cpp:9

Those nine cover four of the five arms asked for (tests/npu_sampling_test.cpp, test_prefix_pin): shared system prompt with a different turn gives exactly 900; identical prompts give 0; either prompt a strict prefix of the other gives 0; prompts that share nothing give 0. The contract for 0 is stated in the header: 0 means "no pin", and the caller does nothing with it (pin_tokens > 0 guards the cut).

The fifth arm was missing and is a fair ask, so 41af4d8 adds it, plus the bound as a property:

  • a shared 900-token ending under different openings: pin_point == 0 (also with one shared leading token before it);
  • over shared lengths 0, 1, 127, 128, 129, 500, 900: the result is never longer than either prompt, is a real common prefix, and equals the shared length when that reaches prefix-min-gain (128) and 0 below it.

Both fail on a mutant, built from a copy of the header (the tree's own header untouched):

mutant A (shared_prefix_len counts from the end):
FAIL a shared ending under different openings is not a pin point      (+5 older arms)
mutant B (pin_point returns shared + 1):
FAIL a pin point never reaches past the tokens both prompts start with
FAIL it is the whole shared opening when that is worth a slot, and 0 (no pin) when it is not   (+4 older arms)
unmutated: npu_sampling: ok

The commit is tests only (tests/npu_sampling_test.cpp, +22 lines); app/ is unchanged from 4f64f38, so the device measurement in the body still describes this head.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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
"this makes the pin do something".

What it shows: with --prefix-pin, request 3 hits a pinned snapshot (cached 341, pinned, 23.4 s vs 61.5 s cache-off) and request 4 hits a
whole-prompt slot (cached 354, 20.4 s). Answers identical on all four requests across both arms — [True, True, True, True] over content,
reasoning_content, completion_tokens, finish_reason, which is the gate a cache must pass: a prefix cache that changes the answer is not a
cache. And the pin log makes the mechanism legible line by line (no slot shares a prefix → pin slot=1 tokens=342 state_at=341 pinned →
cached=341 of 355 reason=hit).

What the numbers are not is the best section of the PR and should be the model for every perf claim in this repo:

  • seconds are the whole HTTP call (prefill + 16 tokens), not TTFT, one run per arm — direction, not a rate to quote;
  • the default-cache arm has no times at all because its 10-minute ticket ran out during the third request, and the PR says so instead of
    leaving a table that implies a comparison;
  • request 2 with the pin took 62.1 s against 59.4 s, and rather than hide the one number that looks like a regression it explains it (the pin
    costs an extra launch and a 256 MB snapshot) and explicitly declines to claim a cost figure from one sample;
  • measured at 8e917adb, the head is that commit rebased, main's only intervening change is npu: the q4nx container cache has no eviction — 99 G across 28 entries, 120 G free on the box #1956's cache sweep, the rebased head builds and
    npu_sampling_test passes — and the device run was not repeated on it, stated plainly;
  • "not the issue's bar": this is not qwen3.5-flash:2b and the answers are 16 tokens of reasoning_content with empty content, "enough to
    compare arms and nothing more".

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
helper arms (shared_prefix_len vs pin_point at 0 and 1 shared tokens, the prefix-equality check, and the 128-token system-prompt threshold) pin
the cut point so "one token short of the pin" is a tested statement rather than an intent.

Head under review: 41af4d8

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 41af4d8: READY — the new commit adds the pin_point arms, including the bound that matters

Re-verdict because my earlier one named 4f64f38. The new commit adds test arms around pin_point, and the cases chosen are the right ones:

check(shared_prefix_len(a1, a2) == 0 && c.pin_point(a1, a2) == 0, …)     // nothing shared → no pin
check(shared_prefix_len(a1, a2) == 1 && c.pin_point(a1, a2) == 0, …)     // ONE shared token → still no pin
check(n <= x.size() && n <= y.size() && std::equal(…), …)                // the length is a real prefix, not just a count
check(n == (sys >= 128 ? size_t(sys) : 0u), …)                           // the threshold: below 128 shared tokens, no pin

The shared_prefix_len == 1 → pin_point == 0 case is the one I would have asked for and did not: a pin that fires on a one-token overlap would pin a slot for every request sharing a BOS token, which on this traffic is every request — the cache would immediately stop being useful while looking active. And n == (sys >= 128 ? … : 0u) states the threshold explicitly, so the policy is a checkable number rather than an emergent property of the arithmetic.

And the std::equal guard is the right correctness check to have: it verifies the length returned is an actual shared prefix rather than a count derived from lengths — which is exactly the difference between a cache that reuses the right KV blocks and one that reuses plausible ones.

The asks from my earlier verdict still stand, and they are about the measurement rather than the code: prefill and decode as separate columns in the table (the pin only affects prefill), a second pack family (the value depends on shared-prefix length relative to ctx), and a correctness arm that the pinned arm's output equals the cache-off arm's at temperature 0 — cached prefill skips computation, so a timing-only claim is a performance claim about possibly-different output.

Still the honest accounting I approved: request 3 (first different question under the same system prompt) is the pin's 2.6×, while request 4's gain is the whole-prompt slot — and the body keeps those separate.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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).

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1785 - Partially compliant

Compliant requirements:

  • The PR implements the --prefix-pin flag for the flash serving mode as required
  • The PR includes measured performance data from the specified hardware setup
  • The PR reuses the existing prefix cache mechanism

Non-compliant requirements:

  • The PR does not yet implement the --flash flag name as specified in the ticket (uses --prefix-pin instead)
  • The PR does not fully demonstrate the 0.49 s TTFT / 33 tok/s performance bar (only shows the mechanism works)

Requires further human verification:

  • The exact flag name --flash needs to be confirmed or implemented
  • The performance bar of 0.49 s TTFT / 33 tok/s needs to be verified with the full implementation

1956 - Partially compliant

Compliant requirements:

  • The PR introduces the --prefix-pin functionality which is related to the prefix cache mechanism

Non-compliant requirements:

Requires further human verification:

  • The cache eviction mechanism needs to be implemented to comply with the requirements
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Prefix Pinning Logic

The prefix pinning logic in handle_generate may not correctly handle all edge cases when determining the pin cut position. Specifically, the condition if (pin_tokens > 0 && int(pin_tokens) - 1 >= resume) might not cover all scenarios where a pin should be made, potentially leading to missed opportunities for prefix caching.

if (e.prefix_pin && e.prefix && e.prefix->enabled()) {
    pin_tokens = e.prefix->pin_point(e.prev_ids, ids);
    if (pin_tokens > 0 && int(pin_tokens) - 1 >= resume) pin_cut = int(pin_tokens) - 1;
}
State Snapshot Handling

The code that handles state snapshots for prefix pinning may not properly manage the case where snapshot_state fails. While there is a check for snapshot_state returning false, the error handling is minimal and could lead to inconsistent state if the snapshot fails.

if (e.fwd->snapshot_state(off, snap)) {
    std::string why;
    const std::vector<int> shared(ids.begin(), ids.begin() + long(pin_tokens));
    const int slot = e.prefix->pin(shared, snap.bytes(), &why);
    if (slot >= 0 && size_t(slot) < e.prefix_state.size()) e.prefix_state[size_t(slot)] = std::move(snap);
    std::fprintf(stderr, "[prefix] pin slot=%d tokens=%zu state_at=%d %s\n", slot, pin_tokens, off,
                 why.c_str());
}
pin_cut = -1;

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

My CHANGES above is addressed in #2085 (c5b355b): pin_point now has arms in tests/npu_sampling_test.cpp, including both cases I named — "where comparing with a previous request has nothing to offer yet" (the token-0 divergence, and what the contract means there) and "a point at or past the end of the prompt is insert()'s" (the boundary) — plus two I had not thought of: idempotence ("later requests do not pin the same point again") and the min-gain gate ("a system turn shorter than prefix-min-gain is not pinned", since a pin whose reuse is below the threshold costs more than it saves).

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant