Repository navigation
hrx: the byte bound's eviction order is selectable, and the trim line says which order ran (engine #841) - #230
Conversation
… says which one ran `plan_evictions` took the oldest program first no matter which bound tripped. When the *byte* bound is what trips, age order can free almost nothing: the cached programs are skewed, so the oldest one is often small and the trim repeats on the next shape. `GGML_HRX_GRAPH_PROGRAM_EVICT=size` sorts the byte-bound victims largest first; the count bound stays oldest-first, because "how many shapes do I keep" is the question age answers. Unset or unparsable is age, i.e. the shipped behaviour, so this patch changes no default. The trim line now carries `evict=<policy> builds=N hits=M`. The policy-question row (engine ggml-org#841) is decided by counting rebuilds per arm, and a trim line that does not name its own order is the same trap as a build that does not say whether it has the addons. builds/hits are the cache's lifetime counters, which do not go backwards when a program is evicted. Host test covers: largest-first under the byte bound, count bound still age under the same env, equal sizes fall back to age (deterministic), unparsable value is age, and the name the log prints is the order that ran.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7197ff1b5 — the host test builds and passes here without a device or an HRX link, the default is provably unchanged, and the log line carries the two counters the body says it needs. One claim in the body is not true when both bounds trip at once, and I have the three-line demonstration.
The test, compiled standalone on this box (g++-16 -std=c++20 tests/test-hrx-graph-program-budget.cpp runtime/graph-program-budget.cpp, no HRX, no device):
test-hrx-graph-program-budget: all checks passed rc 0
and the arms it grew are the right ones: largest-first under the byte bound, equal sizes falling back to age (so the order stays deterministic), unset and unparsable both age, and the count bound answered by age under the same env. The policy name and the trim line are checked against what actually ran, which is the point of the PR — a trim line that does not say which order produced it cannot be A/B'd.
The default really does not move: evict_largest_first() is getenv(...) == "size" read per call, and the sort is inside if (over_bytes && evict_largest_first()), so with the variable unset (or set to anything else) plan_evictions runs the same code it ran before.
The log line prints the counters that survive eviction, which is the trap this PR explicitly avoids: GraphProgramCacheStats carries both builds/hits and prepared_program_builds/prepared_program_hits, and the diff takes the former. Since evicting a program clears the prepared-program fields, printing those would have made every trim look like a regression in the A/B; the lifetime counters are the right instrument.
The nit — "the count bound keeps oldest-first" is only true while the byte bound is not also over. With the same three entries (1, 2, 32 MiB) I ran the three cases:
size policy, byte bound only (budget 4 MiB, max 0): -> [3] largest first, as intended
size policy, BOTH bounds over (budget 4 MiB, max 1): -> [3, 2] largest first, count bound included
age policy, BOTH bounds over (budget 4 MiB, max 1): -> [1, 2, 3] oldest first
So when a single trim has to satisfy both bounds, the size order drives the count bound's victims too — the stable_sort rewrites the one list the loop walks. That may well be the behaviour you want (a byte-bound trip is the trigger, and dropping the biggest first satisfies both sooner), but the body says, unqualified, that "the count bound keeps oldest-first", and the test's count arm only covers the case where the byte bound is not tripped (plan(ties, 0, 1, 0, false)). Two ways to close it: qualify the sentence ("while the byte bound is not tripped"), or add the both-over case to the test so the behaviour is pinned whichever way you decide.
Not re-run: the measurement the PR exists for — the three sweeps of zaya1-8b-Q4_K_M with GGML_HRX_GRAPH_PROGRAM_CACHE=0, the trims/dropped/builds/hits table and the per-arm GTT from /proc/<pid>/fdinfo. That needs GGML_HRX=ON and a GPU window; the host test and the code path are what I could take, and they are green.
Refs engine#841, no close: this is the eviction-order half, and the issue's own row stays open.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7197ff1b.
Built and ran the host test this PR ships (HRX-configured Release build, thermal-run, no device opened):
./bin/test-hrx-graph-program-budget "all checks passed" rc 0
ctest -R graph-program-budget 1/1 Passed 0.01 s
and the cases are the ones the body claims, including the one I would have asked for — a plausible wrong value:
GGML_HRX_GRAPH_PROGRAM_EVICT=size -> largest-first under the byte bound, count bound still age
(unset) -> age
GGML_HRX_GRAPH_PROGRAM_EVICT=largest -> age <- the typo case, and the log name agrees
graph_program_evict_policy_name() -> "age" / "size" / "age"
So "unset/unparsable is the shipped behaviour" is true in the code (strcmp(policy, "size") == 0, nothing else) and tested,
and the name the trim line prints is the order that actually ran rather than a second place to drift.
The measurement is reported as a shape, not a winner, and that is the right call. Age's rebuild count is flat in the budget
(121 → 121 → 118 for 128 → 256 MiB) while size's falls monotonically (194 → 132 → 88), and the body says outright that below
~192 MiB the ordering inverts and age is better — so size is not a free win, the control arm sizes the working set at 1088 MiB
above load, and the default stays age on purpose. A PR that publishes the regime where its own lever loses is the kind that
makes the next person's decision cheap.
One thing to know about where this test runs, not a defect: it is registered inside if (TARGET ggml-hrx), and the fork's
hosted CI never configures HRX (grep GGML_HRX .github/workflows = 0 hits), so it runs on HRX-configured builds only — the
engine ggml-org#1077 gap, which the body acknowledges by saying "compiled standalone against this tree". Worth keeping in mind when
reading the green CI badge on this PR: build passing does not mean this test ran.
The runner's refusal to start unless the trim-line format string is present in libggml-hrx.so.0 is a nice touch — it makes
the "plausible-looking unpatched build" impossible to mistake for a patched one, which is the same discipline as the evict= field
itself: a measurement line that cannot say which configuration produced it is not evidence.
Refs 1bit-MONSTER/engine#841 (the policy row, not a closing fix).
Root cause of the hole this closes
GraphProgramBudget::plan_evictionsfed the byte-bound victims out of the same oldest-first order as the countbound. Age is the right order for "how many shapes do I keep", but for a byte bound it is the wrong one whenever
the cache is a revisit cycle (
A B C A B C): the program it evicts is the one it is about to need again, so the trimreturns almost nothing per program (measured: 0-1 MiB freed per program in the age arm below) and the next shape
re-trims. Nobody could see any of that from the log either, because the trim line did not say which order produced it.
What the patch does
GGML_HRX_GRAPH_PROGRAM_EVICT=sizesorts the byte-bound victims largest-first; the count bound keepsoldest-first. Unset/unparsable is
age, i.e. the shipped behaviour, so this patch changes no default.evict=<age|size> builds=<N> hits=<M>;builds/hitsare the cache's lifetime counters(they do not go backwards when a program is evicted, unlike the prepared-program fields) — which is what makes an
A/B out of this decidable by counting rather than by timing.
tests/test-hrx-graph-program-budget.cpp, no device, no HRX link): largest-first under the byte bound,count bound still age under the same env, equal sizes fall back to age, unparsable value is age, and the name the
log prints is the order that actually ran.
Measured, same box, same binary, 3 sweeps (counters identical in all three)
zaya1-8b-Q4_K_M,-ngl 99 -fit off -c 4096 -np 1, count bound off (GGML_HRX_GRAPH_PROGRAM_CACHE=0) so only thebyte bound can trim, three shapes (24/96/192 words = 111/471/1043 evaluated tokens) x three passes,
cache_prompt=false,fresh server per arm, own GTT from
/proc/<pid>/fdinfo:Read it as a shape, not a winner: age's rebuild count is flat in the budget (121, 121, 118 for 128->256 MiB, i.e.
triple the budget buys three rebuilds) while size's falls monotonically (194 -> 132 -> 88). That is the LRU cycle
pathology, measured: extra bytes cannot help an order that evicts the next-needed program, and largest-first turns the
same bytes into fewer rebuilds. Below ~192 MiB the ordering inverts (age better), so largest-first is the better order
only when the bound is a fraction of the working set; the control arm sizes that working set at 1088 MiB above load.
The shipped default is therefore left as
ageon purpose: this PR is the lever and the observable, not a default flip.Verification
test-hrx-graph-program-budget: all checks passed(compiled standalone against this tree, no device opened).libggml-hrx.so.0(so a plausible-looking unpatched build cannot produce the rows above).gpu-hold; peak own-GTT 7.1 GiB,MemoryMax=24G.