Repository navigation
sampling : read context outputs with one synchronization - #30196
hthadicherla wants to merge 7 commits into
Conversation
|
Please provide exact description what is behind "Full SPEED-Bench" Was there any warm-up ? How to reproduce the results on any other machine ? 42 % is a nice-huge win... |
| @@ -146,3 +146,15 @@ LLAMA_API uint32_t llama_model_target_layer_ids_n(const struct llama_mode | |||
| // if out is nullptr, returns the number of tokens without writing to out | |||
| // caller must allocate enough memory for out before calling | |||
| LLAMA_API uint32_t llama_model_get_tok_embd(const struct llama_model * model, float * out); | |||
|
|
|||
| // Read one sampling row after synchronization. Borrowed pointers are valid until the context outputs change. | |||
| struct llama_sampling_output_view { | |||
There was a problem hiding this comment.
I feel we should avoid adding this kind of new API here. cc. @ggerganov
There was a problem hiding this comment.
@ruixiang63 , i agree but i don't see a good way to implement this otherwise.
I could add a flag for bookkeeping, that would avoid having to create a new LLAMA_API. But i feel like that is just maintainence overhead.
@ggerganov, any thoughts on this ?
|
agree. |
|
Yes, I am starting to have serious doubts about the testing methodology that @hthadicherla is using. It's not possible to have such massive gains just from sparing a few redundant syncs. Even on Windows this seems unbelievable. Let's understand first what we are testing in details. There is most certainly some flaw in the methodology. Also have to wait for this fix to land: #30223. We found out yesterday that backend sampling causes graph reallocations. So depending on how the requests are being sent, it can result in significant overhead. |
@ggerganov , I tried benchmarking another machine with the same RTX 6000 PRO GPU, since I was suspecting the perf gains as well, to see if if the perf gain repro there as well or not. I observed that it doesn't reproduce the same perf gain. Machine 1 (where i observed huge perf gains):
Machine 2:
I'm observing way more throughput in machine 2 and way less and reasonable perf gain here. I'm suspecting this issue happened in machine 1 because of some undervolting or some issue with the GPU and some other config differences in these machines (first machine has an Intel CPU (Intel(R) Xeon(R) w5-3425) and second has an AMD one (AMD 7800X3D)). It might have been possible that cudaStreamSynchronize was way more efficient with AMD CPU than an Intel one (seems a bit farfetched but could be possible). Methodology used was essentially the same for both machines and is as follows:
Server Launch command: for nemotron we append this --override-kv tokenizer.ggml.pre=str:tekken. Finally the speedbench command: Will update the results in the description with the machine 2 one, since that seems reasonable. Apologies for any confusion, i should have noticed this earlier. |
@ruixiang63 These numbers seem to be coming from an faulty config. Please find the updated numbers in the earlier comment. The larger gain with Nemotron can be explained by the draft model’s sampling path. Ordinary DFlash calls common_sampler_sample() for each draft position, while DFlash2 selects draft tokens through its selector lattice and bypasses those calls. This means the PR can benefit both draft selection and target verification with DFlash, whereas with DFlash2, the benefit comes from target verification. if (is_dflash2) {
const float * lattice = llama_get_embeddings_nextn(ctx_dft);
int32_t predecessor = 0;
for (int32_t i = 1; i < n_block_tokens; ++i) {
const float * row = lattice + (size_t) (beg + i) * n_embd_dec;
const float * scores =
row + selector_top_k + (size_t) predecessor * selector_top_k;
predecessor = (int32_t) std::distance(
scores, std::max_element(scores, scores + selector_top_k));
// Confidence checks omitted.
result.push_back((llama_token) row[predecessor]);
}
// Minimum draft-length check omitted.
continue;
}Ordinary DFlash calls for (int32_t i = 1; i < n_block_tokens; ++i) {
common_sampler_sample(smpl, ctx_dft, beg + i, true);
const auto * cur_p = common_sampler_get_candidates(smpl, true);
const llama_token id = cur_p->data[0].id;
// Logging and confidence checks omitted.
common_sampler_accept(smpl, id, true);
result.push_back(id);
}Therefore, ordinary DFlash exercises the optimized sampler during both draft selection and target verification. DFlash2 bypasses it during draft selection, while target verification still benefits. This supports expecting a smaller benefit for DFlash2, although it does not establish the magnitude of the performance difference. Source: I have updated the description with DFlash results since it doesn't seem to benefit DFlash2 significantly. |
|
This is a valid performance improvement. Do you think this PR is a general improvement, or does it only benefit speculative decoding? If it’s a general optimization, I would expect less synchronization in the scheduler, based on my understanding of the previous discussions. cc @aendk |
It should be general optimization since the common_sampler_sample gets invoked for ordinary decoding as well, although this seems to be more observable for DFlash since this function is called for each draft position which seems to compound to a noticable perf gain. In tools/server/server_context.cpp: if (slot.can_speculate() && !slot.spec_draft.empty()) {
return; // sample using speculative decoding
}
// shifted according to the current sub-batch
const int tok_idx = slot.i_batch - off;
llama_token id;
{
scoped_timer timer(t_sampl, n_sampl);
id = common_sampler_sample(slot.smpl.get(), slot.ctx_tgt, tok_idx);
}
slot.i_batch = -1;
common_sampler_accept(slot.smpl.get(), id, true); |
|
Have you tried testing this in other backends, such as The change in common/sampling.cpp:606 (removing The CI here shows no vk-testing, so I would suggest doing so manually (separate build with Edit: @0cc4m for viz |
Sure, Let me get the Vulkan results as well and paste in this thread. |
|
I feel we should expose a batch-level helper that synchronizes once and then populates all required accessors, something like: llama_process(ctx, LLAMA_PROCESS_TYPE_DECODE, batch);
const llama_outputs * outputs = llama_get_outputs(ctx); // waits and reorders once
for (int idx : positions_to_verify) {
llama_token token = common_sampler_sample_from_outputs(smpl, outputs, idx); // sample without waiting again
common_sampler_accept(smpl, token, true);
// Existing acceptance and stopping rules.
}We already work on batches as logical entities in llama.cpp, and this would improve EAGLE3/spec dec methods that require access to LLM's embeddings as well. |
I feel this is more proper way to do that as well. |
Tested the PR unchanged against its parent commit using separate Vulkan-only builds (-DGGML_VULKAN=ON, CUDA disabled).
That makes sense. I’ll try this approach: synchronize once for the batch, then reuse the outputs for sampling and embedding access. I’ll implement it locally and compare performance and correctness against the current approach before updating the PR with the changes and results. |
|
I’ve simplified this to one batch-level API, llama_get_outputs(), and removed the sampling-specific API and exported accessor methods. The server now shares the prepared outputs across speculative verification slots, avoiding repeated preparation of the same batch. Ordinary decoding retains its existing getter path and sampling-time accounting. CUDA validation showed identical output tokens and draft counters, with an 8.85% improvement over the pre-PR baseline for Nemotron Lightning + DFlash. Updated the PR description with commands used for benchmarking for reproducibility. To keep this PR focused and easier to review, I’ll add the dedicated tests in an immediate follow-up PR. @ggerganov @ruixiang63 @ORippler, can you PTAL at this PR now ? |
0ed381d to
cddd5b8
Compare
|
Disclosure: my own measurements; data gathering done with AI assistance. RADV datapoint (RX 7900 XTX, Mesa 26.2.4, llama.cpp master e467e5b,
|
Assisted-by: Codex
Assisted-by: Codex
This reverts commit dd16baf6bb7ccd3fc670ac91a44c39a2f9db3d19. Assisted-by: Codex
Assisted-by: Codex
Assisted-by: Codex
cddd5b8 to
77c1d26
Compare
Assisted-by: Codex
|
@ggerganov @ruixiang63 @ORippler, could you PTAL at this PR and let me know if there is anything that needs to be done ? It has been open for a while now. |
Yes, so the benefit should be more observable on higher draft tokens.
It would be helpful if you can do that. Thanks. |
ORippler
left a comment
There was a problem hiding this comment.
Not sure if there is a need to add tests to test-backend-sampler for this, the logic seems fairly self-explanatory (and if the synchronize in llama-ouputs does not synchronize we should see all kinds of weirdness/races)
Assisted-by: Codex
|
Let's apply this to all spec-dec techniques/more generally now |
Overview
Add
llama_get_outputs(ctx)to synchronize, reorder, and prepare batch outputs for reuse. Speculative verification and DFlash draft loops read from this view, with server verification sharing it across slots.common_sampler_sample_from_outputs()uses the existing sampling logic without additional synchronization. The view also provides embedding access and remains valid until the context outputs change or the context is freed.Ordinary decoding retains its existing scalar-getter path and sampling-time accounting. Scalar getters are unchanged.
Benchmarking
Tested on Windows 11 with an RTX PRO 6000 Blackwell, Ryzen 7 7800X3D, CUDA 13.4 and NVIDIA driver 597.16.
Full SPEED-Bench coding subset: 80 conversations / 89 turns, one run per version. Comparison is against the original pre-PR baseline.
Updated results (10/22) against baseline d88040c:
Throughput is mean conversation decode tokens/s. Acceptance is accepted/proposed draft tokens. All output tokens and per-request draft counters matched.
Commands used to run the benchmark
llama-server
Target and draft are the NVFP4 GGUFs. Use the same model files and command for both builds.
llama-server -m nemotron-target.gguf -md nemotron-draft.gguf \ --spec-type draft-dflash --spec-draft-n-max 7 --spec-draft-p-min 0 \ --backend-sampling --spec-draft-backend-sampling \ -c 40960 -np 1 -ngl 99 -ngld 99 -fa on -ctk f16 -ctv f16 \ --spec-draft-type-k f16 --spec-draft-type-v f16 \ -b 2048 -ub 512 -t 12 -tb 12 --seed 1234 --fit off \ --jinja --chat-template-file chat_template.jinja \ --chat-template-kwargs '{"enable_thinking":true,"preserve_thinking":true}' \ --override-kv tokenizer.ggml.pre=str:tekken \ --metrics --no-webui --host 127.0.0.1 --port 9931SPEED-Bench
Stock SPEED-Bench command with the same dataset and generation settings:
python tools/server/bench/speed-bench/speed_bench.py \ --url http://127.0.0.1:9931 --bench qualitative --category coding \ --osl 512 --concurrency 1 --output before.json \ --extra-inputs '{"temperature":0,"seed":1234,"cache_prompt":true,"chat_template_kwargs":{"enable_thinking":true,"preserve_thinking":true}}'Model Links:
Nemotron Lightning Target Model : Link
Nemotron Lightning Draft Model : Link
Run against each build, changing the output filename to
after.jsonfor the PR build.Measurement note: The reported results used saved token arrays and conversation histories through
/completion, with a separate 128-token warmup excluded. The stock command above uses/v1/chat/completionsand builds conversation histories from generated responses, so it is not an exact reproduction of that replay.Validation
Sampling and embedding first-read tests passed on CPU, CUDA, NVIDIA Vulkan and AMD Vulkan, covering sparse outputs, multiple sequences/microbatches, negative indices and repeated decodes. Shared-library first-read tests also passed.
Before/after server checks matched for ordinary decoding, CPU/backend sampling, probabilities, grammar, rejection sampling and confidence pruning. Vulkan synchronization validation reported no errors. Core sampling/grammar tests passed. Existing backend-sampler failures (
test_logit_bias,test_penaltiesandtest_multi_output_multi_sequence_dist, depending on backend) reproduced in both builds; there were no new failures.Requirements