Repository navigation
ggml-cuda: drop the CUDA graph cache count cap, as upstream does - #253
danielhanchen merged 2 commits into
Conversation
The shape keyed graph cache added a 64 entry LRU cap on top of the existing time based sweep, to bound a workload with many distinct shapes. Tensor split needs about 2 * n_layers + 1 graphs per device for a single shape, one per segment between the cross-device reductions, so every model past ~31 layers exceeds 64. The LRU then evicts graphs that are still in use and every token re-captures instead of replaying, which is as slow as running with GGML_CUDA_DISABLE_GRAPHS=1. Upstream tried the same cap (64, then 128) in ggml-org#21611 and dropped it in review for this reason, keeping only the sweep that evicts graphs unused for 10 s. Do the same here. The shape key stays, so an alternating speculative verify batch keeps one graph per shape. Refs unslothai/unsloth#12468
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e8ffdc72f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // bounded only by the time based sweep below, as upstream does: tensor split mode alone | ||
| // needs about 2 * n_layers + 1 graphs per device and shape, so any count cap small enough | ||
| // to matter evicts graphs that are still in use and re-captures them on every token | ||
| std::unordered_map<uint64_t, std::unique_ptr<ggml_cuda_graph>> cuda_graphs; |
There was a problem hiding this comment.
When CUDA or HIP graphs are enabled in a server and clients generate many distinct prompt or batch shapes within the 10-second retention window, each shape and tensor-split segment can now add a cache entry, including a full node_props vector and potentially a graph executable after warmup. The time-based sweep does not impose a count or byte limit, so a sufficiently fast burst can exhaust host or device memory before any entry becomes old enough to evict; it also cannot release stale entries while the server is idle because it only runs from cuda_graph(). Keep a bound sized for tensor-split workloads, or avoid shape-keying one-off prefill graphs.
Useful? React with 👍 / 👎.
…DA graph cache count cap unslothai#253 is unslothai#241 plus one commit that removes the 64 entry cap on the shape keyed CUDA graph cache, which made tensor split re-capture graphs on every token (unslothai/unsloth#12468). Its branch now merges unslothai#241's current head, so pinning it instead of unslothai#241 carries every unslothai#241 item and the fix, and keeps pin_contract from reading the removed cap lines as a lost unslothai#241 change.
unsloth: pin unslothai#253 (no CUDA graph cache count cap) in place of unslothai#241
1 participant
Fixes the tensor split decode regression in unslothai/unsloth#12468: every
-mixprebuilt fromb10715-mix-86bd2d3onwards decodes 2.4x to 4.3x slower than the official ggml-org build of the same tag with--split-mode tensor. Single GPU and layer split are unaffected.Cause
ggml-cuda: key the CUDA graph cache by shape(271f947, from #144, now carried by #241) keys the graph cache by first node, last node and node count, so an alternating speculative verify batch keeps one graph per shape instead of resetting warmup on every call. To stop many distinct shapes from growing the map, it also capped it at 64 entries with LRU eviction, on top of the existing sweep that drops graphs unused for 10 s.That cap was sized from single GPU runs (4 captures for Qwen3.8-27B). Tensor split needs about
2 * n_layers + 1graphs per device for one shape, one per segment between the cross-device reductions, and twice that with MTP. Any model past about 31 layers exceeds 64, the LRU evicts graphs that are still in use, and every token re-captures instead of replaying. The reporter's counters show it: about 230k graphs created and about 230k evicted by the cap in two minutes on Qwen3.8-27B.What upstream does
Upstream has no count cap. ggml-org#21611 started as a 64 entry ring buffer, was raised to 128, and in review the cap was removed because tensor parallel splits each layer into two graphs (one reviewer saw about 800 graphs with
-sm tensoron gpt-oss-120b). What merged (b94050e) is only the time based sweep, and no revision ofcommon.cuhon master has had a count cap since.This PR does the same: it removes
max_cuda_graphsand the LRU loop and keeps the sweep. The shape key stays.Results
Qwen3-4B Q4_K_M (36 layers, so about 73 graphs per device in tensor split), 2x B200 on exclusive leases, CUDA 13.1,
llama-bench -fa 1 -p 0 -n 128 -r 5, three interleaved rounds per arm. Decode tok/s, median of the round means (min to max).Built from the carry branch head (9dd7972) with and without this change, on a quiet host:
Rebuilt at this PR's base, b96a713 (the commit #241 is pinned at), with and without this change. This run was on a heavily loaded host (load average about 500), so the layer and single GPU rows are noise; only the tensor row is a result:
Memory
Measured with
llama-serveron the same model and GPUs,-c 16384, on the carry branch head with the same patch (the cache code is identical). MiB:The extra memory is the graphs that are now kept instead of re-captured, plus host side bookkeeping for one-off prefill shapes until the sweep drops them. It levels off: over six bursts of 60 new prompt lengths, each followed by a sweep, VRAM stayed at 3384 MiB per GPU, and host RSS grew by about 11 MiB per burst late in the run, the same rate as with the cap (about 9 to 13 MiB). Peak RSS (about 3 GB, at load) is unchanged.
Not covered
Branch
The fix commit (
0e8ffdc72) sits on #241's old pinb96a713a. The head (20dd977ea) merges #241's current head9dd797259on top, so it carries the same #241 that #252 pins, plus the cap removal; against #241 the diff is still onlycommon.cuh(+5 -14). It is pinned in #241's slot by #254, stacked on #252; replayed withadditive_merge.pyonto b11491, all 15 pins merge andpin_contract.pyreports all intact.Supersedes
#232 (cap raised to 512) and #238 (cap that doubles under thrash, hard limit 2048). Both target
mtp/qwen4exp-nextn, which is no longer pinned, and both keep a fixed number that upstream's review already ruled out. Thanks to @floewe for the diagnosis and counter runs in the issue, and to @LeoBorcherding for #238.To ship, #254 pins this PR in #241's slot.