gguf: release weight mmap through buffer ownership - #47
Merged
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
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.
Companion to CrispASR #347. Adds the shared
core_gguf::release_weight_buffercontract and mapping lifetime cleanup required by CrispASR shared sources (crisp_audio,crisp_punc,crisp_lid).Validation on Apple Silicon:
test-gguf-loader-mmap: pass; no-copy mmap path exercised, mapping count 1 while loaded / 0 after freeThis branch is the companion implementation already referenced by CrispASR #347.