Conversation
…ablup#2096) KVCache::trim returned 0 for a pool-backed cache, so the pad rows a tile-padded prefill writes on M5 stayed in the pool and `offset` stayed at the padded length. Decode then attended over the pad rows and rotated the next token at the padded position, which made the default (paged) server return wrong greedy output for any prompt whose length is not a multiple of 32. trim now rewinds the pool block table through the cache's own backing and moves `offset` by the count it removed. The decode lookahead teardown routes through the same call; its pool-only branch rewound the block table and left `offset` one ahead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 tasks
KVCache::trim for pool-backed caches
This branch has not been deployed
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
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.
Summary
KVCache::trimreturned0for a pool-backed cache, so the pad rows a tile-padded prefill writes on M5 stayed in the pool andoffsetstayed at the padded length. Decode attended over the pad rows and rotated the next token at the padded position, which made the default (paged) server return wrong greedy output for any prompt whose length is not a multiple of 32.trimnow rewinds the pool block table and movesoffsetwith it, and the decode lookahead teardown routes through the same call.Related issues
Closes #2096. Sibling of #1755 (the model-owned variant of the same pad trim). Refs #1760 for the replay abort described under "Not fixed here".
Type of change
feat— new user-visible featurefix— bug fixperf— performance improvement (include before/after numbers in the PR body)refactor— internal restructuring without behavior changechore— build, CI, dependencies, release infrastructuredocs— documentation onlytest— tests onlyWhat changed
src/lib/mlxcel-core/src/cache.rs: for a pool-backed cache,KVCache::trimcallsPagedBlockPool::rewind_tokensthrough the cache's own backing and subtracts the removed count fromoffset. The four pad trims inscheduler/prefill.rsand every speculative rollback already calltrimand expect a real one; none of them pairs it with a pool rewind.src/server/batch/scheduler/decode_tick.rs:apply_lookahead_trimdrops its pool-only branch and trims every cache throughKVCache::trim. The branch rewound the block table withrewind_paged_tokensand leftcache.offset, which is whereBatchedAttentionMetadatatakes the RoPE offset from, one ahead.src/lib/mlxcel-core/src/cache/paged_batch_decode_tests.rs:trim_drops_padded_prefill_rows_from_a_pool_backed_cache, checkpoint-free. It writes 64 rows, trims 27, and asserts the returned count,offset, the poollen, and that the nextupdate_and_fetchreturns the 37 real rows plus the new token at row 37. On the pre-fix code it fails at the first assertion (left: 0, right: 27).Real-checkpoint validation
Apple M5 Pro, macOS 27.0,
temperature 0,max_tokens 24, five prompts per run, fresh server per run. "default" ismlxcel-server -m <model>with no other flags; the reference is--decode-storage-backend dense.mlx-community/Llama-3.2-1B-Instruct-4bitHuggingFaceTB/SmolLM2-135M-Instructmlx-community/Qwen3-0.6B-4bitmainat 9a0be04 is wrong in the same way on the prompts I repeated there: SmolLM2 returns the same text as below, and Llama-3.2-1B answersRepeat: one two threewithOne!Two!Three!(dense:One two three.).cache.rschange, concurrent was 4 of 5 on Llama-3.2-1B and SmolLM2. On Llama-3.2-1B it was 5 of 5 withMLXCEL_FORCE_SYNC=1, and 5 of 5 once the lookahead teardown used the same trim.contentis mostly empty because it is still reasoning.SmolLM2-135M,
What is the capital of France?, default flags:Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --features metal,accelerate -- -D warningscargo test --workspace --profile test-fast --features metal,accelerate --no-fail-fast -- --test-threads=1: 11818 passed, 4 failed. The same 4 fail on unpatchedmainin this environment, see below.cargo deny check(not installed on this host; not run)The 4 failures, identical before and after this change:
fp8_block_requantize_round_trip_stays_within_half_an_e4m3_step,mxfp4_gather_qmm_matches_host_reference,mxfp8_gather_qmm_matches_host_reference. This host has Command Line Tools only and no Metal shader compiler, so themainbuild linked the v0.7.0 releasemlx.metallibinstead of compiling the kernels atmain's MLX pin. These three exercise kernels that changed between the two pins.speculative::prompt_lookup::tests::warmup_runs_every_verify_width_and_rolls_each_backrecords a first forward width of 32 instead of 5 on M5. It uses a denseKVCache::new(), which this change does not touch.Because of that build caveat I also applied the same change to a v0.7.0 source build, where the release metallib matches exactly, and it fixes the same reproduction there.
Notes for reviewers
[broadcast_shapes] Shapes (32,63) and (1,9,32,64) cannot be broadcast). That is the whole-prompt hit of fix(prompt-cache): a whole-prompt cache hit re-runs the last token on top of a cache that already holds it #1760; the evidence is in fix(server): pool-backed KVCache::trim is a no-op, so tile-padded prefill leaves pad rows in the cache and default M5 serving returns wrong greedy output #2096 under "Related".prefill.rs:700) pads to the longest row in the window on any hardware, so this should also change output there for concurrent prompts of different lengths under the paged backend; I have no such host.CachePool::rewind_paged_tokenshas no caller left outside tests. I left it in place.models/. The unit test pins thetrimcontract both call sites now share.WebUI installed artifact. Its activity-performance gate reportedtwo-visibleasinvestigatebecause the baseline CV was 5.10% against a 5% budget; the median decode degradation was -0.25%. The same gate failed onmainin runs 36961726625 (baseline CV 6.2%) and 36735057944 (degradation 2.7%). I could not re-run it from a fork, so I mergedmain(c0b7134) into the branch; the run on that head is green.Checklist
feat:,fix:, etc.)docs/if user-facing behavior or supported models changed (no flag or documented behavior changes)// Used by: ...comments on any shared function I modified (seedocs/code-guidelines.md).envfiles committed🤖 Generated with Claude Code