gguf: add release_weight_buffer, keyed to the buffer rather than the load - #46
Open
mculbert wants to merge 1 commit into
Open
gguf: add release_weight_buffer, keyed to the buffer rather than the load#46mculbert wants to merge 1 commit into
mculbert wants to merge 1 commit into
Conversation
…load CrispASR is adding core_gguf::release_weight_buffer to its copy of this loader and routing every weight-buffer teardown through it, including in the three shared libraries CrispEmbed builds from ../CrispASR: crisp_audio, crisp_punc and crisp_lid. Those sources compile against THIS header, so without a matching entry point here all three fail to compile — verified, not assumed: reverting this commit and building any target that pulls them in gives "no member named 'release_weight_buffer' in namespace 'core_gguf'" at crisp_audio/src/audio_tower.cpp:692 and crisp_lid/src/lid_cld3.cpp:807. The point of the function upstream is that a no-copy buffer is a view onto pages the backend does not own — ggml_backend_dev_buffer_from_host_ptr has no deallocator parameter — so freeing the buffer alone leaves the weight file mapped. Declaring the name without that behaviour would be worse than not having it: the shared sources would compile here and silently mean something different, which is the cross-repo drift the contract note in this header already exists to prevent. So the mapping is now keyed to the backend buffer, not only to the WeightLoad. WeightLoad::mmap_addr/mmap_len stay as the caller-visible record of a load; free_weights releases through release_weight_buffer and clears them rather than unmapping a second time. This repo has no instance of the leak today. The no-copy path is opt-in (load_weights' try_mmap defaults to false) and its only two callers, deepseek_ocr2 (DS_MMAP) and unlimited_ocr (UOCR_MMAP), keep the whole WeightLoad and tear down through free_weights. But eleven models in src/ move wl.buf into their own struct and free it directly, letting the WeightLoad and its mmap_addr go: bidirlm_vision, fireredpunc, gliner_ner, glm_ocr, got_ocr, internvl2_ocr, lfm2_embed (x2), pcs, qwen2vl_ocr (x2). Each is correct only because its loader never maps, and each would leak the moment try_mmap were added to it. Keying the region to the buffer means the release is correct for both shapes; converting those eleven call sites is left out of this commit deliberately, since none of them is wrong as written. tests/test_gguf_loader_mmap.cpp now asks the kernel which regions still name the weight file rather than inferring release from free_weights returning: 1 while loaded, 0 after. The positive control is not decoration — it caught the first version of the probe comparing against /tmp while the kernel reports /private/tmp, which would have made the absence check vacuous. Made to fail on purpose by skipping the unmap inside release_weight_buffer: "free_weights left 1 mapping(s)". Build: crispembed and every test target build; the mmap test passes. firered-punct-ab still fails to link on _fireredpunc_debug_token_ids, which CrispEmbed's local src/fireredpunc.cpp defines and CrispASR's crisp_punc copy does not — pre-existing drift in the punctuation pair, unrelated to this change and invisible in CI, which has no sibling CrispASR checkout and so builds the local copies. clang-format 18.1.8 clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
This repo does not have the leak CrispStrobe/CrispASR#347 fixes. Claude checked before writing anything, and the
answer is worth stating up front so this PR is read for what it is: a small API change to keep the
shared sources compiling, plus one latent hole closed on the way. Nothing here is a bug fix.
Three reasons CrispEmbed is not affected:
load_weights(..., try_mmap = false)by default. Only two callerspass
true, both behind an env switch:deepseek_ocr2(DS_MMAP) andunlimited_ocr(
UOCR_MMAP).WeightLoad(ctx.model_wl) and tear down throughfree_weights, which already freed the buffer and then unmapped.PROT_READ | MAP_SHARED(gguf_loader.cpp:302;PAGE_READONLY+FILE_MAP_READon Windows). Clean, droppable page cache — not the dirtyMAP_PRIVATE+PROT_READ|PROT_WRITEpages that make the CrispASR case cost real memory.Why this is needed
CrispEmbed builds three shared libraries out of
../CrispASR—crisp_audio,crisp_puncandcrisp_lid, wired up by theCRISP_*_DIRcache paths in the top-levelCMakeLists.txt. Thosesources compile against this repo's
core/gguf_loader.h, and CrispStrobe/CrispASR#347 routes everyweight-buffer teardown in them through a new
core_gguf::release_weight_buffer.So merging #347 without this breaks the CrispEmbed build. Verified rather than reasoned about — with
this commit reverted, any target that pulls those libraries in stops at:
Four call sites in total across the three libraries —
crisp_audio/src/audio_tower.cpp:692,crisp_lid/src/lid_cld3.cpp:807,crisp_punc/src/fireredpunc.cpp:908andcrisp_punc/src/pcs.cpp:1024.What it does, and why not a stub
The narrow fix would be to declare
release_weight_bufferasggml_backend_buffer_freeplus anull-out. It would compile, and it would be correct for this repo today. I did not do that.
Upstream the function's contract is "release this buffer and whatever host mapping the loader
attached to it". A name that means that in one repo and something weaker in the other is exactly the
cross-repo drift the
CROSS-REPO TENSOR-MAP CONTRACTnote at the top of this header already existsto prevent — the header records a run of commits spent recovering from the last round of it, and
tests/test-copies-in-sync.cppcompares paths within one checkout, so it structurally cannot seethis pair.
So the mapping is now keyed to the backend buffer as well, not only to the
WeightLoad:load_weights' no-copy path registers(buf → base, size)in a small mutex-guarded side mapalongside setting
WeightLoad::mmap_addr/mmap_len.release_weight_buffer(ggml_backend_buffer_t&)takes-and-erases that entry in one criticalsection, frees the buffer, then unmaps, then nulls the caller's handle. Take before free, so a
concurrent load that receives a new buffer at the same address cannot have its record erased. A
missing entry is the ordinary case — the copy path maps nothing that outlives the load — and is
released like any other buffer.
free_weightsroutes every buffer through it and then clearsmmap_addr/mmap_lenwithoutunmapping again. Those fields stay as the caller-visible record of a load; they are no longer a
second owner.
Net behaviour for existing callers is unchanged, which is what the test below checks.
The latent risk this leaves, and what closes it
Eleven sites across nine models in
src/movewl.bufout of a function-localWeightLoadintotheir own context and free it directly. The
WeightLoadthen dies at scope exit, takingmmap_addrwith it, so
free_weightsis never reached and nothing holds a handle to the mapping:src/bidirlm_vision.cpp:560ctx.model_bufsrc/fireredpunc.cpp:1085ctx->bufsrc/gliner_ner.cpp:1121ctx->model.bufsrc/glm_ocr.cpp:623ctx.model_bufsrc/got_ocr.cpp:422ctx.model_bufsrc/internvl2_ocr.cpp:1091ctx.model_bufsrc/lfm2_embed.cpp:328ctx->model.bufsrc/lfm2_embed.cpp:345ctx->model.bufsrc/pcs.cpp:982ctx->bufsrc/qwen2vl_ocr.cpp:1135ctx.mmproj_bufsrc/qwen2vl_ocr.cpp:1143ctx.model_buf(Derived by provenance, not by name: an expression assigned from a
WeightLoadfield and never alsofrom
ggml_backend_alloc_ctx_tensors/alloc_buffer/buft_alloc_buffer. That second rule iswhat keeps
kvc.buf,bake_bufand the KV-cache buffers out of the list — those are separateallocations and are freed correctly where they are.)
Every one of them is correct only because its loader never maps. There are two independent
one-word changes that break that, and they are worth separating because they do different things:
Adding
try_mmapto any of these eleven makes the leak happen. That is a single argument atthe
load_weightscall, of exactly the shapedeepseek_ocr2andunlimited_ocralready use, andit is an attractive change — it is there to halve resident memory on a large model. Nothing at the
call site says the teardown a few hundred lines away is now wrong.
Changing the mapping mode makes a leak expensive rather than merely untidy. Today's
PROT_READ | MAP_SHAREDleaves clean pages the kernel can drop under pressure. CrispASR'sMappedFiletakes awritableflag and mapsMAP_PRIVATE | PROT_READ|PROT_WRITE, because someof its backends fold weights in place after load (parakeet's batch-norm-into-conv). A private
writable mapping privatizes on first read, so the resident pages are dirty and anonymous and can
only be compressed or swapped, never dropped. If CrispEmbed ever needs the same in-place folding,
that flag flip turns every leaked mapping from reclaimable page cache into real memory held for
the life of the process — which is the whole of the CrispASR#347 measurement (11.25 GB against
7.73 GB for two models in one process).
Neither trigger is visible from the file it would be typed in.
The fix is one line per site —
ggml_backend_buffer_free(X)→core_gguf::release_weight_buffer(X)— and after this PR the function it needs already exists.
release_weight_bufferisggml_backend_buffer_freeplus a side-map release, so converting a site that never maps isbehaviourally identical; the conversion costs nothing and removes the coupling entirely.
Those eleven are left out of this PR deliberately, since none of them is wrong as written and I did
not want a compile-unblocking change to arrive as a sweep through eleven unrelated models.
CrispASR#347 does the equivalent conversion on its side, so there is precedent for doing it in one pass.
Tests
tests/test_gguf_loader_mmap.cppalready exercised the no-copy path and checked that the tensorsmatch the copy path. It now also asks the kernel which regions still name the weight file, rather
than inferring release from
free_weightsreturning:This is what makes the routing change checkable at all:
free_weightsnow reaches the mappingthrough
release_weight_bufferrather than throughmw.mmap_addr, and a desync between where theregion is registered and where it is taken would leave the file mapped while clearing the field
anyway. The field cannot be the oracle.
Two notes on the check, both learned the hard way:
/tmpwhile the kernel reports the resolved
/private/tmp, so it found zero regions while the file wasmapped — and the absence assertion after the free would have passed for the wrong reason forever.
It is caught because the control asserts at least one mapping exists before anything is released.
proc_regionfilenameis not usable here. Asked about the base of an anonymous region itanswers with the file of the next region at or above that address, counting an unrelated neighbour
as a mapping of the weight file. The probe reads
PROC_PIDREGIONPATHINFO, which returns the regionand its path in one record. Linux reads
/proc/self/maps; elsewhere it reports unsupported and thecheck is skipped rather than silently passing.
Made to fail on purpose, by skipping the unmap inside
release_weight_buffer:FAIL: free_weights left 1 mapping(s) of /tmp/crispembed_test_loader_mmap.gguf.Verification
crispembedand every test target build (macOS arm64,-DGGML_METAL=ON).test-gguf-loader-mmappasses, output above.crisp_audio,crisp_puncandcrisp_lidall compile against the changed header withCrispASR#347 applied, and all three fail to compile without this change.
One pre-existing failure, unrelated and left alone:
firered-punct-abdoes not link whencrisp_punccomes from a sibling CrispASR checkout, because_fireredpunc_debug_token_idsisdefined in this repo's
src/fireredpunc.cppand not in CrispASR'scrisp_punc/copy — the sameduplicated-file drift the header warns about, in the punctuation pair rather than the loader. It does
not appear in CI, which has no sibling checkout and so builds the local copies.
Authored by Claude Opus 5.