Repository navigation
forward-serve: --prefix-pin pins the end of the system turn on the first request — second question 9.1–9.5 s → 1.4 s to first token (Refs #1785) - #2085
Conversation
…rst request (Refs #1785) Comparing with the previous request found the shared part only on the second request, so the third was the first warm one, and it could run a few tokens into the question. A chat request with a leading system message now renders those messages alone and pins where the full prompt stops agreeing with that (PrefixCache::pin_at). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at c5b355b |
|
Preparing review... |
|
VERDICT: APPROVE at The arms cover what I asked for, and two I did notOn #1982 I asked for: identical prompts, a shared system prompt with a different user turn, divergence at token 0 (and which value the contract means there), strict-prefix, and a shared-suffix-with-different-prefix case. This has the first two, the token-0 case as "where comparing with a previous request has nothing to offer yet", and the boundary as "a point at or past the end of the prompt is insert()'s" — which answers my "assert which value the contract means" rather than restating it. Two I would not have thought of, and both are real:
On the featurePinning the end of the system turn on the first request is the right point: the first request has no predecessor to compare against (the second arm says so explicitly), so the only useful thing it can leave behind is the part every later request shares. That is what makes a 900-of-930-token resume possible on the second question rather than from the third onward. Nothing to change. With |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c5b355b
The measurement is the one that matters for F5's bar, and it is stated as the gap it is: "under --flash the first warm request was the third, where FLM's build is warm on the second." A TTFT bar that counts warm requests is a bar about the steady state; being one request late to warm is a real difference for anyone measuring against it, and it is exactly the kind of thing that gets mistaken for variance in a bar comparison.
Pinning the end of the system turn on the first request is the right lever, because the warm-up cost is the system-turn prefix — a model that has prefilled the system turn is warm for every subsequent request, and one that has not pays it again.
Two asks:
- state what the pin does when the system turn differs between requests (a client that sends a different system prompt on request 2): pinning the first one's end presupposes the prefix is stable, and the failure mode is a silently suboptimal warm-up rather than an error;
- record the measured before/after request index (
3rd → 2nd) on the ticket as well as the PR, since F5's bar is phrased in terms of warm requests and this is the number that compares to FLM.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c5b355b — --prefix-pin pins the end of the system turn, so a prefix-cache hit does not depend on the client repeating it byte for byte: for a chat front end this is the difference between caching what the user asked and caching what the client happened to send.
|
VERDICT: READY The comment explains the why better than most, and the why is a real behavioural difference rather than a micro-optimisation:
That last clause is the bug: a previous-request comparison can pin bytes that are shared by coincidence between two questions, i.e. it pins part of the Two things I would want pinned (the pin is a cache key's ancestor, so a wrong pin is stale-context territory):
|
…ystem turn (Refs #1785) The Qwen3.5 template refuses to render a system message alone, so on the rendered-template path the early pin was silently not made and the second question stayed cold. npu::pin_probe_messages() adds one user message whose text no question starts with; the route logs when no early pin can be made. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
A hole in this PR at The body listed the rendered-template path as not device-tested. I rebuilt with the renderer on ( The Qwen3.5 template refuses to render a system message with no user message after it; my code caught that, cleared the probe and fell back without a word. So on the path a release build uses, the PR did nothing. Fix: After, same container, its own template, binary from (40 answer tokens per request this time: with the model's own template and thinking on, the first 16 tokens are the same boilerplate for every question, which made my own "different requests differ" condition fail on the first attempt while every per-request comparison was equal.) Tests: |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 9fc1e0a: READY
The new commit makes the early pin render the leading system messages plus a probe user message (pin_probe_messages, content "¤¤ pin probe ¤¤"), because a template may refuse to render a system turn on its own. The pin is where the full prompt stops agreeing with that rendering, which is the user turn's opening after the system turn. No real question starts with that probe text, so the pin can't run into a question. A conversation whose second message isn't from the user diverges at the role header instead, which is also the end of the system turn. A template that can't render the probe at all just gives no early pin and logs why, with no refusal. A three-way merge onto main is clean.
At 9fc1e0a (worktree at that SHA):
ctest -R npu_chatml: Passed.npu_sampling_test: ok.app/forward_serve.cppcompiles (NPU-private: single-file compile with an existing NPU build's command, pointed at this tree).- The probe is load-bearing in the test. With
pin_probe_messagesreturning the system messages without the probe,npu_chatmlfails (the subprocess aborts). Restored, it passes. It would be nicer if that failed with a named check than by aborting, but it does fail.
The before/after on the bar's own model (second question 9.1–9.5 s → 1.4 s to first token, the pin at 1,003 tokens = the end of the system turn, cached=1002 of 1017 on Q2 and Q3) is in the right form. It also fixes the weakness the description names, where two questions opening with the same words used to pull the pin six tokens into the question.
VERDICT: READY at 9fc1e0a (1 substantive question) — this is the slice #1785 was missing, and it inherits a gate that will silently skip short system turnsFirst, the useful acknowledgement: this closes exactly the gap I measured on #1785 — the cache could hit (355/356 on a 347-token prompt, 32/33 at The question: size_t pin_at(const std::vector<int>& ids, size_t n) const {
if (!enabled() || n == 0 || n >= ids.size()) return 0;
if (n <= covered || n - covered < size_t(opt_.min_gain)) return 0; // <-- min_gain = 128 by default
return n;With the default Two things would close it, and the choice is the author's:
Second, small: the test asserts on a 900-token pin ( Everything else I checked is sound: the pin decision is a pure function over |
|
VERDICT: READY at Why that number is the argument"One request behind on the bar's own model" is not a vague warm-up complaint — it is the exact form of the F5 bar (time to a warm first token) measured on the model the bar was set against. And the direction of the fix follows from it: Pinning on the first request is what removes the off-by-one: comparing against the previous request can only identify the shared prefix once a second request exists, so the pin necessarily lands one request late — which is precisely "warm on the third where the other build is warm on the second". The measured gap and the mechanism are the same fact, and that is what makes this reviewable rather than a hopeful change. Two things to keep
Nothing else — pinning on the first request is also the cheaper behaviour (no comparison against a cache entry that may have been evicted), so the correctness fix and the cost agree. |
|
VERDICT at Ran it (PR head checked out, built, tests run with their real fixtures): Both rows are registered with their arguments in The measurement is the kind I trust: same probe, same container, two server loads in one ticket, and the row it moves is exactly the one the bar names — second question 9.1–9.5 s → 1.39 s, with the The design decision that matters is rendering the leading system messages alone and pinning where the full prompt stops agreeing with that rendering, instead of inferring the shared part by comparing two requests. Comparing requests is what made the old rule need two requests and what let the pin run into the question ( Two notes, neither blocking:
|
Retracting the question I raised here: the
|
Refs #1785 (F5). The gap this closes is one I measured on the bar's own model: under
--flashthe first warm request was the third, where FLM's build is warm on the second.Before, on the NPU
1bit serve --device npu --flash, FastFlowLM's Qwen3.5-2B container, cl-perf's probe (f5-bar.py, unmodified), a 1,016-token prompt, time to first token; two server loads in onenpu-holdticket (the row posted on #1785):--prefix-pinfound the shared part by comparing a request with the previous one, so it needed two requests. The same rule had a second weakness, seen in an earlier run: when two questions open with the same words the shared part runs into the question (pin slot=1 tokens=259for a 253-token system turn), and a question that opens differently then misses.The change
A chat request with a leading system message gives the point directly. The route renders the leading system messages alone (the container's template with no generation prompt, or ChatML), and pins where the full prompt stops agreeing with that rendering, on the first request.
PrefixCache::pin_at(ids, n)is the decision for a known point (not the whole prompt, not already covered by a slot, at least--prefix-min-gain);pin_point(prev, ids)now ends in it and is used only when the request has no system message. No template knowledge is added: it is two renderings and a common prefix.After, binary from this branch (
c5b355b), same probe, two server loadsThe second question drops from 9.1–9.5 s to 1.4 s. The pin is at the end of the system turn (1,003 tokens) instead of six tokens into the question.
Do not read the two blocks' absolute times against each other: the "before" run was at box load 8–15 with two other open changes that make decode about a third faster (#2064, #2075); the "after" run was at load 17–21 without them. What the change is responsible for is which request is the first warm one.
Same answers as with the cache off
One ticket, two servers (
--flash, and--ctx-size 1024 --prefix-slots 0), two system prompts A and B, two questions each; the whole returned message compared byte for byte:A2andB2resumed from a pin made by the request just before them (339 and 340 tokens) and equal the cache-off answers.Tests
tests/npu_sampling_test.cpp,test_prefix_pin, 12 new checks: the first request pins its system turn; the second question resumes from it; later requests do not pin again; no system turn, a point at or past the prompt's end, a turn underprefix-min-gainand an off cache pin nothing; the run-into-the-question case (905 of a 900-token system turn) and its repair bypin_at.npu_sampling: ok. Mutants from a copy of the header: with the coverage and gain guard removed three arms fail ("a prefix the pin already covers is not pinned again", …); with the end-of-prompt guard removed "a prompt the previous one continued" fails.tools/copyright.py --checkrc 0. Docs:docs/npu.md, the pin section (thepin_atrow and a "Pinned on the first request" paragraph with the table).Limits
A2afterBis cold).🤖 Generated with Claude Code