Repository navigation
recipe: flash-next gets 128 graph programs, because a re-JIT costs 41 ms - #1006
Conversation
|
#1006 ready for review at 6921e64. |
|
Preparing review... |
… ms (engine #679) Qwen3.8-Flash-Next's decode graph needs 66 distinct programs and the HRX program cache holds 64, so at least one program is evicted and re-JITed for a whole run: 933 `trim_graph_programs ... (1 of 66 programs, ... 64 max)` lines in one 40-minute 18.8k-token server run, 779 in the pre-rebuild run of the same row, each trim evicting one program and freeing ~6 MiB (so the 8192 MiB byte budget is not the constraint - the arenas sit at ~2.4 GiB). What a re-JIT costs was never measured, and item (a) for #679 was ranked on intuition because of it. Measured now on this box by timing the first graph_compute of a fresh graph against the immediately following one (the second is served from the program cache), same graph, same leaves: 1 GDN layer 44 nodes first 41.9 ms second 0.94 ms -> 40.9 ms 12 layers 528 nodes first 52.0 second 6.04 -> 46.0 ms 36 layers 1584 nodes first 58.4 second 15.68 -> 42.7 ms 36 layers, cache=1, twice 56.8 / 59.6 second 15.8/18.5 -> 41.0/41.1 ms Flat in graph size (compile+load, not per-command emission), and 3-4x the whole steady decode of a 36-layer GDN graph (p50 0.65 / 5.39 / 15.29 ms for those same graphs). In a cold process the same first compute is 110-136 ms; 41 ms is the warm figure. The recipe is scoped to hrx + qwen4exp and is not a default change, because the tok/s effect at 18.8k is NOT measured: that row needs the 76.33 GiB file with the box to itself (load alone measured at >= 109 GiB of GTT, against a 120 GiB ceiling and #680's 106.5 GiB reset regime), and the box has carried other tenants' serves for five days. So the trims' per-step cost has a ceiling, not a value: ~4 ms/step at the observed ~1 trim per 10 decode steps, up to 41 ms/step if it were 1 per step. tests/recipes_route.sh gains the case for it: hrx-scoped (nothing on cpu), 128 and named on hrx for a qwen4exp file, nothing for another hrx architecture, and the suite's run helper now unsets GGML_HRX_GRAPH_PROGRAM_CACHE so an ambient value cannot mask the recipe. docs/recipes.md: the built-in table.
6921e64 to
e431f2d
Compare
|
Rebased onto current main |
PR Reviewer Guide 🔍(Review updated until commit 1c95546)Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
Built this head (cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc-16 -DCMAKE_CXX_COMPILER=g++-16, target onebit, 32/32, 1bit links) and ran the PR's own test on it:
tests/recipes_route.sh build/1bit -> 19 ok / 0 FAIL, PASS, rc 0 — including both new hrx arms.
The two claims re-derived from primary sources, not from the body:
- 933 trims,
66 programs ... 64 max: the pre-rebuild log is on this box,~/lb/aval/tattn-18k-topk/server.log
->grep -c trim_graph_programs= 779, first line verbatim:
trim_graph_programs: HRX graph program cache trimmed (1 of 66 programs, 4212 -> 4206 MiB of arenas, budget 8192 MiB, 64 max): kept the current one, trim 1.
So it is one program per trim, ~6 MiB freed (4212 -> 4206), and the arena figure sits far under the 8192 MiB byte
budget — the count limit is the binding one, exactly as thewhystring says. The 933 figure is the cell's
post-rebuild run (TATTN-STATE 2026-11-04,~/lb/aval/...window). - the arithmetic in the
whystring: 933 trims in 40 min at ~4.1 tok/s is ~9.8k steps, i.e. one trim per ~10.5
decode steps, and 0.105 x 41 ms = 4.3 ms/step — the "~4 ms/step at ~1 trim per 10 steps, ~41 ms if it were 1
per step" pair is self-consistent.
Config and docs: config/recipes.json has 4 entries; the new one is the only one with env and no args, scoped
to {"device":["hrx"],"architecture":["qwen4exp"]}, so it cannot leak to qwen35 or to cpu — and the test pins
both directions (nothing on cpu, nothing for plainq35 on hrx). docs/recipes.md now says "Four" and lists four
rows. Not measured, and the recipe string says so plainly: the tok/s effect (the 76.33 GiB file needs the box).
One nit, and it is about the test rather than the recipe:
The positive arms are gated on the HSA library existing ([ -e "$hsalib" ]), but they do not need it: the backend
is a stub and the recipe routes on the device name, so my plain non-HRX build — no HRX backend at all — ran both
hrx checks green. On a CI runner without TheRock those two checks are therefore skipped for no reason, which is the
one place this test's coverage is thinner than it looks. Dropping the guard (or replacing it with a command -v on
nothing at all) would let CI run them.
|
Persistent review updated to latest commit 7112228 |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7112228a4 — merge-only update, own content byte-identical.
Checked with a hash of the PR's own diff at both heads rather than by eyeballing the file list: git diff <merge-base> <head> | sha256sum is the same at e431f2d5b and at 7112228a4, so nothing of this PR's moved. The branch is MERGEABLE/BEHIND; the whole delta is main advancing (#1007, #1002, and the console-TUI/prefix-cache work that landed with them).
Evidence from the review at e431f2d5b stands: the hrx-graph-program-cache-128 recipe — built and tests/recipes_route.sh 19 ok / 0 FAIL, both hrx arms included.
CI at this head: red/pending: build — pr_agent failures on engine PRs are superseded-run cancellations ("Canceling since a higher priority waiting request exists"), not findings.
|
Persistent review updated to latest commit 614ec08 |
|
Persistent review updated to latest commit 5490a2b |
|
Persistent review updated to latest commit 1c95546 |
What this changes
One built-in recipe,
hrx-graph-program-cache-128: on thehrxdevice with theqwen4exparchitecture(Qwen3.8-Flash-Next),
1bit servesetsGGML_HRX_GRAPH_PROGRAM_CACHE=128instead of leaving it at the default 64.Plus the docs table row and a
tests/recipes_route.shcase that it fires onqwen4expand on nothing else.Root cause, in two measured parts
This model needs more programs than the cache holds. Flash-Next's own server log at
-c 19456, 18.8k-token row,40-minute run: 933
trim_graph_programs: HRX graph program cache trimmed (1 of 66 programs, ... 64 max)lines(and 779 in the pre-rebuild run of the same row). Each trim evicts exactly one program and frees ~6 MiB, so the
8192 MiB byte budget is nowhere near binding - the arenas sit at ~2.4 GiB - and the count limit is the whole story.
A program that is not resident costs ~41 ms. Measured on this box on 2026-11-04 by timing the first
graph_computeof a fresh graph against the immediately following one (the second is served from the program cache),same graph, same leaves:
The price is flat in graph size - it is compile+load overhead, not per-command emission - and it is 3-4x the
whole steady decode of a 36-layer GDN graph. (In a cold process the same first compute costs 110-136 ms; 41 ms is the
warm figure, verified by the steady p50 in those same runs moving with the clocks, not with the JIT.)
What is NOT measured, and why this is a recipe and not a default change
The tok/s row at 18.8k. It needs the 76.33 GiB file with the box to itself: the load alone was measured at >= 109
GiB of GTT (engine #680's reset regime starts at 106.5 GiB, the GTT ceiling here is 120 GiB), and this box has been
held by other tenants' serves (32-40 GiB) for five days, so the row has been blocked since 2026-10-31. What the trims
cost per step therefore has a ceiling, not a value: at the observed rate of ~1 trim per 10 decode steps it is ~4
ms/step; if trims were 1 per step it would be the whole ~41 ms. The recipe claims the mechanism and the price of the
thing it removes, not a token rate - and it is scoped to this architecture rather than made the default, because the
opposite-direction evidence (a 66-program-class model at cache 128 logging 0 trims, i.e. the knob is a no-op below the
limit) is exactly why a global default is not justified yet.
Raising the count does not preallocate: the byte budget still caps the cache at 8192 MiB, and 128 slots of ~6 MiB are
~768 MiB against a 120 GiB GTT budget.
Testing
tests/recipes_route.sh(ctestrecipes_route, no GPU), 19/19 checks pass: the built-in set setsGGML_HRX_GRAPH_PROGRAM_CACHE=128on aqwen4expfile served with--device hrxand names the recipe, is silent oncpu (it is an HRX-only variable), sets nothing for the other architectures on either device, and leaves another
hrxarchitecture alone. The positive case is skipped (not failed) when the HRX runtime library is absent, sinceservehas to resolve the device; the model itself never loads - the backend is the suite's stub. The suite'srunhelper also unsets
GGML_HRX_GRAPH_PROGRAM_CACHE, so an ambient value cannot mask the recipe.ctest -R recipes_routein the build dir: Passed (2.02 s) - this is the test CI runs (ci.ymlruns the wholectestsuite, andrecipes_routeis registered atCMakeLists.txt:147against theonebittarget).tools/copyright.py --checkclean.docs/recipes.md: the built-in table gains the row and the sentence that said "three, allqwen35" now says four,naming this one as the only environment setting.
Refs engine #679 (the long-context decode issue). Does not close it: the 18.8k token-rate row is still the missing
number, and the recipe says so in its own
measuredfield.