Metal: rebase MTLResidencySet expert residency onto split runtime#426
Merged
Conversation
…hutdown) Env-gated (COLI_METAL_RESSET=1) persistent MTLResidencySet attached to g_queue (macOS 15+, @available-guarded with a one-line stderr fallback). Adds resset_add/resset_remove/resset_flush helpers, wires them into the existing coli_metal_register/coli_metal_unregister/coli_metal_shutdown bodies -- no new functions in backend_metal.h, no glm.c changes, every existing call site keeps its current signature and behavior. register() defers the commit (resset_add just marks dirty, under the same g_slab_mtx that already serializes parallel OMP loader threads) so a loader burst doesn't pay a commit per slab; unregister() commits synchronously and immediately, because the caller frees the underlying host memory right after it returns and a deferred removal would leave the set referencing freed memory. Nothing yet reads g_resset_enabled to change dispatch behavior -- this commit is bookkeeping only, gate on or off, so it does not change what any command buffer does (verified by inspection: no useResource:/useHeap: call site is touched here). Gate off (default) is byte-for-byte the stock path: g_resset_enabled starts false and nothing sets it outside the COLI_METAL_RESSET branch in coli_metal_init, so every new helper is a no-op. See SUMMARY.md (next commit) for the full design and UNCERTAINTIES.
…esource: The one seam the mechanism history actually implicates: moe_submit's `use` vector (resolved expert weight/scale slabs) is the only useResource: loop whose length scales with LRU cache size. With COLI_METAL_RESSET=1, skip that loop entirely -- the queue-attached MTLResidencySet already guarantees those buffers are resident -- after resset_flush() commits any pending adds from a loader burst. Every other useResource: call site (bind_gemv's weight/scale buffers, attn_decode/layer_decode's Lb/Rb/kvbW/kvbS/inB/pnB/rwB/rbB, coli_metal_gemm's wb/sb) is left unconditionally unchanged: none of them scale with cache size, and Lb/Rb carry real GPU-side write traffic ordered by existing explicit memoryBarrierWithScope: calls, not by useResource:'s hazard tracking -- narrowing the blast radius rather than removing useResource: uniformly. Full reasoning, the hazard-tracking tradeoff (residency sets don't support hazard tracking, per Apple's MTLResidencySet developer documentation and residency-set adoption guide; the SDK header itself is silent on the topic), and every judgment call in UNCERTAINTIES: see SUMMARY.md. Gate off is unaffected: g_resset_enabled is false, so the useResource: loop runs exactly as before.
…tations) 1. REQUIRED (backend_metal.mm): no Metal call runs under g_slab_mtx anymore. The set mutations (addAllocation/removeAllocation/commit) and the dirty flag moved to a dedicated g_resset_mtx; coli_metal_register/unregister do their g_slabs bookkeeping under g_slab_mtx exactly as stock, then call resset_add/resset_remove after dropping it (still before returning, so unregister's remove+commit still lands before the caller frees the host memory). The original shape -- commit under the slab lock the parallel OMP loader threads contend on -- was structurally identical to the mutex-over- live-Metal-call bug E4's audit round 2 fixed, the leading suspect for its replicated +12s expert-disk regression. The register->flush->resolve happens-before argument survives the split: resset_add completes inside coli_metal_register before it returns, the engine cannot dispatch an expert before its load returns, and add/flush are serialized by g_resset_mtx (comment at resset_add). The two mutexes are never held together, so no lock-order hazard exists. 2. REQUIRED (backend_metal.mm, backend_metal.h, glm.c): the resset_flush cost in moe_submit -- deliberately outside the g_t_setup window so the A/B harness counters keep their meaning -- was invisible. New g_t_resset_flush accumulator timed around the flush, exported via coli_metal_resset_stats(), printed by profile_print as a separate gate-on-only "METAL-RESSET: flush" line (mirrors E4's METAL-HEAP line; the METAL: line the harness parses keeps its exact format; stock output byte-identical -- the stats call returns 0 with the gate off and the whole block sits in the pre-existing #ifdef COLI_METAL arm). Register- side add/remove costs land in the existing t_ewait window; comment at resset_add says so instead of a second counter. 3. REQUIRED (SUMMARY.md; the moe_submit commit message was already reworded in place, pre-push): corrected two false attributions -- the hazard- tracking and thread-safety statements were credited to the SDK header, which is silent on both; the actual source is Apple's online MTLResidencySet class reference and residency-set adoption guide (fetched 2026-07-18). Note: the scaffolding commit's message was checked and contains no such claim, so it was left untouched. 4. DOC (SUMMARY.md): the pre-existing fslab OOM-unwind bug (glm.c ~1868-73, frees s->slab without unregister) has a strictly longer-lived exposure under E5 (permanent set member vs stock's transient per-CB declaration). Out of scope here; the upstream PR built from E5 must carry the one-line fix (reference: E4 branch commit 6753225).
expert_load's fslab-OOM unwind freed s->slab via compat_aligned_free without coli_metal_unregister -- a pre-existing gap on main/dev (validator-confirmed during E4) that leaves a stale g_slabs entry over freed memory, letting resolve() hand the GPU a dangling pointer. Under COLI_METAL_RESSET=1 the exposure is strictly longer-lived: the wrapped buffer would stay a permanent residency-set member over the freed pages instead of stock's transient last-command-buffer window. Ported from e4/metal-heap validator fix 6753225, adapted to dev's non-heap code shape (no coli_metal_heap_free wrapper -- plain unregister-before-free). The uring_load_add analog (E4 audit round-2 insurance) is deliberately not carried: that arm is #ifdef __linux__-gated while COLI_METAL is macOS-only, so it is dead code on every real build target, and unlike E4 this branch has no allocation-path reason to touch the function. Reachable only through allocation failure mid-load; verified by inspection and clean builds (no OOM-injection harness in tree). SUMMARY.md item 10 updated to "carried on this branch".
…x ref) 1. DEFENSIVE (backend_metal.mm, coli_metal_register): re-registering a live base overwrote s.buf and resset_add'ed the new wrapper without removing the OLD one from the residency set -- ARC drops our reference but the set retains the object and keeps its pages resident: a leak and a set/g_slabs divergence. The found branch now stashes the replaced wrapper under g_slab_mtx and, after dropping the lock (round-1 invariant: no Metal call under the slab lock), resset_remove(old)s it before resset_add(b); identical old==b early-outs both set operations. Invariant defended, stated in the comment: residency-set membership mirrors g_slabs exactly. No in-tree caller re-registers a live base today (audited) -- closed defensively. 2. DOC (SUMMARY.md): the moe_submit lifecycle bullet still said resset_flush commits "under g_slab_mtx" -- stale text from before the round-1 mutex split; the code takes g_resset_mtx and never touches the slab lock there. Parenthetical corrected; register bullet updated to describe the re-register hygiene from item 1.
Update the rebased documentation and comments to name colibri.c after the JustVugg#391 coordinator split, and correct the E5 comparison table to reflect its instrumentation and OOM-unwind touches. Co-authored-by: Christopher Brand <brand.christopher.c@gmail.com>
Record that the explicit Objective-C++ warning build reports only the pre-existing unused TG constant inherited from current dev. Co-authored-by: Christopher Brand <brand.christopher.c@gmail.com>
rgbkrk
marked this pull request as ready for review
July 19, 2026 16:43
Contributor
|
*Authored by Fable 5 in Claude Code, analysis in partnership with @monotophic". This is fantastic, thank you for picking up the thread. I appreciate the attribution Verification from the #392 sideWe independently verified patch-equivalence against the original branch:
The M3 Max gate-off control complements the M5 Max data on #392 (ours + @RDouglasSharp's Again, I endorse this PR to supersede #392, and look forward to it merging - I'm Mechanics when posting:
|
This was referenced Jul 19, 2026
Contributor
|
Datapoint from a smaller Apple box: M5 Pro, 64 GB, macOS 26.5.2, GLM-5.2 REAP-504B int4, expert-pruned to 168 per layer, 232 GB container streamed with DIRECT=1. Built this branch with METAL=1 ARCH=native and ran gate-off vs gate-on as three interleaved pairs on one binary. Greedy, 13-token prompt, 128 generated, PIPE=1 PIPE_WORKERS=8 RAM_GB=50 TOPP=0.7, same .coli_usage restored before every run, quiet-machine bracket around each.
Expert hit rate was 72.2% in all six runs, dispatch counts identical, outputs byte-identical. Residency commit cost 0.18 to 0.23s per 128 tokens, so the 1.5s figure I saw on an 8-token smoke was just load-dominated. Pair 3 reads as thermal, not mechanism: every kernel ran about 30% slower at the same dispatch counts, attention kernel 6.25s in pair 3 vs 4.65s in pair 1, and the on-arm always ran second and hotter, so run order works against this PR. Decode here is disk-bound, 51% of wall is expert-disk wait, which caps what any GPU-side change can show. The matmul-wall drop is still clearly visible. Works as advertised on this hardware. |
Contributor
|
Authored by Fable 5 in Claude Code, analysis in partnership with @monotophic Testing delivered, as promised above. This branch (@
Endorsement stands; ready to merge from our perspective. |
This was referenced Jul 19, 2026
Merged
JustVugg
added a commit
that referenced
this pull request
Jul 20, 2026
int3-g64 (fmt=5) container: 3-bit weights with per-group (gs=64) scales. Measured 3.34x lower outlier-row RMS than int4-row at ~25% fewer bytes (#132 ablation point). Rebased by @fabio-rovai onto post-#426 dev; fmt=5 registered in qt_resolve_fmt (#413 security gate) with fmt-4/fmt-5 disambiguation tested at real GLM I=7168; io_uring expert path converted to qt_resolve_fmt (fixes latent fmt-4 mis-tag); CUDA graceful CPU-fallback. Verified locally: no regression on existing formats, test_int3 + CI green.
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
4 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
Rebase the
MTLResidencySetexpert-residency work from #392 onto currentdevafter thec/glm.csplit in #391, following JustVugg's maintainer guidance.The mechanism remains env-gated behind
COLI_METAL_RESSET=1. On macOS 15+, it attaches a persistent residency set to the Metal command queue and removes the per-command-bufferuseResource:walk over expert buffers frommoe_submit. Gate-off behavior remains unchanged.Rebase notes
dev.c/glm.cintoc/colibri.c.git range-diffshows the mechanism commits are patch-equivalent apart from that file relocation.colibri.c, and corrected the E5 comparison table to acknowledge its instrumentation and OOM-unwind touches.Attribution
This is a rebased continuation of Christopher Brand's work in #392. All five mechanism commits preserve @monotophic as their original Git author. Additional M5 Max validation and analysis remain documented on #392 by @monotophic and @RDouglasSharp.
M3 Max validation
Hardware: Apple M3 Max, 128 GB unified memory, macOS 26.4.1.
make -C c colibri METAL=1: passedmake -C c colibri METAL=0: passedmake -C c metal-test: passed all Metal kernel, MoE, GEMM, and fused-attention tests-Wall -WextraObjective-C++ compile: only the pre-existing unusedTGwarning from currentdevThe real-model comparison used the same prompt and settings as the prior local baseline:
Compare the myths of Lucifer and Prometheus,--ram 96 --ngen 1024,DIRECT=1,IO_THREADS=8,MTP=0,DRAFT=0, plusCOLI_METAL_RESSET=1.dev+ residency setThe residency-set run had zero CPU fallbacks and reported only 0.62s total residency-set flush time. This is a useful end-to-end result but not a clean attribution of the 1.69x historical improvement because current
devcontains substantial unrelated performance work.A same-binary gate-off control confirms that distinction: the residency-set run was about 1.11 tok/s at its 256-token checkpoint, while a subsequent gate-off 256-token run completed at 1.08 tok/s with a comparable 73% hit rate. The roughly 3% difference is within single-run noise and the gate-off control ran second with warmer caches. On this moderate M3 Max configuration, the result supports #392's intended framing as a robustness/scalability fix that removes residency bookkeeping cliffs rather than a large standalone throughput lever.