llama: map each contiguous run of a context's tensors, not one span over all of them - #152
Open
danielhanchen wants to merge 1 commit into
Open
llama: map each contiguous run of a context's tensors, not one span over all of them#152danielhanchen wants to merge 1 commit into
danielhanchen wants to merge 1 commit into
Conversation
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. |
…ver all of them get_mapping_range returns a single [first, last) taken as the min and max offset of the tensors a context holds in one file, so it cannot express a gap. When a large tensor belonging to a different buffer type sits between them, that tensor is inside the mapped range. On a backend that copies per tensor this costs nothing, which is why it has gone unnoticed. On a backend that maps the host pointer, buffer_from_host_ptr is called on the whole range and the gap becomes resident for the run. Qwen3.8-Flash-Next puts a 26.8 GiB per_layer_token_embd, which is CPU resident, at tensor index 4 of 373 in its second shard. The GPU context's range then covers the entire 49.84 GiB shard for 20.4 GiB of its own weights, and on Metal all of it is wired. Map each contiguous run separately. Runs closer together than 32 MiB are merged, so a model whose tensors are laid out per buffer type still gets one buffer and one mapping, and only a genuinely large foreign tensor splits the range. If a layout somehow yields more than 64 runs, fall back to the single span rather than create a buffer per tensor. ggml_backend_tensor_alloc already asserts that the address lies inside the buffer it is given, so a tensor resolved against the wrong run aborts rather than mapping silently to the wrong place. Assisted-by: Claude
danielhanchen
force-pushed
the
mmap/per-run-buffers
branch
from
August 31, 2026 04:06
3cb3da7 to
258345e
Compare
![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: 258345efa6
ℹ️ 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".
Comment on lines
+247
to
+248
| // near-adjacent runs are merged, so a normal layout yields one range and only a large | ||
| // foreign tensor splits it |
There was a problem hiding this comment.
Join this declaration comment into one physical line; it splits a single sentence between lines 247-248, and the new explanation in src/llama-model.cpp does the same at lines 1730-1731, contrary to the repository's explicit comment-formatting rule.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
danielhanchen
added a commit
that referenced
this pull request
Aug 31, 2026
* unsloth: pin ggml-org#27941, #152 and #154 ggml-org#27941 at 02eb201, no longer a draft and MERGEABLE upstream. It fixes four qwen4exp correctness defects and is the only account of the Flash-Next reporter who is on Vulkan only, where the allocator variable is never set. The previous pin PR #148 carried the stale 8161d11 and is closed. #152 at 258345e and #154 at 31e432e both merge onto b10708 on their own. #137 is already pinned at 4e1865e, which is still its head, so no change. #142 and #144 are NOT pinned here: both predate an upstream refactor that folded lazy_mode and model_shared into a lazy struct, so they conflict on src/llama.cpp and src/llama-model-loader.cpp against b10708. b10708 has 'ml.lazy.mode = params.lazy_mode' where those branches still write 'ml.lazy_mode' and 'ml.model_shared'. They need rebasing onto current upstream before they can be pinned; pinning them now would fail the resolve. * unsloth: repin ggml-org#27941 to 6b2b85c The PR moved on at 10:44Z, after 02eb201 was pinned. Verified to merge onto the current base tag: 6 files changed, 480 insertions, 138 deletions. * unsloth: pin #144, rebased, and leave #142 out as contained in it #144 rebased onto b10709 at 6fc8df1. Two of its nine commits were dropped rather than carried: 'qwen4exp: fix QSA correctness defects and harden metadata loading' and the comment tidy on top of it. That work is what ggml-org#27941 supersedes, and keeping both copies is what made the two pins conflict in llama-kv-cells.h and llama-memory-hybrid-idx.cpp. Listed after ggml-org#27941 so the upstream version of that work lands first. #142 is deliberately NOT pinned: its single commit is byte-identical to #144's c7bd6f2 apart from the lazy API spelling, so #144 already contains it and pinning both would apply the same change twice. Verified: b10709 + ggml-org#27941 + #144 merges clean, 24 files, +612/-94. * unsloth: repin ggml-org#27754 to 949f7ef The PR moved on at 10:39Z; 5796547 was pinned by #159 earlier today. Verified to merge onto b10709: 44 files changed, 2673 insertions, 38 deletions. Every entry in the set is now at its PR's current head. --------- Co-authored-by: danielhanchen <elliegouldingstuff@gmail.com>
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.
get_mapping_rangereturns a single[first, last)computed as the min and max offset of the tensors a context owns in one file. A min/max cannot express a gap, so when a large tensor belonging to a different buffer type sits between them, that tensor is inside the mapped range.On a backend that copies tensor by tensor this costs nothing, which is why it has gone unnoticed. On a backend that maps the host pointer it does not:
buffer_from_host_ptris called on the whole range, and everything in the gap becomes resident for the life of the process.Four backends advertise the capability. CPU, BLAS and zendnn only wrap the pointer, so the range costs address space and nothing else. Metal creates real
MTLBufferobjects over it withnewBufferWithBytesNoCopy, counted againstrecommendedMaxWorkingSetSize, and pins them in a residency set on macOS 15 and later.Where it bites
Qwen3.8-Flash-Next places
per_layer_token_embd, 26.8 GiB and CPU resident, at tensor index 4 of 373 in its second shard. Every other tensor in that shard belongs to the GPU context, on both sides of it.Reproducible on Linux, because the CPU backend takes the same path and the PLE table lives in its own lazy context.
-ngl 0, UD-IQ1_S, and llama.cpp's own per buffer log:CPU_MappedbuffersThe shard 2 span splits into 347.70 + 19861.28 = 20208.98 MiB, and 47674.92 - 20208.98 = 27465.94 MiB, the PLE table to the byte. The model file is 69165 MiB, so the old span over maps it by 39.7 percent and the new one lands on it.
What the change does
Map each contiguous run of a context's tensors instead of one span over all of them.
Runs closer together than 32 MiB are merged, so a model whose tensors are laid out per buffer type still produces exactly one buffer and one mapping, and only a genuinely large foreign tensor splits the range. If some layout yields more than 64 runs, it falls back to the single span rather than create a buffer per tensor.
ggml_backend_tensor_allocalready asserts that the address lies inside the buffer it is given, so a tensor resolved against the wrong run aborts rather than mapping silently to the wrong place.Testing
Built at
9723942ad, CUDA, B200.llama-mtmd-cli, which is the only path that exercises the M-RoPE branch.test-llama-archsfor qwen4exp, llama, qwen3next, gemma3n and qwen3moe, checked for real result rows rather than exit code alone.git diff --stat -- ggml/is empty, sotest-backend-opscannot say anything this diff does not already guarantee.What is not tested here
There is no Apple hardware on this box. The Metal consequence is derived from the source plus the byte accounting above, and cross checked against the memory figures published for a third party GGUF of the same model that happens to isolate the table in its own shard. The buffer counts and sizes in the table are measured; the Metal residency saving is not.