[hthadicherla]

Overview

Repeated synchronization calls can enter the CUDA runtime even when no stream work has been submitted since the previous wait.

Track stream use inside the CUDA context. Acquiring a stream or BLAS handle marks the stream as used; successful synchronization clears that state. A later synchronization returns immediately if the stream has not been acquired again. Graph, copy and event callers use the existing accessors without separate pending-work bookkeeping.

The stream and BLAS handle arrays are private. Borrowed handles must stay within the current backend call and must not be cached across context synchronization.

The optimization applies only to native CUDA with one logical device. Multiple devices, HIP and MUSA retain unconditional synchronization. The change touches two CUDA files: no scheduler changes, generic backend API/ABI changes, or CUDA kernel changes.

Performance

Full SPEED-Bench coding, C1: 80 conversations / 89 turns per run.

Metric	Stock	This change
Mean conversation decode throughput (tokens/s)	91.11	138.98
Mean conversation latency (s)	7.27	4.89

Decode throughput increased by 52.55% on this setup. The changed version is the mean of two full runs (139.20 and 138.77 tokens/s); stock is the preserved earlier full run. All outputs and generation/draft counters match. These are local Windows measurements, not a general cross-platform performance claim or a statistical equivalence test.

Setup: Qwen3.8-27B + DFlash2, both Q4_K_M, target output Q8_0 shared with the draft; RTX PRO 6000 Blackwell Server Edition, Windows, CUDA 13.4. Context 40960, F16 KV, Flash Attention, greedy seed 1234, 512-token cap, draft length 7, thinking, prompt caching and target/draft backend sampling enabled.

Separate fresh Nsight Systems captures cover three identical coding requests, 512 output tokens each, after an excluded warmup:

CUDA API	Stock calls	This change
cudaStreamSynchronize	36,199	12,370
cudaGraphLaunch	1,142	1,142
cudaMemcpyAsync	26,133	26,133

Synchronization calls decreased by 65.8%; outputs and counters match. The throughput table above comes from unprofiled full runs, not the instrumented captures.

Local validation

• Sampling, grammar parser/integration, CUDA backend sampler, CPU mixed fallback and server grammar request pass.
• External stress probes pass for copies after synchronization, 2D transfers, cross-context copies, events, meta backend, graph reuse, F32 matrix graphs and sequential/parallel scheduler modes, with CUDA graphs enabled and disabled.
• Nsight confirms repeated idle synchronization skips runtime waits on one device. Generic backend dispatch remains unconditional; two virtual devices retain all waits.

Requirements

• I have read and agree with the contributing guidelines.
• AI usage disclosure: AI assisted with the code, local validation and description.

[pwilkin]

/bot review

[ggml-gh-bot]

Automated code review

Review of PR #29796 (llama : avoid redundant synchronization in output getters).

I traced every async work submission and every synchronization site in src/llama-context.cpp to validate the output_pending invariant. Summary of the verification, then findings.

Verification of the core invariant

• graph_compute() is the only place ggml_backend_sched_graph_compute_async is submitted, and it now sets output_pending = true. Setting it unconditionally (even on GGML_STATUS_FAILED) is correct: partial splits may already be in flight, so the getter must still synchronize.
• All async host output copies (ggml_backend_tensor_get_async for logits/embeddings/nextn/layer inputs/sampling tensors, issued in decode(), encode_ubatch() and extract_layer_inputs()) are enqueued after graph_compute() within the same call, and ggml_backend_sched_synchronize() iterates all sched backends, so they are covered by the flag.
• Every internal synchronize() call site (destructor, sched_reserve(), output-buffer realloc at ~line 2189, embd_seq clear at ~1799/1642, the T5 cross-attention hack, all state save/load entry points) goes through llama_context::synchronize(), which clears the flag. No path clears it without actually synchronizing.
• In common_sampler_sample, llama_get_sampled_token_ith() is called before gsmpl->tm(), so the "sampling time measured after context synchronization" property is preserved: the first getter after a decode synchronizes via output_synchronize(), and the CPU-sampling fallback (set_logits -> llama_get_sampled_probs_ith/llama_get_logits_ith) is equally covered.
• Decode-after-decode without getters in between is unaffected: process_ubatch() already resets the sched per ubatch today, so this PR does not introduce a new pending-work hazard there.

No blocking correctness or security issues found. The approach is minimal, reuses the existing synchronize() machinery, and no public API surface changes.

Will slow the review (minor, worth addressing)

(point 1) src/llama-context.cpp:1426 (pipeline-parallel graph-reuse path) calls ggml_backend_sched_synchronize(sched.get()) directly, which completes the pending work but leaves output_pending = true. The flag is then conservative, so a later getter performs one redundant (no-op) backend synchronize. This is harmless, but since you are introducing the flag in this PR, consider having this path also clear it (e.g. call synchronize() there, or factor the sched-sync+clear), or add a short comment noting the flag is deliberately conservative at this site. Calling full synchronize() there would additionally fold timing stats mid-decode, which changes behavior, so if you keep the raw sched call, just document it.

(point 2) src/llama-context.h:360 - the comment "Output getters share completion of the last graph and its host copies." is cryptic. Suggest wording that states the actual invariant, e.g. "a graph has been launched async and not yet synchronized - output getters must synchronize before reading host output buffers".

Nits

(point 3) common/sampling.cpp:595 - the comment "start measuring sampling time after the llama_context synchronization..." now refers to a sync that happens implicitly inside llama_get_sampled_token_ith() two lines above. Consider rewording so it stays accurate (e.g. "... after the synchronization performed by llama_get_sampled_token_ith()").

(point 4) K-shift/defrag memory graphs (llama-kv-cache.cpp:880 calls lctx->graph_compute()) also set output_pending, so the flag can be true with no output-bearing graph pending. That is only a conservative extra sync at most once per decode, so it is fine as-is; no change requested, just noting the flag is not exclusively "output" graphs despite the name.

Everything else checks out: naming/indentation match surrounding style, ASCII only, concise comments, no API or docs changes needed, and concrete perf data is included in the PR description. AI usage disclosure is filled in per the PR template.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[ggerganov]

Any reason to introduce output_synchronize() instead of adding the flag logic directly to the existing synchornize()?

[hthadicherla]

No particular reason against it. I didn't get the idea at the time .
Added the flag logic directly inside synchronize now in the latest commit.

[ggerganov]

My concern is that this bookkeeping is fragile - we can easily forget to set the output_pending flag in the future if another call to ggml_backend_sched_graph_compute_async is added somewhere in the llama_context.

Maybe we should think about moving this flag down inside the ggml scheduler?

[hthadicherla]

My concern is that this bookkeeping is fragile - we can easily forget to set the output_pending flag in the future if another call to ggml_backend_sched_graph_compute_async is added somewhere in the llama_context.

Fair point. I didn't think about there being multiple instances of ggml_backend_sched_graph_compute_async

Maybe we should think about moving this flag down inside the ggml scheduler?

Makes sense, I will do this in the next commit

[ruixiang63]

Could you attach screenshots of the nsys reports to show the before and after comparison for this PR?

[hthadicherla]

Thanks for the suggestion @ruixiang63 . Here are the before/after Nsight Systems captures.

Before:

After:

On three identical public Qwen3.8-27B + DFlash2 coding requests (both Q4_K_M, C1, draft length 7, 512 output tokens each), recorded cudaStreamSynchronize calls fell from 36,199 to 14,341 (-60.4%). Generated outputs and draft counters match.

[hthadicherla]

Updated in latest commit. The flag is now tracked at the shared ggml backend entry points rather than in llama_context. A scheduler-only flag would miss async output copies submitted directly through backend APIs.

I also narrowed the optimization to CUDA through an explicit opt-in. SYCL/virtgpu edits are removed, and all other backends retain unconditional synchronization. Other backends can opt in after their maintainers verify that every async submission path invalidates the tracking state.

The backend API bump is needed for the internal struct layout change. The fallback regression test and local CPU/CUDA checks pass. I updated the description to include the added tests

[ruixiang63]

cc @aendk

[ggerganov]

A scheduler-only flag would miss async output copies submitted directly through backend APIs.

I missed that. So this does not work. Also the new version of the PR does not work - this is much more complex than I initially thought.

For now I am not sure what would be a good way to support this.

Btw ~10% gains from this change sound a bit unrealistic. Would be useful to get a second confirmation of this suggested improvement.

[am17an]

The improvement would probably be in windows where these things cost a lot?

[aendk]

@am17an I highly doubt that 10% is likely here.
This PR can be loosely compared to my previous PRs #17795 and later #20793.
Both tackled the same area. My changes proposed to remove all syncs between memory copies and led to 5-10% speed up in the best case of windows.
This PR proposes to remove only syncs associated with the tensor get/set/copy functions. The ggml_backend_synchronize() in ggml_backend_compute_splits() (L1710,1725, 1733,1748,1771) are untouched. So zooming into the NSight Systems screenshots posted above should still show alternating memcpy-sync-memcpy-graph_launch on the CUDA API row.

[aendk]

this PRs master:

PR:

Quickly ran it through llama-bench on linux. The PR moves from 18 syncs for each token leadup to 10.
However I need to correct myself, the 10% speed up could be realistic for top of the line hardware (RTX PRO 6000 Blackwell) on windows, but represent the absolute best case scenario. In my previous testing, this correlated directly with mem bandwidth, so RTX Pro 4500 with roughly half the bandwidth was closer to half speedup (5% in this case), with lower bandwidth devices benefitting even less.

My stance on this is that medium term, we should continue on the path to first improve testing on the scheduler via #27258 and then to rework the scheduler via #25319, as previously planned.

This could serve as a bandaid-fix for the mean time, but with the same associated risks as my previous linked PRs (which led to them being reverted).
In any case, I consider @JohannesGaessler as the authority on this. Do you think this is worthwhile to introduce this complexity, even temporarily ?

[JohannesGaessler]

My opinion is that we should be conservative with changes to the scheduler until #27258 is done. The way I see it our bottleneck is reviewer time so we should prioritize the things that make this easier, i.e. the tests. Once we have that I would look at this PR in detail, compare it against #25319 , and then see how to proceed from there. I think a 10% speedup, while nice, is not large enough to justify doing things in an order that is suboptimal in terms of maintainer time. So I would say not to merge this PR as a band-aid fix.

[ggerganov]

I agree - let's park it for now.

[hthadicherla]

Thanks for the feedback everyone. I’ve narrowed this to a CUDA-only change: stream usage is tracked inside the CUDA context, with no scheduler changes or backend API bump. The final diff touches just two CUDA files. It preserves the measured C1 benefit, and local correctness tests pass. It still changes synchronization behavior beneath the scheduler, so I understand the caution and will leave the review timing to you.

@ggerganov @aendk , can you PTAL at the PR now ?

[JohannesGaessler]

Is there any measurable performance benefit from just these changes without the ggml backend scheduler changes?

[hthadicherla]

@JohannesGaessler Yes. I measured the CUDA-only change now in latest commit, without the earlier generic backend changes or any scheduler changes. Sorry, I should have made that explicit in my previous comment.

Both arms already use DFlash2 with Qwen3.8-27B (both Q4_K_M, target output Q8_0). On Windows / RTX PRO 6000 Blackwell / CUDA 13.4, at C1 over the full SPEED-Bench coding category (80 conversations / 89 turns per run):

Metric	Stock + DFlash2	CUDA-only change + DFlash2
Mean conversation decode throughput (tokens/s)	91.11	138.98 (+52.55%)
Mean conversation latency (s)	7.27	4.89 (-32.78%)

The changed value is the mean of two full runs (139.20 and 138.77 tokens/s); stock is one preserved earlier run. Requests/settings and all 89 generated outputs, usage and generation/draft counters match, with zero failures. This is the measured benefit on this Windows DFlash2 workload, not a claim of the same gain on Linux or ordinary decoding; an independent reproduction would still be useful.

[hthadicherla]

Btw the improvements numbers are for model with DFlash2. The improvement in perf for just the baseline model without DFlash2 are much less than above.

Comparision in perf with only Qwen3.8-27B model

Metric	Stock	Current PR	Change
Mean conversation decode throughput	51.25 tokens/s	59.92 tokens/s	+16.90%
Mean conversation latency	11.84 s	10.15 s	−14.30%

[ORippler]

Thanks for the feedback everyone. I’ve narrowed this to a CUDA-only change: stream usage is tracked inside the CUDA context, with no scheduler changes or backend API bump. The final diff touches just two CUDA files. It preserves the measured C1 benefit, and local correctness tests pass. It still changes synchronization behavior beneath the scheduler, so I understand the caution and will leave the review timing to you.

Imo it's not the responsibility of a backend to track whether its busy or not, and this PR adds technical debt in that regard.

If we want to invest in improving spec dec, we should instead focus & invest on enabling a path where spec dec fully resides on a backend. We are doing baby steps in that regard already with rejection sampling.

[JohannesGaessler]

I think I'm missing some context for this PR. To my understanding, in its current form essentially all it does is cache in the ggml CUDA code whether or not a call to the CUDA API for synchronization is actually necessary. And I guess on Windows the performance of those API calls is terrible so it makes a difference?

[hthadicherla]

I think I'm missing some context for this PR. To my understanding, in its current form essentially all it does is cache in the ggml CUDA code whether or not a call to the CUDA API for synchronization is actually necessary. And I guess on Windows the performance of those API calls is terrible so it makes a difference?

@JohannesGaessler Yes, that’s essentially what the current version does. After a successful synchronization, subsequent calls skip cudaStreamSynchronize until the stream or BLAS handle is acquired again. Acquiring either conservatively marks the stream as potentially having new work.
The benefit comes from avoiding repeated CUDA API calls when the previous wait has already completed the work.

Imo it's not the responsibility of a backend to track whether its busy or not, and this PR adds technical debt in that regard.

If we want to invest in improving spec dec, we should instead focus & invest on enabling a path where spec dec fully resides on a backend. We are doing baby steps in that regard already with rejection sampling.

@ORippler, I agree that moving speculative decoding onto the backend is a better long-term direction. This overhead also affects ordinary decoding: without speculative decoding, we measured a 16.9% throughput improvement on the same Windows setup. That broadens the benefit, but I understand it doesn’t remove the concern about maintaining additional state in the backend. The tradeoff is whether that measured benefit justifies the bookkeeping while the broader synchronization work progresses.

I would like to hear your thoughts on this.

[hthadicherla]

Until we are sure that this is the right solution for this issue, and the solution is robust enough, it makes sense to keep this PR as draft.

Parking it for now.