Repository navigation
common : take the top-k straight from the logits when top-k starts the chain - #313
Conversation
…e chain common_sampler_sample() built a token array for the whole vocabulary (set_logits) and then let the top-k sampler partial-sort it. When every sampler before top-k does nothing with the current settings (penalties, dry and top-n-sigma off, no logit bias, no mirostat, no backend sampling), no grammar applied first and the reasoning budget not forcing, select the k largest logits directly. The selection repeats the heap steps of libstdc++ std::partial_sort, so entries and the order of equal logits are the same as before. Checked against std::partial_sort on 120000 random arrays (ties, NaN, inf) and on server outputs (sampling, tool calls, penalties, top_k 1/200, logprobs): identical. LLAMA_SAMPLER_FAST_TOPK=0 turns it off. Decode with MTP n-max 2: +1.9 % (greedy benchmark), +1.85 % with the Hermes sampling settings (thinking, budget 16384, top_k 20). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The environment switch of the previous commit was only there to measure the change; the direct top-k selection is now used whenever its conditions hold. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
bri-prism
left a comment
There was a problem hiding this comment.
No blocking findings from this review. The fast-path eligibility checks match the existing no-op conditions, and grammar rejection correctly rebuilds the full vocabulary before resampling. The reasoning-budget FORCING guard also preserves access to a forced token outside the raw top-k.
Independent validation on AppleClang/libc++, CPU-only Release build:
- The PR builds and
test-samplingpasses. - An additional probe exercises the actual
common_sampler_sampleimplementation with controlled logits. All 422 paired samples across 23 configurations match the original path, covering top-k 1/20/128 and fallback at 0/129, active and disabled penalties/DRY/top-n-sigma, reordered chains, logit bias, greedy and stochastic sampling, adaptive-p, mirostat, grammar rejection, grammar-first sampling, and forced reasoning-budget completion. Clone/copy preserve the eligibility flag. - 2,000 selection comparisons against the existing top-k sampler match every selected logit. Unique-logit arrays also match token IDs and order. Of 1,000 tied-logit arrays, 984 differ in token ordering on libc++, confirming the compatibility caveat already disclosed in the PR. This can change fixed-seed output, including which tied maximum greedy sampling selects; it is not equivalent to the old path on this standard library.
Non-blocking follow-up: please retain focused coverage of the new common-sampler path, especially grammar rejection/forcing guards and tie behavior. The existing test-sampling pass alone does not cover this optimization. If preserving existing fixed-seed output on libc++/MSVC is a merge requirement, gate the optimization by supported standard library until that behavior is covered.
I have not reproduced the RTX 4070 performance numbers, run MSVC, or tested backend sampling with a live backend. The comparison above uses controlled logits and a small synthetic model, so it establishes these code paths rather than real-model quality or throughput.
Review assisted by Codex.
|
@bri-prism thanks, all three points are right.
Speed is not the point of this change: on the 4070 the kTrain stack with it is +0.17 % at depth 0 (12 pairs, CI [+0.11, +0.23] %, outputs identical). An A/A control shows +0.10 % for the second run of a pair, so I count it as no change. The checks were run with Claude Code (AI assistance). |
…ng#313 are merged Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Li7aNoBEvKTV1NBubgCCKg
Overview
common_sampler_sample()builds a token array for the whole vocabulary (set_logits, 248k entries) and then lets the top-k sampler partial-sort it. When top-k is the first sampler that does anything (penalties, DRY and top-n-sigma off with the current settings, no logit bias, no mirostat, no backend sampling), no grammar is applied first and the reasoning budget is not forcing, the k largest logits (k <= 128) are now selected directly from the logits pointer. The selection repeats the heap steps of libstdc++std::partial_sort(make_heap on the first k, replace the heap top by every larger logit in index order, sort_heap), so the entries and the order of equal logits are the same as before; a 16-wide block test skips blocks without a candidate. Everything else (the rest of the chain,cur_p) runs unchanged on the k entries.With MTP speculative decoding the sampler runs on 3 positions per step on the host while the GPU is idle (~0.55 ms of a 24.6 ms step). RTX 4070, Bonsai 2 27B PTQ1_0 + MTP head (n-max 2), 8 interleaved A/B pairs of the same binary, outputs identical in all runs: greedy benchmark +1.92 % (CI [+1.85, +2.01] %), thinking sampling (1.0 / 0.95 / 20 / 0.05, seed 42, reasoning budget 16384, context 114688) +1.85 % (CI [+1.75, +1.94] %).
Additional information
LLAMA_SAMPLER_FAST_TOPK(=0restores the old path) that was used for the measurements below, the last one removes it. To reproduce a measurement, build the first commit. Penalties, logit bias, mirostat, backend sampling, k > 128 or a grammar applied first take the old path.std::partial_sorton 120000 random arrays (k 1-128, n up to 248k, many ties, NaN, +-inf, sorted inputs): identical index and logit at every rank. Server outputs with the switch on and off are identical for tool calls with a grammar, thinking, presence/repeat penalty, top_k 1 and 200, and logprobs (tool-call ids ignored). The checks are not part of this PR.__adjust_heap. The identical tie order is therefore guaranteed (and tested) with libstdc++ only; with libc++ or MSVC STL the order of exactly equal logits may differ from whatstd::partial_sortgave before (the result is still a correct top-k). If you prefer not to depend on that, the same speed-up is possible with a plain selection that gives up the tie order.common_sampler_clone/common_sampler_copycarry the newfast_topkmember along.common/sampling.cppchanges.Test results
-DGGML_CUDA=ON -DCMAKE_CUDA_ARCHITECTURES=89.prismat88c4bc60b; the four commits since (SYCL, WebGPU andcuda: fused FWHT quantizer for 64-wide warps (#303)) do not touch the sampler. The branch is rebased on2459f68b5and builds.--spec-type draft-mtp --spec-draft-n-max 2, q4_0 K/V cache, one slot; vocabulary 248320 tokens.LLAMA_SAMPLER_FAST_TOPK, first commit of the branch) flipped through an environment variable, alternating runs A B B A, 8 pairs, paired differences with a 95 % bootstrap interval; run-to-run noise about 0.04 to 0.12 %.--reasoning-format deepseek --reasoning-budget 16384, thinking sampling 1.0 / 0.95 / 20 / 0.05, fixed seed 42): 100.84 -> 102.71 tok/s, +1.85 % (CI [+1.75, +1.94] %), outputs identical, acceptance unchanged (61.6 %).set_logits0.36 ms, top-k partial sort 0.18 ms; nsys with CPU sampling).std::partial_sorton 120000 random arrays (k 1 to 128, n up to 248k, many ties, NaN, +-inf, ascending and descending inputs): identical index and logit at every rank (libstdc++, GCC 16). This test is not part of the PR; I can turn it into a test case intests/if you want it.LLAMA_SAMPLER_FAST_TOPK=0and=1on the same build are identical for: tool calls with a grammar (sampling and greedy), thinking with sampling,presence_penalty1.5 (old path),repeat_penalty1.1 (old path),top_k200 (old path) andtop_k1, andlogprobs/top_logprobs(tool-call ids ignored).test-samplingpasses on the rebased branch (it exercisesllama_sampler_*, notcommon_sampler, so it does not cover the new path).top_kabove 128 (old path), grammar-first sampling with an active grammar (old path by construction).Requirements