perf(cpp): async layer-ahead prefetch + transparent hugepages for mmap'd GGUF - #29
perf(cpp): async layer-ahead prefetch + transparent hugepages for mmap'd GGUF#29Jackson57279 wants to merge 5 commits into
Conversation
…p'd GGUF Builds on PR #28 (SSD-aware mmap policies) by: 1. Adding an async layer prefetcher that overlaps SSD page-in with per-layer CPU compute via Linux readahead(2). 2. Completing the mmap_hugepages recommendation from PR #28 by wiring madvise(MADV_HUGEPAGE) through GgufLoadOptions and the CLI. Details: - GgufModel groups blk.<N>.* tensors into per-layer byte ranges and keeps shard file descriptors open for readahead(2). - New LayerPrefetcher issues readahead(fd, offset, len) hints from a background thread while the main thread computes the current layer. - CLI adds --prefetch-layers <n> (-1=autotune, 0=off, 1+=lookahead) and --mmap-hugepages. - Autotune enables prefetch for models >80% RAM and deeper prefetch for models >1.5x RAM; enables mmap_hugepages for models >=200 GiB with free 2 MiB hugepages. Both appear in --print-plan JSON/summary. - Tests cover layer range grouping, mmap_hugepages load, and autotune plan output. Benchmark on ai@192.168.1.132 (Qwen2.5-32B-Q4_K_M, ~18.5 GiB, warm cache): --prefetch-layers 0: total 9.23s, prefill 0.43 tok/s --prefetch-layers 1: total 6.37s, prefill 1.35 tok/s (-31% wall, +3.1x prefill) --prefetch-layers 2: total 8.34s, prefill 0.46 tok/s Research basis: LLM in a Flash (Apple, 2024); llama.cpp prefetch PRs.
![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: 0650f5b80b
ℹ️ 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".
| if (prefetcher_) { | ||
| size_t ahead = l + static_cast<size_t>(prefetch_layers_); | ||
| if (ahead < cfg.layer_count) prefetcher_->request(ahead); |
There was a problem hiding this comment.
This request only runs in run_layers(), but the common non-MoE prompt path in forward() sends any multi-token prefill to forward_batched(), whose layer loop has no matching prefetcher_->request(). With --auto on a large model this can combine mmap_policy=demand with no layer-ahead readahead during the cold prefill, so the most I/O-bound phase falls back to synchronous page faults until single-token decode starts; mirror the request in the batched layer loop or avoid that path when prefetching is enabled.
Useful? React with 👍 / 👎.
| size_t n = 0; | ||
| if (!parse_size(v, n) || static_cast<int>(n) < -1) { |
There was a problem hiding this comment.
The help text advertises --prefetch-layers -1 as the auto sentinel and Args initializes the field to -1, but this parser reads the value with parse_size(), so an explicit -1 is rejected before the cast check ever matters. Users or scripts cannot request the documented auto behavior explicitly; parse a signed integer here or special-case -1 before using the unsigned parser.
Useful? React with 👍 / 👎.
| auto model = oxidize::GgufModel::load( | ||
| "../tests/fixtures/valid-v3.gguf", opts); |
There was a problem hiding this comment.
ctest runs this binary from the CMake build directory, so this relative path only works for a build directory immediately under oxidize-cpp; in an out-of-source build such as cmake -S oxidize-cpp -B /tmp/oxidize-cpp-build, gguf_mmap_policy_test aborts with cannot open ../tests/fixtures/valid-v3.gguf. Use a CMake-provided source-dir define or generate the tiny fixture inside the test so the test suite is not tied to a specific build directory layout.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="oxidize-cpp/src/model_llama.cpp">
<violation number="1" location="oxidize-cpp/src/model_llama.cpp:823">
P2: The async prefetch request is only issued in `run_layers()`, but multi-token prefill typically routes through `forward_batched()`, whose layer loop has no corresponding `prefetcher_->request()`. This means the most I/O-bound phase (cold prefill with a prompt longer than one token) won't benefit from layer-ahead readahead and will hit synchronous page faults. Consider mirroring the prefetch request in the batched layer loop, or documenting clearly that prefetch only helps single-token decode.</violation>
<violation number="2" location="oxidize-cpp/src/model_llama.cpp:825">
P2: Layer-ahead prefetch is effectively one-shot per layer, so long decodes can lose the intended SSD/compute overlap after initial passes. The added `request(ahead)` call is gated by `LayerPrefetcher`'s permanent `requested_` dedupe, so consider allowing re-requests per decode step/window instead of lifetime-only dedupe.</violation>
</file>
<file name="oxidize-cpp/src/gguf.cpp">
<violation number="1" location="oxidize-cpp/src/gguf.cpp:695">
P2: Split-model load can leak file descriptors on shard-parse errors, which can exhaust process FD limits after repeated failed loads. This happens because `fd` is now retained for readahead, but the parse-failure cleanup path only unmaps and does not close the just-opened shard FD.</violation>
</file>
<file name="oxidize-cpp/tests/prefetch_layer_map_test.cpp">
<violation number="1" location="oxidize-cpp/tests/prefetch_layer_map_test.cpp:106">
P2: Test assertions use `assert()` which is compiled out in `-DNDEBUG` builds. If the test binary is ever built without debug macros, all checks silently vanish but the test still prints "ok" and exits 0. Consider using a test framework or a custom `TEST_ASSERT` macro that remains active regardless of build config.</violation>
</file>
<file name="oxidize-cpp/src/cli/main.cpp">
<violation number="1" location="oxidize-cpp/src/cli/main.cpp:229">
P2: `--prefetch-layers` currently rejects the documented `-1` auto mode because the parser only accepts unsigned values. Parsing into `size_t` then casting to `int` can also mis-handle very large values; parsing directly as signed and validating `>= -1` avoids both behaviors.</violation>
</file>
<file name="oxidize-cpp/tests/gguf_mmap_policy_test.cpp">
<violation number="1" location="oxidize-cpp/tests/gguf_mmap_policy_test.cpp:14">
P2: The fixture path `"../tests/fixtures/valid-v3.gguf"` is relative to the working directory at test runtime, which is typically the build directory. This works only when the build directory is directly inside `oxidize-cpp/`; in an out-of-source build (e.g., `cmake -S oxidize-cpp -B /tmp/build`) the test will fail to open the file. Consider using a CMake-defined `TEST_DATA_DIR` compile definition (e.g., `target_compile_definitions(gguf_mmap_policy_test PRIVATE TEST_DATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures")`) so the path is always correct regardless of build layout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| size_t n = 0; | ||
| if (!parse_size(v, n) || static_cast<int>(n) < -1) { | ||
| std::fprintf(stderr, "error: invalid --prefetch-layers '%s'\n", v.c_str()); | ||
| usage_and_exit(argv[0], 2); | ||
| } | ||
| a.prefetch_layers = static_cast<int>(n); |
There was a problem hiding this comment.
P2: --prefetch-layers currently rejects the documented -1 auto mode because the parser only accepts unsigned values. Parsing into size_t then casting to int can also mis-handle very large values; parsing directly as signed and validating >= -1 avoids both behaviors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/src/cli/main.cpp, line 229:
<comment>`--prefetch-layers` currently rejects the documented `-1` auto mode because the parser only accepts unsigned values. Parsing into `size_t` then casting to `int` can also mis-handle very large values; parsing directly as signed and validating `>= -1` avoids both behaviors.</comment>
<file context>
@@ -208,6 +218,21 @@ Args parse_args(int argc, char** argv) {
+ a.mmap_hugepages_explicit = true;
+ } else if (arg == "--prefetch-layers") {
+ std::string v = take_value(argc, argv, i, "--prefetch-layers");
+ size_t n = 0;
+ if (!parse_size(v, n) || static_cast<int>(n) < -1) {
+ std::fprintf(stderr, "error: invalid --prefetch-layers '%s'\n", v.c_str());
</file context>
| size_t n = 0; | |
| if (!parse_size(v, n) || static_cast<int>(n) < -1) { | |
| std::fprintf(stderr, "error: invalid --prefetch-layers '%s'\n", v.c_str()); | |
| usage_and_exit(argv[0], 2); | |
| } | |
| a.prefetch_layers = static_cast<int>(n); | |
| int n = 0; | |
| auto* begin = v.data(); | |
| auto* end = v.data() + v.size(); | |
| auto res = std::from_chars(begin, end, n); | |
| if (res.ec != std::errc{} || res.ptr != end || n < -1) { | |
| std::fprintf(stderr, "error: invalid --prefetch-layers '%s'\n", v.c_str()); | |
| usage_and_exit(argv[0], 2); | |
| } | |
| a.prefetch_layers = n; |
| for (size_t l = 0; l < cfg.layer_count; ++l) { | ||
| if (prefetcher_) { | ||
| size_t ahead = l + static_cast<size_t>(prefetch_layers_); | ||
| if (ahead < cfg.layer_count) prefetcher_->request(ahead); |
There was a problem hiding this comment.
P2: Layer-ahead prefetch is effectively one-shot per layer, so long decodes can lose the intended SSD/compute overlap after initial passes. The added request(ahead) call is gated by LayerPrefetcher's permanent requested_ dedupe, so consider allowing re-requests per decode step/window instead of lifetime-only dedupe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/src/model_llama.cpp, line 825:
<comment>Layer-ahead prefetch is effectively one-shot per layer, so long decodes can lose the intended SSD/compute overlap after initial passes. The added `request(ahead)` call is gated by `LayerPrefetcher`'s permanent `requested_` dedupe, so consider allowing re-requests per decode step/window instead of lifetime-only dedupe.</comment>
<file context>
@@ -807,6 +820,11 @@ void LlamaModel::run_layers(size_t pos) {
for (size_t l = 0; l < cfg.layer_count; ++l) {
+ if (prefetcher_) {
+ size_t ahead = l + static_cast<size_t>(prefetch_layers_);
+ if (ahead < cfg.layer_count) prefetcher_->request(ahead);
+ }
+
</file context>
|
|
||
| size_t shard_idx = first_model.shards_.size(); | ||
| first_model.shards_.push_back(Shard{smap, sbase, ssize}); | ||
| first_model.shards_.push_back(Shard{smap, sbase, ssize, fd}); |
There was a problem hiding this comment.
P2: Split-model load can leak file descriptors on shard-parse errors, which can exhaust process FD limits after repeated failed loads. This happens because fd is now retained for readahead, but the parse-failure cleanup path only unmaps and does not close the just-opened shard FD.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/src/gguf.cpp, line 695:
<comment>Split-model load can leak file descriptors on shard-parse errors, which can exhaust process FD limits after repeated failed loads. This happens because `fd` is now retained for readahead, but the parse-failure cleanup path only unmaps and does not close the just-opened shard FD.</comment>
<file context>
@@ -640,7 +692,7 @@ GgufModel GgufModel::load_split(const std::string& first_path,
size_t shard_idx = first_model.shards_.size();
- first_model.shards_.push_back(Shard{smap, sbase, ssize});
+ first_model.shards_.push_back(Shard{smap, sbase, ssize, fd});
// Append this shard's tensor infos, tagging their owning shard so tensor()
</file context>
| oxidize::GgufModel model = oxidize::GgufModel::load(path); | ||
| const auto& ranges = model.layer_ranges(); | ||
|
|
||
| assert(ranges.size() == 2); |
There was a problem hiding this comment.
P2: Test assertions use assert() which is compiled out in -DNDEBUG builds. If the test binary is ever built without debug macros, all checks silently vanish but the test still prints "ok" and exits 0. Consider using a test framework or a custom TEST_ASSERT macro that remains active regardless of build config.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/tests/prefetch_layer_map_test.cpp, line 106:
<comment>Test assertions use `assert()` which is compiled out in `-DNDEBUG` builds. If the test binary is ever built without debug macros, all checks silently vanish but the test still prints "ok" and exits 0. Consider using a test framework or a custom `TEST_ASSERT` macro that remains active regardless of build config.</comment>
<file context>
@@ -0,0 +1,130 @@
+ oxidize::GgufModel model = oxidize::GgufModel::load(path);
+ const auto& ranges = model.layer_ranges();
+
+ assert(ranges.size() == 2);
+ assert(ranges.count(0) == 1);
+ assert(ranges.count(1) == 1);
</file context>
| std::vector<float> head_scratch(head_dim); | ||
|
|
||
| for (size_t l = 0; l < cfg.layer_count; ++l) { | ||
| if (prefetcher_) { |
There was a problem hiding this comment.
P2: The async prefetch request is only issued in run_layers(), but multi-token prefill typically routes through forward_batched(), whose layer loop has no corresponding prefetcher_->request(). This means the most I/O-bound phase (cold prefill with a prompt longer than one token) won't benefit from layer-ahead readahead and will hit synchronous page faults. Consider mirroring the prefetch request in the batched layer loop, or documenting clearly that prefetch only helps single-token decode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/src/model_llama.cpp, line 823:
<comment>The async prefetch request is only issued in `run_layers()`, but multi-token prefill typically routes through `forward_batched()`, whose layer loop has no corresponding `prefetcher_->request()`. This means the most I/O-bound phase (cold prefill with a prompt longer than one token) won't benefit from layer-ahead readahead and will hit synchronous page faults. Consider mirroring the prefetch request in the batched layer loop, or documenting clearly that prefetch only helps single-token decode.</comment>
<file context>
@@ -807,6 +820,11 @@ void LlamaModel::run_layers(size_t pos) {
std::vector<float> head_scratch(head_dim);
for (size_t l = 0; l < cfg.layer_count; ++l) {
+ if (prefetcher_) {
+ size_t ahead = l + static_cast<size_t>(prefetch_layers_);
+ if (ahead < cfg.layer_count) prefetcher_->request(ahead);
</file context>
| opts.mmap_policy = oxidize::MmapPolicy::Prefetch; | ||
| opts.mmap_hugepages = true; | ||
| auto model = oxidize::GgufModel::load( | ||
| "../tests/fixtures/valid-v3.gguf", opts); |
There was a problem hiding this comment.
P2: The fixture path "../tests/fixtures/valid-v3.gguf" is relative to the working directory at test runtime, which is typically the build directory. This works only when the build directory is directly inside oxidize-cpp/; in an out-of-source build (e.g., cmake -S oxidize-cpp -B /tmp/build) the test will fail to open the file. Consider using a CMake-defined TEST_DATA_DIR compile definition (e.g., target_compile_definitions(gguf_mmap_policy_test PRIVATE TEST_DATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures")) so the path is always correct regardless of build layout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At oxidize-cpp/tests/gguf_mmap_policy_test.cpp, line 14:
<comment>The fixture path `"../tests/fixtures/valid-v3.gguf"` is relative to the working directory at test runtime, which is typically the build directory. This works only when the build directory is directly inside `oxidize-cpp/`; in an out-of-source build (e.g., `cmake -S oxidize-cpp -B /tmp/build`) the test will fail to open the file. Consider using a CMake-defined `TEST_DATA_DIR` compile definition (e.g., `target_compile_definitions(gguf_mmap_policy_test PRIVATE TEST_DATA_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures")`) so the path is always correct regardless of build layout.</comment>
<file context>
@@ -0,0 +1,41 @@
+ opts.mmap_policy = oxidize::MmapPolicy::Prefetch;
+ opts.mmap_hugepages = true;
+ auto model = oxidize::GgufModel::load(
+ "../tests/fixtures/valid-v3.gguf", opts);
+ assert(model.size() > 0);
+}
</file context>
Replace assert() with an abort-based require() helper in gguf_mmap_policy_test and autotune_test so assertions still fire in Release builds (-DNDEBUG), and round-trip all four mmap policies (demand/prefetch/sequential/random) plus the empty-string case. Addresses cubic.dev review on PR #28. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cargo deny advisories fail on anyhow 1.0.102 (unsound Error::downcast_mut). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Picks up the anyhow 1.0.103 bump (RUSTSEC-2026-0190) and the NDEBUG-proof mmap-policy test rework; hugepages test kept and ported to require(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Triage note (maintainer pass): CI is green and the prefetch benchmark (-31% wall, +3.1x prefill on Qwen2.5-32B-Q4_K_M) is a solid self-contained win. Blocker before merge is overlap, not correctness: #32's cubic summary lists the same async prefetch + Also this stacks on #28 — see the ordering note there. |
1 participant
Builds on #28 (SSD-aware mmap policies).
Adds async layer-ahead prefetch via Linux readahead(2) and completes the transparent-hugepages recommendation from #28 by wiring
madvise(MADV_HUGEPAGE)through the load path.Benchmark on ai@192.168.1.132 (Qwen2.5-32B-Q4_K_M, ~18.5 GiB):
--prefetch-layers 0: total 9.23s, prefill 0.43 tok/s--prefetch-layers 1: total 6.37s, prefill 1.35 tok/s (-31% wall, +3.1x prefill)See SPEC.md in the diff for full design details and known limitations.
Validation update (continued from Kimi session)
autotune_test,gguf_mmap_policy_test,prefetch_layer_map_test.ai@192.168.1.132.ai@192.168.1.132with Qwen2.5-32B-Q4_K_M + Qwen2.5-3B-F16 draft. It did not improve decode throughput on this dual-socket CPU setup; the additional draft-model memory traffic saturated available bandwidth and batched verification overhead outweighed the gains. Those experimental changes have been stashed and are not included in this PR.Summary by cubic
Adds async layer-ahead prefetch for mmap’d GGUF weights and wires transparent hugepages into the load path to cut cold page faults and reduce decode jitter. On Qwen2.5-32B-Q4_K_M (~18.5 GiB),
--prefetch-layers 1cut wall time by 31% and increased prefill throughput by 3.1x.New Features
GgufModeland a backgroundLayerPrefetcherthat issues Linuxreadahead(2)while computing the current layer.madvise(MADV_HUGEPAGE)viaGgufLoadOptions.mmap_hugepages; added--mmap-hugepages.--prefetch-layers <n>and--mmap-policy demand|prefetch|sequential|random; autotune setsmmap_policyand lookahead depth based on model size vs RAM and prints them in--print-plan. Tests are NDEBUG-proof and round-trip all mmap policies; cover layer mapping, policy parsing, and plan output.Migration
--prefetch-layers 0to disable.--autoor set--prefetch-layers 1(Linux only). Optionally add--mmap-hugepagesif THP is available.Written for commit feaee40. Summary will update on new commits.