fix: lease pinned staging bundles per load to make concurrent loads safe - #34
Open
AlperenKonukbay wants to merge 1 commit into
Open
fix: lease pinned staging bundles per load to make concurrent loads safe#34AlperenKonukbay wants to merge 1 commit into
AlperenKonukbay wants to merge 1 commit into
Conversation
…l pool Two concurrent read_flashpack_file calls previously received the SAME staging buffers — reader A preadv'd bytes into buffers whose async H2D copies reader B had in flight, silently corrupting weights on the GPU (bit-exact packs, checkerboard/black outputs; worked around app-side by serializing loads in fal-ai/registry #11822/#11826). Leases check bundles out of a free list under the lock; concurrent loads that outrun the cache allocate fresh bundles and return them on release, capped by FLASHPACK_PINNED_CACHE_BUNDLES (default 16 = the old 2 GiB footprint). Bundles are keyed by chunk size only, so differing thread counts still reuse the cache. Release happens after thread join + device synchronize, so no in-flight copy can source from a re-leased bundle.
AlperenKonukbay
changed the title
Contributor
|
Review by fal-9000 (GPT-5.6 Sol, xhigh) — reviewed at head No issues found. Checked:
Checks were green at review time. |
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.
The shared pinned-buffer pool is unsafe under concurrent loads: two simultaneous
read_flashpack_filecalls in one process can receive the same staging bundles and silently corrupt each other's weights. This is why id-lora serializes request-path loads behind a lock and why the #11822/#11826 workarounds exist.This changes the pool to a lease: each load takes exclusive pinned bundles (keyed by chunk size, capped by
FLASHPACK_PINNED_CACHE_BUNDLES, default 16) and releases them after its reader threads join and synchronize. Single loads are unchanged; concurrent loads become safe. Note the cap bounds the free-list, not retained host memory: bundles dropped above the cap return to torch's caching host allocator, which recycles them (measured at 32 threads on a real app: the leased build was faster than its merge-base in 6/6 paired 40.86 GB reads, 3.26 s vs 3.75 s mean).Measured on flux-2 (B200): the treated arm carried this commit plus the re-enabled 3-way concurrent component load it makes safe (concurrency without the lease is the known-broken configuration, so the two ship together). Two simultaneous same-node loads produce byte-identical outputs — including a deliberate worst-case pair racing on one node — and the concurrent load gives 13.1–15.0 s vs 16.6–18.7 s sequential (2–3 boots per arm; cache state dominates wider swings). 216 tests green including new lease units.
Note the latency win lands only when consumers re-enable concurrency — follow-ups once released: restore flux-2's parallel component load and remove id-lora's request-path lock (the id-lora one needs a pinned-alloc cost check first; per-request lease churn above the cache cap is unmeasured).