perf: Conditional gallocr caching for Fast-AR decode on pure CPU runs (%4~ faster)#43
Open
Skyrion9 wants to merge 2 commits into
Open
perf: Conditional gallocr caching for Fast-AR decode on pure CPU runs (%4~ faster)#43Skyrion9 wants to merge 2 commits into
Skyrion9 wants to merge 2 commits into
Conversation
… %4~ faster CPU inference. - Implemented ggml_gallocr_t with proper lifecycle management. - We check if we're running purely on CPU and use the cached codepath, or fallback to old behavior when any GPU backend is involved. - We effectively eliminate redundant allocation cycles in Fast-AR decoding loop for pure CPU configs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesSlowARModel execution
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/s2_model.cpp (1)
1238-1289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the tensor-set calls shared by both branches.
The
ggml_backend_tensor_setcalls forhidden0,positions, andprefix_ids(Lines 1254-1258) are byte-for-byte identical to the ones in the GPU-offload branch (Lines 1275-1279). Only the allocation and compute calls actually differ between branches; the input-setting step can be pulled out into a single shared block after allocation succeeds in either branch, avoiding the two copies drifting apart later.♻️ Proposed refactor
if (is_cpu_only) { if (!fast_gallocr_) { fast_gallocr_ = ggml_gallocr_new(ggml_backend_get_default_buffer_type(backend_cpu_)); if (!fast_gallocr_) { std::fprintf(stderr, "[fast_decode] gallocr creation failed\n"); ggml_free(ctx0); return false; } } if (!ggml_gallocr_alloc_graph(fast_gallocr_, gf)) { std::fprintf(stderr, "[fast_decode] gallocr alloc failed\n"); ggml_free(ctx0); return false; } - - ggml_backend_tensor_set(hidden0, hidden_in.data(), 0, hidden_in.size() * sizeof(float)); - ggml_backend_tensor_set(positions, pos_vals.data(), 0, pos_vals.size() * sizeof(int32_t)); - if (prefix_ids) { - ggml_backend_tensor_set(prefix_ids, prefix_tokens.data(), 0, prefix_tokens.size() * sizeof(int32_t)); - } - if (ggml_backend_graph_compute(backend_cpu_, gf) != GGML_STATUS_SUCCESS) { std::fprintf(stderr, "[fast_decode] cpu compute failed\n"); ggml_free(ctx0); return false; } } else { ggml_backend_sched_reset(fast_sched_); if (!ggml_backend_sched_alloc_graph(fast_sched_, gf)) { std::fprintf(stderr, "[fast_decode] sched alloc failed\n"); ggml_backend_sched_reset(fast_sched_); ggml_free(ctx0); return false; } - - ggml_backend_tensor_set(hidden0, hidden_in.data(), 0, hidden_in.size() * sizeof(float)); - ggml_backend_tensor_set(positions, pos_vals.data(), 0, pos_vals.size() * sizeof(int32_t)); - if (prefix_ids) { - ggml_backend_tensor_set(prefix_ids, prefix_tokens.data(), 0, prefix_tokens.size() * sizeof(int32_t)); - } - + + } + + ggml_backend_tensor_set(hidden0, hidden_in.data(), 0, hidden_in.size() * sizeof(float)); + ggml_backend_tensor_set(positions, pos_vals.data(), 0, pos_vals.size() * sizeof(int32_t)); + if (prefix_ids) { + ggml_backend_tensor_set(prefix_ids, prefix_tokens.data(), 0, prefix_tokens.size() * sizeof(int32_t)); + } + + if (is_cpu_only) { + if (ggml_backend_graph_compute(backend_cpu_, gf) != GGML_STATUS_SUCCESS) { + std::fprintf(stderr, "[fast_decode] cpu compute failed\n"); + ggml_free(ctx0); + return false; + } + } else { if (ggml_backend_sched_graph_compute(fast_sched_, gf) != GGML_STATUS_SUCCESS) { std::fprintf(stderr, "[fast_decode] sched compute failed\n"); ggml_backend_sched_reset(fast_sched_); ggml_free(ctx0); return false; } ggml_backend_sched_reset(fast_sched_); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/s2_model.cpp` around lines 1238 - 1289, Deduplicate the input tensor setup in the fast decode flow: remove the identical ggml_backend_tensor_set calls for hidden0, positions, and optional prefix_ids from both is_cpu_only branches, and place one shared block after the branch-specific graph allocation succeeds and before computation. Preserve the existing allocation, compute, cleanup, and failure behavior for both CPU and scheduled backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/s2_model.cpp`:
- Around line 1238-1289: Deduplicate the input tensor setup in the fast decode
flow: remove the identical ggml_backend_tensor_set calls for hidden0, positions,
and optional prefix_ids from both is_cpu_only branches, and place one shared
block after the branch-specific graph allocation succeeds and before
computation. Preserve the existing allocation, compute, cleanup, and failure
behavior for both CPU and scheduled backends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95e6695c-8e20-49e1-a5ed-373709072048
📒 Files selected for processing (2)
include/s2_model.hsrc/s2_model.cpp
- In the GPU path of fast_decode, ggml_backend_sched_reset() was called at the end of the else block, before ggml_backend_tensor_get() read the output logits. ..This released the scheduler's buffer ownership while the output tensor was still being read. - Moved the reset to after tensor_get, guarded by !is_cpu_only since the CPU gallocr path does not use the scheduler.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented ggml_gallocr_t with proper lifecycle management.
We check if we're running purely on CPU and use the cached codepath, or fallback to old behavior when any GPU backend is involved.
This effectively eliminates redundant allocation cycles in Fast-AR decoding loop for pure CPU configs.
Before/After PR test
CPU: AMD Ryzen 7 5700X3D (8C/16T, pinned to physical cores)
Model:
s2-pro-q8_0.gguf| Backend: CPU (-ngl 0) | Allocator: mimallocPrompt: "hello world, how are you today?" (~204 tokens prefill)
My build script:
My runtime setup:
Summary by CodeRabbit
Performance
Bug Fixes