Skip to content

compile: a freed closure leaves every thread's compile cache - #43

Merged
davidtai merged 2 commits into
mainfrom
fix/compile-erase-all-threads
Oct 24, 2026
Merged

davidtai merged 2 commits into
mainfrom
fix/compile-erase-all-threads

Conversation

@davidtai

@davidtai davidtai commented Oct 24, 2026 •

Copy link
Copy Markdown

Summary

compile(std::function fun, bool shapeless) keys a closure (a function with no addressable target) by the address of a heap copy pfun. The deleter of pfun erased the trace only from the compile cache of the thread that called compile(). Compile caches are thread-local. A trace that another thread recorded stayed in that thread's cache after pfun was freed:

  • The trace keeps its arrays alive (inputs, outputs, tape, captured constants) until that thread exits.
  • The key is a freed heap address. A later closure can get the same address. When that closure is called on the same thread with the same input shapes, dtypes and default stream, the cache returns the old trace and the old graph runs.

Fix: the deleter erases through the fork's cache registry (detail::compile_erase(fun_id), PR #10), which erases the key from every live thread cache. The erase runs before delete, so no new allocation can get the address while an old trace is still in a cache.

Note: the cross-thread registry (CompileCacheRegistry) is a fork addition (PR #10), not upstream code. Upstream has only the per-cache compile_erase(cache, fun_id).

Changes

  • mlx/compile.cpp: the pfun deleter calls detail::compile_erase(fun_id) instead of detail::compile_erase(cache, fun_id) with the creating thread's cache.
  • tests/compile_tests.cpp: test freeing a compiled closure erases every thread cache. It compiles a closure that captures an array, calls it once on a second thread (the trace goes into that thread's cache only), frees the closure while the second thread is still alive, and checks captured.is_uniquely_referenced(). A trace that stays holds the captured array, so the check fails. The array is built from host data and the worker does not evaluate the output, so no device work holds a reference. The test uses only existing API (array::is_uniquely_referenced, fork PR for buffer uniqueness). No test-only production API.
  • fork.yaml: section "Freed compiled closures leave every thread cache".

Not changed (open finding): the Python binding PyCompiledFun::~PyCompiledFun erases only the cache of the thread that called the function last. Same class of hole; out of scope for this PR.

Test first

Merge order

This PR is step 1 of a pin chain. Merge in this order, each with Create a merge commit (never squash; a squash makes the pinned SHAs unreachable from main, as with mlx #38 / mlx-c #17):

  1. Layr-Labs/mlx (this PR)
  2. Layr-Labs/mlx-c re-pin PR (GIT_TAG = this PR's head)
  3. Layr-Labs/mlx-swift re-pin PR (gitlinks = this PR's head and the mlx-c PR head)
  4. Layr-Labs/mlx-swift-lm re-pin PR (mlx-swift revision = the mlx-swift PR head)

Draft upstream issue (not opened)

compile(std::function): freeing a closure leaves its traces in other threads' compile caches

Since compile caches became thread-local, mlx::core::compile(std::function<...> fun, bool shapeless) handles a function with no addressable target like this: it copies fun into a heap pfun, uses pfun.get() as fun_id, and gives pfun a deleter that calls detail::compile_erase(cache, fun_id) with cache = detail::compile_cache() taken when compile() runs.

The compiled function can be called on any thread. Each call records the trace in the calling thread's cache. When the last copy of the compiled function is freed, the deleter erases fun_id only from the cache of the thread that called compile(). Traces on other threads stay until those threads exit.

Effects:

  1. The traces keep their arrays (inputs, outputs, tape, captured constants) alive. In a long-lived thread pool this is a leak that grows with each closure compiled on one thread and called on another.
  2. The key is a freed heap address. A new closure can get the same address. If it is called on a thread that holds the old trace, with the same input shapes, dtypes and default stream, CompileCache::find returns the old entry and the old graph runs in place of the new function.

Repro (C++, doctest): compile a closure that captures an array c; call it once on a second thread; keep that thread alive; free the compiled function on the first thread. c still has an extra reference (the second thread's trace). With the fix, c.array_desc_.use_count() == 1.

Possible fix: keep a process-wide registry of live thread caches (each CompileCache adds itself in its constructor and removes itself in its destructor), and make the pfun deleter erase fun_id from every registered cache before delete. Layr-Labs/mlx carries this registry (also used for the process-wide compile_erase(fun_id) / compile_clear_cache() that older bindings call) and this fix.

The Python binding has a related path: PyCompiledFun::~PyCompiledFun calls compile_erase(cache, fun_id) with the cache of the thread that called the function last (call_impl sets cache). fun_id is the address of the Python function object, which Python reuses freely.

🤖 Generated with Claude Code

davidtai and others added 2 commits October 24, 2026 10:44
compile(std::function) keys a closure by the address of a heap copy.
The deleter of that copy erases only the cache of the thread that
called compile(). A trace recorded on another thread stays after the
copy is freed. The new test records a trace on a second thread, frees
the closure, and checks that the captured array is uniquely referenced
again. It fails until the deleter erases through the cache registry.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
compile(std::function) keys a closure by the address of a heap copy.
Each call records its trace in the calling thread's cache, but the
deleter of the copy erased only the cache of the thread that called
compile(). Traces from other threads stayed after the free: they kept
their arrays alive, and a later closure at the same heap address could
replay them. The deleter now erases through the cache registry
(detail::compile_erase(fun_id)), before the delete.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@davidtai
davidtai marked this pull request as ready for review October 24, 2026 16:05
davidtai added a commit to Layr-Labs/mlx-c that referenced this pull request Oct 24, 2026
GIT_TAG moves from c51f9f32 (mlx main with ml-explore#40) to ff4bb35b, the head
of Layr-Labs/mlx#43. It is based on c51f9f32 and adds the fix that
erases a freed compiled closure from every thread compile cache. The
fork.yaml Core pin prose names the new pin.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
davidtai added a commit to Layr-Labs/mlx-swift that referenced this pull request Oct 24, 2026
Source/Cmlx/mlx moves from c51f9f32 to ff4bb35b, the head of
Layr-Labs/mlx#43 (a freed compiled closure is erased from every thread
compile cache). Source/Cmlx/mlx-c moves from f1a489c9 to 004c735f, the
mlx-c#19 head whose GIT_TAG is the same mlx commit.

tools/update-mlx.sh changes only the Metal.hpp commit stamp: #43
changes mlx/compile.cpp and a C++ test, not a JIT source or a header.

fork.yaml: the Submodule pins prose names the new pins.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
davidtai added a commit to Layr-Labs/mlx-swift-lm that referenced this pull request Oct 24, 2026
The Layr-Labs/mlx-swift revision moves from 8d927e6d to eedca444, the
new mlx-swift#63 head. It pins mlx ff4bb35b (Layr-Labs/mlx#43, a freed
compiled closure is erased from every thread compile cache, on top of
main with #40) and mlx-c 004c735f.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@davidtai
davidtai merged commit 22dc52f into main Oct 24, 2026
18 checks passed
@davidtai
davidtai deleted the fix/compile-erase-all-threads branch October 24, 2026 17:35
jonathan308 added a commit to Layr-Labs/d-inference that referenced this pull request Oct 24, 2026
…x ff4bb35b)

libs/mlx-swift-lm moves efd38585 -> d4fd516e, mlx-swift-lm #356's head
after it merges SDK main 476cb7c0. Main gained #358, which pins
Layr-Labs/mlx-swift eedca444 (was 6c31f9f1). That mlx-swift pins mlx
ff4bb35b and mlx-c 004c735f, which bring two fixes:

- float16 and bfloat16 floor divide computed in float32
  (Layr-Labs/mlx#40; CPU and the Metal binary_ops kernel);
- a freed compiled closure erased from every thread's compile cache
  (Layr-Labs/mlx#43).

libs/mlx-swift moves to eedca444 and libs/mlx to ff4bb35b in lockstep,
so the metallib is rebuilt from the new kernels. No Swift API changed:
the SDK's own change is Package.swift only, and mlx-swift changes its
pins and regenerated sources.
jonathan308 added a commit that referenced this pull request Oct 26, 2026
Brings in main at f78f0fa (2026-10-24): the upstream ml-explore/mlx sync
to afc03c5 (#38, with its history restored by #42) and the fork fixes
#40 and #43-#46.

Conflicts and how they were resolved:
- mlx/distributed/jaccl/lib/jaccl/mesh.cpp: main's per-wire queue setup
  (upstream ml-explore#4607, a second cable for the mesh) with this branch's
  COMPLETION_QUEUE_DEPTH for every completion queue.
- mlx/distributed/jaccl/lib/jaccl/mesh_impl.h: main's stride parameters
  for all_gather and sum_scatter, with this branch's progress-guard calls
  and stage_send in every send path.
- mlx/distributed/jaccl/lib/jaccl/ring_impl.h: this branch keeps its
  guarded member dispatch_wires (pool-wire errors are reported, the group
  closes after all wires return). Main moved an unguarded copy into
  threadpool.h and called it as dispatch_wires(pool_, ...); the ring call
  sites call the member again. The all_gather slice code is main's
  (identical to this branch's backport of upstream ml-explore#4443).
- quantized_nax.h, fp_quantized_nax.h: main's kernels. Main already keeps
  the revert of the upstream ml-explore#4352 simdgroup skip (no sg_active) and adds
  the ragged-K tail bound of upstream ml-explore#4009.
- fork.yaml: main's file, plus the JACCL description and section. The
  upstream backports this branch carried (ml-explore#4443, ml-explore#4557, ml-explore#4558, the
  scoped_env helper of ml-explore#4474) are now upstream ancestry, so their "not fork
  work" section and its references are gone. The JACCL section now says
  that a mesh with more than one wire has one guard per wire.

Also:
- python/tests/ring_test_distributed.py: main's file. The auto-merge had
  added test_strict_init_after_fallback twice.
- tests/jaccl: the mesh all_gather and sum_scatter calls pass main's new
  stride argument (equal to the chunk size, as main's own
  all_reduce_scatter_gather does).

Tests: tests/jaccl (simulated verbs, ASan and UBSan): 10 of 10 passed.
check_forkdiff.py --upstream-ref upstream/main: 111 changed files, all
described, base afc03c5 is the merge-base. clang-format 21: no changes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
jonathan308 added a commit that referenced this pull request Oct 26, 2026
…ectives branch

Brings in merge/33 at 187b6c6, which is main f78f0fa (the upstream
ml-explore/mlx sync to afc03c5 with its restored history, and the fork
fixes #40, #43-#46) merged into the JACCL send-frame and progress-guard
branch. This branch had merged main up to ad4745a; everything main added
since comes in here.

Conflicts and how they were resolved:
- mlx/fence.h, mlx/backend/{metal,cuda,no_gpu}/fence.cpp, mlx/transforms.cpp:
  main's value-based fence API (upstream ml-explore#4552: update returns a value,
  wait takes it) with this branch's fenced inputs and eager commit.
  Fence::wait_in_consumer now takes the value of the update it waits for,
  and FencedInput carries that value, so a fenced input never waits for a
  later update than its own. The Metal fence keeps the two-word buffer of
  the fused coherent update, now owned through main's allocator::Data.
- Metal fast fence wait: this branch orders the spin with
  wait_for_all_pending (the pass waits for every pass in flight). Upstream
  ml-explore#4689 gets the same order by putting the spin in a pass of its own
  (end_encoding() before it, end_encoding(true) after it). The merge keeps
  the spin in the current pass and adds a buffer barrier after
  wait_for_all_pending, so it also follows the earlier kernels of the same
  pass, as upstream's separate pass does (the same line that the next
  branch, fix/cluster-metal-fence-wait, adds). A wait adds no encoder
  boundary; tensor-parallel decode waits for a CPU-stream all-sum about
  twice a layer. end_encoding(bool wait_in_flight) stays and shares one
  loop with wait_for_all_pending.
- docs/src/usage/environment_variables.rst: both sides' entries
  (MLX_METAL_FUSED_FENCE_UPDATE, MLX_METAL_FENCED_INPUTS, MLX_CONV_WINOGRAD).
- fork.yaml: the merged JACCL branch's file plus this branch's sections
  (startup readiness, two-rank sealed all-sum, fused fence update, fenced
  inputs, cheaper custom-kernel launches). The fenced-input section now
  explains the fence-wait order against upstream ml-explore#4689 and the value-based
  waits. The "not fork work" backport section is gone (upstream ancestry).

Also adapted:
- mlx/distributed/jaccl/lib/jaccl/mesh.h: main's mesh has one MeshImpl per
  wire (upstream ml-explore#4607). The startup wait budget goes to every wire; the
  two-rank exchange of the streamed sealed sum runs on the first wire. The
  two-Mac cluster's device matrix has one device per peer, so it has one
  wire and nothing changes for it.

Unchanged by the merge: main's quantized, matmul and SDPA kernels and their
dispatch (this branch had no change there beyond what main already carried
since #37), main's revert of the upstream ml-explore#4352 simdgroup skip, and this
branch's revert of 4d91320 (main does not carry 4d91320; the footprint
policy keeps {page, page, 0, 0, page}).

Tests (Ultra A, M5 Ultra; Debug build with Metal, 8 jobs):
- tests/tests on the GPU: 310 of 316 cases passed. The 6 failures (linalg
  QR, inverse, Cholesky, pseudo-inverse, eigh, LU) fail the same way on
  main f78f0fa on this machine.
- tests/tests with DEVICE=cpu: 316 of 316 passed.
- ctest process_allocation_cap: 15 of 15 passed.
- check_forkdiff.py --upstream-ref upstream/main: 137 changed files, all
  described, base afc03c5 is the merge-base.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
jonathan308 added a commit that referenced this pull request Oct 26, 2026
…e fence-ordering branch

Brings in merge/35 at 2b5253a: main f78f0fa (the upstream ml-explore/mlx
sync to afc03c5 and the fork fixes #40, #43-#46) through the JACCL and
cluster-collectives branches, with their conflict resolutions. This
branch's tip is d11238f (32 x 32 steel tiles where kernels are
precompiled).

Conflicts and how they were resolved:
- mlx/transforms.cpp: this branch's event fences (a GPU stream never spins
  on another GPU stream, nor on CPU work that can wait for one) with main's
  value-based waits (upstream ml-explore#4552). FenceNeed records the value of each
  of its two updates (fast_value, event_value); every wait uses the value of
  the update it waits for, fast and event fences alike, and a fenced input
  gets the fast value. Waits look the fence up with at(), as main does.
- mlx/backend/{metal,cuda,no_gpu}/fence.cpp: Fence(Stream, bool fast) next to
  the value-based wait. The Metal fence-wait barrier was already in the
  merged collectives branch (the same line), so it merged cleanly.
- mlx/backend/metal/quantized.cpp (qmm): this branch's affine steel tiles
  (MLX_QMM_STEEL_TILE, 64 x 32 / 32 x 64 for prompts) beside main's FP4
  half-loader 64 x 64 tile (upstream ml-explore#4709). The modes do not overlap:
  wide affine tiles keep the "_bm<m>_bk<k>_bn<n>" name, the FP4 tile keeps
  main's precompiled "_bm_64_bn_64" name. The wide-tile special case is gone:
  main's affine_qmm_t has has_global_scale before BM, BK, BN, so every
  transposed qmm now passes (global scale, bm, bk, bn) in main's order. The
  old call would have instantiated affine_qmm_t<..., bm, bk, bn> into the
  wrong template parameters. d11238f's kernels_jit_compiled() gate is kept.
- mlx/ops.cpp: main's 1-bit affine_quantize branch, with this branch's
  caller stream on the scales select.
- mlx/distributed/sealed.cpp: this branch's __APPLE__ guard around the
  dispatch include.
- tests/CMakeLists.txt: metal_fence_chain beside main's GPU_TEST_SOURCES
  (renamed from METAL_TEST_SOURCES upstream), which now also lists
  d11238f's quantized_tile_tests.cpp.
- .github/workflows/fork-tests.yml: main's restructured workflow; the
  bounded Metal fence chain step now runs in the build-cpp job after the
  standalone allocation cap tests (main has no single build-and-test job
  any more).

fork.yaml merged without conflicts (this branch's sections inside the
merged file). The steel-tile section now also lists kernels.h,
jit_kernels.cpp and nojit_kernels.cpp: main dropped the "Upstream v0.32.2
sync" section that described them on this branch, and d11238f's
kernels_jit_compiled() lives there.

Tests (Ultra A, M5 Ultra; Debug build with Metal, 8 jobs):
- tests/tests on the GPU: 315 of 321 cases passed. The 6 failures (linalg
  QR, inverse, Cholesky, pseudo-inverse, eigh, LU) fail the same way on
  main f78f0fa on this machine: TF32 is on by default on a NAX GPU. With
  MLX_ENABLE_TF32=0: 321 of 321 (with quantized_tile_tests).
- tests/tests with DEVICE=cpu: 321 of 321 passed (with the sealed stream
  tests).
- ctest process_allocation_cap: 15 of 15; metal_fence_chain (fast synch,
  8/50/1024 ops a buffer): 3 of 3.
- check_forkdiff.py --upstream-ref upstream/main: 144 changed files, all
  described. clang-format 21: no changes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants