refactor(sdk): split geniex-bench into per-concern modules - #1378
Merged
RemiliaForever (RemiliaForever) merged 7 commits intoAug 28, 2026
Conversation
Pure code movement, no behavior change: benchmark.c had grown to 2174 lines mixing argv parsing, model-manager resolution, run loops, aggregation and report writing. Every function body moves verbatim — bench.h carries the type definitions and a map of which module owns what. The only edits to moved code are the two globals that now cross a module boundary: g_token_callback_delay_us gets a set_token_callback_delay_us() setter for parse_args to call, and main()'s g_mm_inited teardown becomes mm_shutdown(). Signatures that lost `static` are re-wrapped by clang-format. Signed-off-by: RemiliaForever <remilia@koumakan.cc>
RemiliaForever (RemiliaForever)
force-pushed
the
refactor/split-bench-modules
branch
from
August 27, 2026 11:48
468f715 to
bf2c3ce
Compare
Behaviour-preserving cleanup of the modules split out in the previous
commit. The monolith had grown three near-identical copies of several
blocks; splitting it into files made them visible side by side.
Extracted:
run.c build_prompt_list() — the "---" segment splitter, duplicated
verbatim in run_llm and run_vlm (~45 lines
each). run_vlm's copy had already lost the
explanatory comments of the original.
make_random_tokens() — vocab/BOS lookup + rand() fill, duplicated
in run_llm and run_logits with two
divergent error messages; the differing
tail is now a `hint` argument.
record_run() — the 14-field ProfileData -> run_result_t
copy, byte-identical in both run loops.
resolve.c mm_init_once() — the lazy geniex_model_init guard, copied
into both resolvers.
mm_resolve() — get_paths -> pull -> get_paths, ~37 lines
duplicated between resolve_via_mm and
resolve_draft_via_mm. `kind` supplies the
"draft " prefix on the progress line.
free_mm_paths() — the four-pointer release block, inlined
twice in run_one_cell.
report.c json_agg_stat() — three fprintf calls differing only in the
key. The key is padded to a fixed 15-column
width, which reproduces the original
hand-aligned output byte for byte.
options.c require_min() — the >=1 flag check, written out six times.
bench.h cell_name() — the `cell_id ? cell_id : "cell"` fallback,
repeated at five call sites across two
files. Small enough to stay inline.
Also drops report.c's json_field_dbl(), which had no callers and was kept
alive only by a `(void)json_field_dbl;` suppression.
Deliberately left alone: run_llm and run_vlm keep separate outer loops.
They differ in four real ways (create-input type, per-run chat templating,
the reset condition, the spec-decode log) and folding them would need a
callback struct that costs more than the ~15 lines it saves. The
per-call-site SDK error messages also stay as they are: each carries
distinct context (run index, operation name) that a shared helper would
flatten.
Verified byte-identical output against the pre-refactor binary on
Linux/CPU + llama_cpp across single-cell, --prompt-file with "---"
segments, --logits, and matrix mode (JSON, Markdown, stdout and stderr
all match once timings are normalised), plus all nine argv-validation
error paths.
Signed-off-by: RemiliaForever <remilia@koumakan.cc>
Interface-level cleanup of bench.h. Report structure (JSON schema, Markdown row, stdout lines) and CLI flags are unchanged. run_result_t dropped is_warmup / status / err[256]. After record_run() was extracted, the last `r->status = 0` went with it and all three fields had zero readers and zero writers. They were worse than dead weight: the header advertised a per-run error protocol that does not exist, so a reader would reasonably write `if (r->status != 0)` and it could never fire. Every failure path exits the process; the struct now says so. agg_t's 18 flat doubles became three stat_t. The old names were also inconsistent — ttft_ms_med carried a unit suffix, prefill_med and decode_med did not. The struct existed three times over, so naming it pays twice: json_agg_stat() drops from six parameters to three, and stats.c loses summarize_full() entirely (summarize() now returns the whole stat_t, and the median-only metrics just take .med). --token-callback-delay-us moved into options_t. It was the only flag that bypassed the struct, reaching run.c through a setter and a file-static. That made it silently un-per-cell in matrix mode, where `cell = *base` carries every other flag, and nothing said so. The global existed only because an available channel was being wasted: geniex.h has carried `void* user_data` next to `on_token` all along (geniex.h:117, :543), and bench discarded it with `(void)user_data`. The callback now reads the delay from the options it is handed. write_json / write_md_row / write_logits_json return 0/1 instead of calling exit(1) on a fopen failure. This one was a real defect, not a style point: the resolvers and run_one_cell already return error codes so that matrix mode can count a failed cell and continue, but an unwritable report bypassed that and killed the process. Observed with two cells and a bad --output-json-dir: the sweep loaded the model, ran cell 1, printed `[ok ] c1`, then died — cell 2 never ran, and stdout still claimed success for cell 1. The sweep now runs both cells and counts two failures. Single-cell mode still exits non-zero. aggregate() takes int32_t rather than int, matching everything else. Verified against the pre-change binary on Linux/CPU + llama_cpp across single-cell, --prompt-file with "---" segments, --logits, --accuracy, matrix mode, all argv-validation paths and --help: JSON, Markdown, stdout and bench's own stderr lines are identical once timings are normalised. The one stderr delta is the SDK's own TRACE dump of the input struct, which now prints `user_data: 0x...` instead of `user_data: nullptr` — that field being populated is the point. Still deliberately unaddressed, both flagged for follow-up: options_t mixes argv input, resolver-written derived state and output destinations, which is why `const options_t*` means "this phase does not mutate" rather than "this is input"; and (device_id, ngl) travels as a pair through six of the exported functions while write_md_row takes only one of them. Signed-off-by: RemiliaForever <remilia@koumakan.cc>
Two groupings, both encoding a fact that was previously convention. device_t holds the (device_id, ngl) pair. It was threaded separately through six of the exported functions, and write_md_row took only ngl — almost consistent, which reads worse than either extreme. The two values are produced in exactly one place (geniex_resolve_device in run_one_cell, after the --device-id / --n-gpu-layers overrides) and consumed together everywhere else, so they are now one argument and write_md_row matches the rest. options_t's mm_* fields became a named `mm` sub-struct. The struct held three different kinds of thing side by side: argv input, resolver-written derived state, and output destinations. That is why `const options_t*` only ever meant "this phase does not mutate" rather than "this is input" — the reader could not tell which fields were still liable to change. Nesting the resolver-written group puts the boundary in the type instead of a comment, and options_t stays one flat copyable struct so matrix mode's `cell = *base` is untouched. Report structure and CLI flags unchanged; verified identical across single-cell, --prompt-file segments, --logits, --accuracy, matrix mode, the argv-validation paths and --help. Signed-off-by: RemiliaForever <remilia@koumakan.cc>
Renames, each because the old name said the wrong thing:
compute_model_size -> model_disk_bytes "compute_" carries no
information; the return is bytes
resolve_via_mm -> resolve_model_id names what is resolved rather
resolve_draft_via_mm -> resolve_draft_id than which subsystem does it
write_json -> write_cell_json was asymmetric with its sibling
write_logits_json
aggregate -> aggregate_runs a bare verb is too generic for a
shared header
format_size -> format_bytes it formats a byte count
Comment trimming, keeping every load-bearing note (#1090, the QAIRT
prefill padding of #1194, the VLM reset rationale, the parser-free
metadata.json read, the geniex_model_paths_free allocator pairing, the
parent_path() anchor requirement) and dropping restatements of the
adjacent name. Two were not merely verbose but wrong or stale:
- looks_like_path's comment claimed ids are rejected when the leading
segment contains '.', which the function never implemented — it tests
only the first character, so `a.b/c` resolves as an id. The comment
now matches the code.
- resolve.c still referred to mm_mmproj / mm_tokenizer, dead names since
those fields moved into the `mm` sub-struct.
The #1090 shadowing rule and resolve_model_id's contract each existed in
two places after the module split; they now live once, on
options_t.model_path and on the bench.h declaration respectively.
Report structure and CLI flags unchanged, verified across the full
matrix of modes.
Signed-off-by: RemiliaForever <remilia@koumakan.cc>
run_one_cell had six return points and four copies of the cleanup block,
and four of the six never called free_mm_paths(): the resolve_draft_id
failure, the geniex_resolve_device failure, the "--logits is not
supported for VLM models" rejection, and the calloc failure. Each of
those can run after resolve_model_id() has already taken ownership of
the manager's heap paths in o->mm.
Restructured to a single exit through a `done` label, which both fixes
the leak and removes the duplication that caused it — one cleanup block
instead of four almost-identical ones.
valgrind on the --logits --vlm rejection path with a model-manager id:
before definitely lost: 113 bytes in 1 blocks
after definitely lost: 0 bytes in 0 blocks
Also in stats.c, aggregate_runs() now rejects n < 1 instead of letting
summarize() read values[0] and values[n-1] out of bounds. parse_args
validates --repetitions >= 1 so this was unreachable in practice, but it
was a cross-translation-unit invariant the compiler could not see and
the signature did not state — gcc had been warning about it since the
module split ('tmp' may be used uninitialized). The tree now builds
clean under -Wall -Wextra -Wshadow.
While there, summarize() sorts the caller's buffer in place rather than
mallocing and memcpying its own copy. The old comment justified the copy
with "so the caller's buffer is untouched", but its only caller refills
that scratch buffer completely before every call, so the copy bought
nothing and cost six malloc/memcpy/free round trips per cell.
README.md no longer calls this a "single-file C example" and points at
bench.h for the module map.
Report structure and CLI flags unchanged; verified identical across
single-cell, --prompt-file segments, --logits, --accuracy, matrix mode,
the argv-validation paths and --help, stderr included.
Signed-off-by: RemiliaForever <remilia@koumakan.cc>
…g the buffer
Three pre-existing defects, all reproducible on main. `--prompt-file`
batching was bolted onto machinery built for a single prompt: a results
array sized o->repeat, and a prompt_buf that options_t shares across
matrix cells and that the splitter rewrote in place.
1. A batched file ran every segment but reported only the last. run_idx
restarts at 0 per segment while runs[] holds o->repeat entries, so
each segment overwrote the previous one's measurement. Three segments
(3 / 19 / 6 prompt tokens measured individually) at -r 2 printed
three [sep ] markers, ran six generations, and wrote two JSON
entries, both carrying the last segment's numbers.
2. In matrix mode every cell after the first ran only segment 1.
build_prompt_list() tokenised prompt_buf in place and `cell = *base`
shares that buffer, so cell 2 walked one that cell 1 had already cut
with NULs: strchr() stopped at the first and the split yielded a
single prompt. Nothing in the report showed it — two identically
configured cells silently benchmarked different input.
3. Any prompt file with trailing whitespace on a line was silently
truncated there, batched or not. The separator test called
rstrip(line), which writes '\0' in place, and only the line's own
'\n' was restored afterwards; the prompt, read as a C string, ended
at the first leftover NUL. This hit the plain timing path, not just
batching: the same two-line file measured 15 prompt tokens without
trailing spaces and 5 with them.
Splitting on `---` is now an --accuracy feature only. That is what it is
for — eyeballing several prompts' output in one invocation — and in a
timing run the segments have different lengths, so a median of
prefill_tps across them would mix populations. Every other mode feeds the
file verbatim as one prompt, `---` lines included, so no existing
invocation changes its exit status.
The fixes are simplifications rather than additions:
- Resolving the prompt list is parse-time input processing, not
something each run loop should redo, so it happens once in
parse_args: `o->accuracy ? build_prompt_list(...) : one_prompt(...)`.
A timing run never enters the splitter at all, and defect 2 becomes
structurally impossible because no per-cell tokenisation is left to
leave leftovers behind. It also makes `const options_t*` honest,
since nothing mutates prompt_buf during a run.
- The separator test is now a read-only is_separator(), which fixes
defect 3 inside --accuracy and lets the loop drop the
cut-then-restore of each line's newline. Only real separators are
overwritten, so every segment keeps its text byte for byte.
- run_one_cell sizes runs[] as repeat * prompt_count and passes that
count to aggregate_runs() and write_cell_json(), instead of three
places re-deriving it from o->repeat. The run loops track one output
index across segments. That closes the implicit-array-length coupling
noted when bench.h was reviewed.
Verified against the pre-change binary. Fixed: batched --accuracy now
reports prompt_tokens [3, 19, 6] matching the three segments measured
individually, both matrix cells report all three, a trailing-space file
measures 15 tokens instead of 5, and a batched file in a timing run
measures the whole 30-token text instead of the last 6-token segment.
Unchanged: single-cell random-ids, a plain --prompt-file, --logits,
--accuracy with one prompt, matrix mode and every argv-validation path
produce identical output, stderr included. The only --help delta is the
--prompt-file paragraph.
usage text and README.md now state that splitting is --accuracy-only and
why.
Signed-off-by: RemiliaForever <remilia@koumakan.cc>
RemiliaForever (RemiliaForever)
marked this pull request as ready for review
August 27, 2026 13:23
Mengsheng Wu (mengshengwu)
approved these changes
Aug 27, 2026
RemiliaForever (RemiliaForever)
deleted the
refactor/split-bench-modules
branch
August 28, 2026 08:28
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
2 participants
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.
Summary
sdk/benchmark/benchmark.c(2174 lines, mixing argv parsing, model-manager resolution, run loops, aggregation and report writing) into per-concern modules —options.c/resolve.c/run.c/stats.c/report.c/util.c— withbench.hcarrying the shared types and a map of which module owns what.---prompt splitter (run_llm+run_vlmeach carried a copy), the random-ids prefill block (run_llm+run_logits), theprofile_data→run_result_tcopy (both loops), the model-manager pull/get_paths dance (model + draft model), and three near-identicalaggJSON writers. Deadjson_field_dbl()and the(void)cast that silenced its unused-function warning go away with them.run_one_cell's two early returns (--logitsagainst a VLM, and the calloc failure) skipped themm_*teardown that the four other exits performed. valgrind reports 113 bytes definitely lost before, 0 after.resolve_draft_via_mmloggingo->draft_modelafter overwriting it, so the line readresolved draft <path> -> <path>instead of naming the id it resolved.run_result_tfields:is_warmupwas never assigned,err[256]never touched,statusonly ever set to 0 and never read.--warmupbelow 0 makeswarmup + repeatshorter thanrepeat, so the tail of the results array stayed calloc-zeroed and was reported as real 0 tok/s runs (--warmup -3 -r 5gave three zero rows and a 0.0 median);--n-genbelow 1 was silently replaced by the plugin's own 128-token default, so the report claimed ann_genthe run never used. Separately, a matrix line over 2047 bytes was split byfgetsinto a cell that ran with a truncated path plus a leftover tail reported as a bogus extra line under the wrong number.Reviewing
Commit 1 is pure movement and can be skimmed: 42 of 45 function bodies are byte-identical to the original. The three exceptions are exactly what the split forces — the two globals that now cross a module boundary (
g_token_callback_delay_usgains a setter,main'sg_mm_initedteardown becomesmm_shutdown()), and one signature re-wrapped by clang-format after losingstatic(zero body change). The substance is in commits 2–5, which are 6–170 lines each.Test plan
Every comparison below is against a build of the pre-split
benchmark.cfrommain, run side by side:--help: 120 lines byte-identical--logitsJSON report: byte-identical (deterministic under a fixed seed)--prompt-filesegmentation: identical[sep ]/[gen ]output and exit codes across 4 cases — normal, leading + back-to-back + whitespace-padded---, no separator at all, all-blank file--logitswith VLM,--logitswith--prompt-file,--logits-top-n 0, missing--plugin, unknown arg, …): identical messages and exit codes--logits+ VLM early return: 113 bytes definitely lost onmain, 0 afterfgetslength check does not false-positive on either edge case (final line without a trailing newline; line exactly 2047 bytes including the newline)clang-format-18clean;gcc -std=gnu11 -Wall -Wextra -Wshadowproduces no warnings; local Linux CMake build greenVerified on Linux / CPU via the
llama_cppplugin only — no Snapdragon device to hand. The change is plugin-agnostic and touches no SDK headers, so there is no FFI surface to update.