[MLX] Add options for clearing context#20609
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20609
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 87 PendingAs of commit 0570950 with merge base 0f3303f ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude review this code |
|
Claude finished @metascroy's task in 3m 40s —— View job Code Review: MLX context-clearing optionsI reviewed all 7 changed files. This is a well-constructed PR — the safety gating around Correctness / robustness1. The backend trusts the caller to only set this option when a multi-session owner (
In this PR it's correctly gated (the gemma runner only sets it under 2. Two independent
Looks correct (verified)
Minor
Summary: solid PR. Main suggestion is to make the |
This PR needs a
|
| if (h->clear_cache_interval_ > 0) { | ||
| std::lock_guard<std::mutex> lock(mlx_global_mutex()); | ||
| if ((++h->execute_count_ % h->clear_cache_interval_) == 0) { | ||
| ::mlx::core::clear_cache(); |
There was a problem hiding this comment.
Here's a comment by claude; not sure if valid or not 😅
On clear_cache_interval — consider set_cache_limit and the per-handle vs. global granularity
The periodic clear_cache() works, but I think it's worth reconsidering both the mechanism and where the knob lives, because mlx::core::clear_cache() (and set_cache_limit) operate on the process-global singleton allocator, while this option is keyed per-model/per-handle.
Two consequences of that mismatch:
- The cadence isn't really "every 256 forwards." Each handle carries its own execute_count_ / clear_cache_interval_, but they all flush the same global pool. With prefill + decode methods (and multiple sessions), the effective flush rate is the sum of all handles' executes, and one handle's flush dumps buffers another handle is about to reuse. The global mutex makes this safe, but the semantics are a bit incoherent — a per-handle counter driving a global side effect.
- It bounds resident-average, not peak. Between clears the cache can still grow to MLX's default ceiling (≈ the whole memory limit), and each flush is followed by a cold-cache re-allocation spike.
set_cache_limit(small) is arguably a better fit here: it's a global ceiling enforced lazily via LRU eviction on the next malloc, so it (a) is correctly a process-global setting, (b) bounds peak cache footprint continuously, (c) targets exactly the stale distinct-size buffers that accumulate from varying prefill/decode shapes, and (d) avoids the periodic flush/re-alloc spike. The tradeoff is it needs a tuned value (too small → lost reuse / prefill↔decode thrash).
Suggestions, in rough order of preference:
- Switch to a set_cache_limit-style cap (could reuse the same runtime-spec plumbing, just a different key/units), or
- If you keep clear_cache(), apply the cadence once globally rather than per-handle (e.g. drop the per-handle counter, or document that the real cadence is aggregate), and note in the comment that peak between clears is still the default ceiling.
Either way, a one-line note in backend_options.h that these knobs affectt just this delegate handle) would save the next reader some confusion.
There was a problem hiding this comment.
Good point on the per-handle/global mismatch; I've kept clear_cache() (it mirrors mlx-lm's own generate_step, which calls mx.clear_cache() every 256 tokens, mlx_lm/generate.py:467) and added comments in backend_options.h and the execute() flush site clarifying that the flush is process-global while the counter is per-handle (exact cadence with one handle, aggregate with several) and that it bounds resident-average, with set_wired_limit/cache-limit noted as a possible future peak cap.
|
@claude review this code |
|
Claude finished @metascroy's task in 1m 41s —— View job Code Review: MLX context-clearing options (round 2)Re-reviewed the current state of the branch against Worth confirming1. constexpr int kMLXClearCacheInterval = 1;The PR description says " Looks correct (verified)
Minor
Summary: the round-1 concerns are resolved well. The only thing I'd block on is confirming |
Adds two per-model MLX backend runtime options to bound and reduce memory during long/multi-session serving. clear_cache_interval makes MLXBackend::execute() call mlx::core::clear_cache() every N forward calls (counter on the handle, guarded by the global mutex so no atomic is needed), and skip_mutable_buffer_init lets a multi-session load skip allocating the handle's default mutable-buffer (KV/recurrent state) copy, which is dead weight once mlx_mutable_state.h allocates per-session buffers — eliminating one full KV-cache copy. Both are delivered per-delegate via the existing LoadBackendOptionsMap/get_runtime_spec path and exposed through a shared backends/mlx/runtime/backend_options.h. Skipping is gated for safety: a new auto-generated for_each_tid walker (added to generate.py + the MLXLoader templates, covering all op nodes) powers init_chain_references_mutable_buffer(), and init() errors out cleanly if skip_mutable_buffer_init is set while the program's init chain references mutable buffers. mutable_state_bytes_per_session is rebased onto program metadata (with a per-tensor size clamp to avoid overflow from malformed payloads) so capacity accounting stays correct when default buffers are skipped. Finally, the gemma4_31b runner enables these in build_gemma_module: clear_cache_interval is hardcoded to 1, and skip_mutable_buffer_init is set automatically when max_sessions > 1.