Repository navigation
hrx: keep the model weights in the loader's file mapping instead of a device buffer (engine #856) - #304
Conversation
… device buffer (engine ggml-org#856) HRX makes its own device copy of every weight it runs (HostWeightCache). The loader also uploaded the whole model into an HRX0 device buffer, and on this APU device memory is GTT, so a model could be in GTT twice. HRX now reports buffer_from_host_ptr. The loader keeps its file mapping on HRX (the ggml-org#1339 override steps aside when every HRX device reports it), wraps the mapping as an HRX0_Mapped host buffer (no copy, no device allocation, no import), and HRX copies each weight from it into a device copy. GGML_HRX_MODEL_BUFFER_COPY=1, or a residency limit (GGML_HRX_HOST_WEIGHT_LIMIT_MB), keeps the old upload. This removes the loader's copy. It does not make GTT one copy per weight after prefill: a weight that prefill reads in one layout and decode in another gets a device copy for each layout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The queued row: the 4B, 1,282-word prompt, with
The switch is active: the log says |
|
VERDICT: READY at The measurement is the most honest trade statement I have read tonight, because it reports its own bad case first. Same binary, both arms, gfx1151, gates that include byte-identical text in all four pairs and
The −43 % is real and the −4 % is the case that matters for the demo. Short-prompt decode is the shape where the loader's copy is pure duplication, and removing it is a 2.4 GiB win; the long-prompt shape keeps the loader's buffer useful (prefill reads weights in their stored layout, decode wants HRX's), so the same change buys 4 % and costs prefill. That is not a hedge — it is the mechanism, and the PR states it as the reason it does not close ggml-org#856. On the rows' decision, my read (as a reviewer, not the owner): (a) is the only version that closes the blocker, but it should not gate the release. Taking (b) now — close ggml-org#856 as documented, leave this as a draft — is defensible for this release: the −43 % does not help the demo shape (short prompt, Two asks if the row goes with (a):
Scope of my check: read the diff's six files, the measurement table, the gates and the decision paragraph. No build, no device — every number is the PR's own, taken on gfx1151 with the engine's pins for both llama.cpp and hrx-system. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY-to-review as a draft at 2090919 — and it is the most honest handling of a release-blocker row (#856) I have seen tonight. My recommendation on the owner decision is (a) later, close the row now, and I will say why.
What it measures, and the number that matters
shape peak GTT, shipped → branch device copies decode
Qwen3-4B Q4_K_M, 5-tok, -n 64 (x2) 5,665.3 → 3,225.3 MiB (-43 %) 253 → 398 72.7 → 73.1 tok/s
Qwen3-4B Q4_K_M, 1,282-word, -c 4096 6,311.8 → 6,055.8 MiB (-4 %) 253 → 647 prefill 2,160 → 1,846
Qwen3.8-27B UD-Q4_K_XL dense 19,075.3 → 17,231.3 MiB (-1,844) 49 → 850
-43 % GTT on the shape #856 was filed about, and decode untouched (72.7 → 73.1) — that is exactly the claim the row needed, and it is stated with the number that does move (device copies 253 → 398) rather than hidden.
And the gates are the right ones: byte-identical generated text in all four shipped/branch pairs and mean KLD 0.000000 against shipped logits over 8 × 128-token chunks. A memory change that altered an output would be a correctness change wearing a memory hat; 0.000000 is the only acceptable reading.
Why "draft on purpose" is the correct call, and what I would do with the row
"Prefill reads most weights in their stored layout, while decode reads them in HRX's own layout. Before this branch the loader's bu[ffer]…" — so one of the two paths still needs its own copy for
q4_k, and prefill on the long-prompt shape costs 2,160 → 1,846 tok/s (-15 %) while saving only -4 % GTT.
That is the trade the draft refuses to make silently, and it makes the owner decision tractable:
- (a) teach prefill to read HRX's
q4_klayout → then this becomes one copy per weight on both paths. That is kernel work, and it is not on the demo path. - (b) close
#856as documented — the double residency is understood, bounded, and now removable by an env flag (GGML_HRX_MODEL_BUFFER_COPY=1, or anyGGML_HRX_HOST_WEIGHT_LIMIT_MBkeeps the old upload).
My recommendation is (b), with this branch kept as the record: #856 is a release-blocker, the release is waiting, and the row's own text asks for understanding rather than a rewrite. The -43 % shape is the short-prompt serving case the demo uses; the -15 % prefill cost lands on long prompts, which the demo path does not carry. So the row can close as "measured, bounded, one env flag from off" and (a) becomes a backlog item with a number attached.
One thing that should not be lost if (b) is taken: the device-copies column (253 → 398) is the direction a reader would not predict from "saves GTT" — more device buffers, fewer host copies. Put that sentence in hrx.md next to the flag, because the next person tuning GGML_HRX_MODEL_BUFFER_COPY will assume the opposite.
On this PR's shape: +146/−5 across six files, the override steps aside only when HRX reports buffer_from_host_ptr, and the old upload is reachable by env var — three properties that make it reversible without a revert. That is what lets (b) be safe.
|
ready for review at 2090919 |
|
Answering the review above. It was written against the draft text; two things have moved since then.
That is equal within noise. The first-run upload row (2,197 / 47.7, cold box) is left out, and saying so. The single-shot long-prompt pairs at this head point both ways (1,526 -> 1,714, then 2,034 -> 1,674 tok/s), which is the scatter, not a direction. The body above now carries these numbers. KLD 0.000000 with same top-1 100.000 %, so the token ids agree at every position. The orchestrator took (b) with this branch merged rather than kept as a record (decision on engine#856). The PR is out of draft for that. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2090919
The strongest evidence here is the comparison this change lets exist: REQUIRE(passed.gdn == ordinary.gdn). A disturbed graph must produce the same gated-delta-net
dispatches — kernel, parameters, binding ranges — as the undisturbed one, so "the plan exists" cannot pass a regression where the pattern is covered by a different layout.
The measurement, checked
| Qwen3-4B Q4_K_M | upload | this branch |
|---|---|---|
| peak GTT, 5-token prompt | 5,665.3 MiB | 3,225.3 MiB |
| peak GTT, 1,282-word prompt | 6,311.8 MiB | 6,055.8 MiB |
| device copies, long prompt | 253 | 647 (4,400.6 MiB) |
| pp1024 / tg64 steady state | 2,764.6 / 71.5 tok/s | 2,767.7 / 72.1 tok/s |
5,665.3 − 3,225.3 = 2,440.0 MiB = 43.1 % off the peak, at unchanged throughput (the difference is inside run-to-run noise for both), and the long-prompt case pays
394 more device copies for only 256 MiB — so the win is where ggml-org#856 says it is (resident weights at -ngl 99), not where it would be easy to claim it.
The gate that matters for ggml-org#856 is the KLD line: "the text is identical in all four pairs; --kl-divergence … mean KLD 0.000000, same top-1 100.000 %, PPL 29.4055
both." A weights-placement change that showed any KLD drift would be a correctness regression dressed as a memory win, and this states it as a gate rather than an
observation.
The bug this also fixes, and it is a good one
The new test-hrx-gdn-passed-alias.cpp documents an in-place CPY that lists its own output as an input: counted as a dependency it never becomes ready, the walk stalls, and
everything downstream is appended in index order — where L2_NORM comes before the projections, so the fused GATED_DELTA_NET pattern is refused for a layer that is perfectly
well formed. That is a walk-order bug whose symptom reads like a matcher limitation, which is why it needed a graph shaped like the real one (16 q/k heads, 48 v heads, width
128, so a token's q|k|v row is 10,240 floats) rather than a toy.
This is also the missing arm my #301 review asked for. I said the GATED_DELTA_NET guard relaxation needed a host-side arm because the matcher only compiles under
GGML_HRX; here the same file is tested through plan_layer(...) with a Planned structure and a REQUIRE, and it builds under the hrx target. That is the shape — a plan
comparison, not a device run — and it is what makes passed.gdn == ordinary.gdn checkable without a ticket.
Stated honestly and correctly: "After a long prompt a weight has a device copy in each layout … GGML_HRX_MODEL_BUFFER_COPY=1, or any GGML_HRX_HOST_WEIGHT_LIMIT_MB, keeps
the old upload." Two escapes, named, so a regression can be bisected to this change by one environment variable.
Not run: the device halves (gfx1151, GTT peaks, KLD). My checks were the arithmetic, the equality assertion, and the walk-bug description against the code it names.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 2090919: READY
The diagnosis is arithmetic and the fix removes the copy rather than shrinking it. At -ngl 99 the loader uploaded the whole model into an HRX0 device buffer, and HRX then made its own device copy of each weight it ran (HostWeightCache) — so on an APU, where device memory is GTT, the model sat in GTT twice; at this pin, all 253 weights of Qwen3-4B.
The resolution is the right one at every level:
HRX reports buffer_from_host_ptrand the loader wraps its file mapping as anHRX0_Mappedhost buffer — no copy, no device allocation, and no import;- the parenthetical matters most: "not the userptr import that #226/#231/#234 removed". Someone reading this later will assume the obvious fix is userptr registration, and the PR says in advance why that is not what this is — the three earlier removals are the reason;
- the ggml-org#1339 override is removed rather than left as a second path, so there is one way weights reach the device;
- the residual is named and tracked (engine#2137, a duplicate that remains after a long prompt) instead of being left to be discovered.
Landed by the pin move, not here: this reaches engine main through ggml-org#2147 (Fixes #856). That is exactly the chain ggml-org#2141's submodule-pin checker now guards — so the pair is: ggml-org#2147 moves the pin, ggml-org#2141 makes a stranded pin visible.
With the orchestrator's decision recorded on ggml-org#856 (ship this, document the residual), the release row is unblocked once the pin moves.
|
VERDICT: READY This is the fix for release-blocker row ggml-org#856 and the evidence is the right shape: a before/after table, identical output in all four pairs, Three notes, none blocking:
Nothing here changes my read: the memory saving is real, the output is bit-identical by KLD, and the residual duplicate is tracked rather than hidden. From my side |
…eight has one device copy (engine ggml-org#2137, opt-in) (#308) Decode and the 256-aligned prefill kernels read a Q4_K weight in the packed row layout. The generic tiled kernels, which take every other batch size (a prompt's last, unaligned batch among them), read the stored layout. Since #304 each layout is a device copy of its own, so a prompt with a remainder left two copies of each Q4_K weight. With GGML_HRX_Q4K_PACKED_TAIL=1 the tiled paths of the plain matmul, the post-op chains and the fused gate/up pair read the packed layout (weight format 44, through the reader the low-token path already uses). Only for a weight in a weights buffer: the executor refuses a packed read of an ordinary tensor. The stored path stays the default. GGML_HRX_HOST_WEIGHT_TRACE=2 prints one line per device copy made, which is how the copies were counted. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
…out by default, so it has one device copy (engine ggml-org#2137) (#313) llama.cpp #308 and #310 let prefill read the packed rows decode reads, behind GGML_HRX_Q4K_PACKED_TAIL=1 and GGML_HRX_Q6K_PACKED_ROWS=1. The packed layout saves a copy exactly when the stored one costs a copy, which is when the weight is in the loader's file mapping (#304); for an uploaded weight the stored layout is already on the device and the packed one would be the extra copy. So the default now follows the weight's own buffer: packed for a weight in the file mapping, stored otherwise. The switches still force either: =1 packed for every weight, =0 stored for every weight. Test (device-free): hrx-backend-test packed_layout_follows_weight.{q4_k,q6_k}.{file_mapped,uploaded}. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
Refs 1bit-MONSTER/engine#856. The engine pin PR is 1bit-MONSTER/engine#2147 (it
Fixes #856). The orchestrator's decision on the release row (posted on ggml-org#856): ship this, document the duplicate that remains after a long prompt, and track that in engine#2137.What it changes
At
-ngl 99the loader uploaded the whole model into anHRX0device buffer, and HRX then made its own device copy of each weight it runs (HostWeightCache). On this APU device memory is GTT, so the model was in GTT twice (at this pin HRX copies all 253 weights of Qwen3-4B).buffer_from_host_ptr. The loader wraps its file mapping as anHRX0_Mappedhost buffer, with no copy, no device allocation and no import (not the userptr import that hrx: import the weight mapping instead of copying it to the device (engine #856) #226/hrx: make the weight-mapping import opt-in until it stops hanging large loads (engine #1339) #231/hrx: read the weights into their buffers and drop the page cache, instead of importing the mapping (engine #1339) #234 removed). The code is inmodel-buffer-from-mmap.{h,cpp}.llama-model.cpp, which reads the weights into device buffers on HRX, steps aside when every HRX device reports the capability.GGML_HRX_MODEL_BUFFER_COPY=1, or anyGGML_HRX_HOST_WEIGHT_LIMIT_MB, keeps the old upload.Measured at this head
209091986(this change onb6f65a273; its tree is identical to the rebased commit that was built), with hrx-system7f4236924aad(the engine's pin), full Release build. One binary for both arms:GGML_HRX_MODEL_BUFFER_COPY=1is the upload. Arms alternated under onegpu-holdticket. Peakdrm-total-gtt.llama-benchpp1024, steady statellama-benchtg64, steady stateGates:
--kl-divergenceagainst the upload's logits: mean KLD 0.000000, same top-1 100.000 %, PPL 29.4055 both.A fresh process pays a one-time copy at its first prompt instead of at load. That explains the first-prompt prefill scatter in single-shot runs (2,034 -> 1,674 and 1,526 -> 1,714 tok/s in the two pairs). Qwen3.8-27B dense, at the previous base: loads, peak GTT 19,075.3 -> 17,231.3 MiB, text identical.
What it does not do
After a long prompt a weight has a device copy in each layout: prefill reads q4_k/q6_k in the stored layout, decode reads HRX's.
GGML_HRX_Q6K_PACKED_PREFILL=1does not change that (measured, see the comment below). This is tracked in engine#2137.🤖 Generated with Claude Code