core: release each tensor's staged mmap pages after the backend copy - #113
Open
erichchampion wants to merge 1 commit into
Open
core: release each tensor's staged mmap pages after the backend copy#113erichchampion wants to merge 1 commit into
erichchampion wants to merge 1 commit into
Conversation
An embedder loading several GGUFs (or reloading one) pays for every staging mmap twice: the pages stay resident for the life of the process even after the backend copy has made them redundant, and on macOS the address space is the scarce resource -- iOS kills on footprint long before malloc fails. gguf/safetensors staging now records each tensor's file range and, once the backend copy of that tensor is done, returns the pages: the mapping is dropped copy-before-unmap, with a debug-mode overlap sweep asserting no tensor's range is read after release, and silent release failures surfaced rather than swallowed. Loads that fail mid-way release what they staged instead of relying on process teardown; the GGUF/safetensors close paths and the weight-ctx copy factory share one release body, and repeated warnings key on the format so a multi-hundred-tensor model prints one report line, not hundreds. Behaviour-preserving numerically: nothing reads a released range, verified by the overlap sweep under Debug on every loader. Memory behaviour is the point of the change. Non-Apple builds keep compiling (the release call degrades to a no-op where the platform has no usable madvise). This is an embedder-concern patch from [Cadenza](https://github.com/erichchampion/cadenza-audio), offered separately from the add_subdirectory / ace_fatal / AceProgress / backend-config series because it is the one we would keep carrying if upstream is not interested.
Contributor
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
A fifth patch from Cadenza, offered separately from the add_subdirectory / ace_fatal / AceProgress / backend-config series: this one is an embedder-concern with narrower appeal, so we would entirely understand if it is not wanted -- it is the one we expect to keep carrying if so.
The problem
An embedder loading several GGUFs (or reloading one) pays for every staging mmap twice: the pages stay resident for the life of the process even after the backend copy has made them redundant. On iOS the kill is on footprint long before malloc fails, so the redundant residency is the difference between a model set fitting and not.
What it does
Numerically a no-op: nothing reads a released range -- that is what the overlap sweep verifies under Debug on every loader. The memory behaviour is the point of the change.
Testing
Full engine build with the overlap sweep on, all loader paths exercised against real GGUFs on macOS/arm64 (Metal); philox/LM-prompt test binaries green.