perf(apple): the low-precision MoE path jumped the arbiter and was 12-60x slower - #720
Merged
Merged
Conversation
Found while attributing a pre-existing flake: an Apple sweep failed 2, then 21, then 62, then 28 tests across four runs of the same command, every one with the same signature -- `dispatch returned False; last_error_kind=0`. Two separate defects in tessera_apple_gpu_mlpkg_dispatch produced it. **The race.** `mtl4_shared_queue` is shared by every packaged dispatch, and this lane deliberately does not take `mtl4_dispatch_mu` (fresh allocator + command buffer per call). But `commit:` and `signalEvent:value:` are two separate queue operations, and unlocked, thread B interleaves between A's commit and A's signal -- so A's signal no longer denotes A's buffer. The context-wide `mlpkg_event` HID this. Any thread's signal satisfied any thread's wait, so a waiter could return on another dispatch's completion and read its outputs while its own command buffer was still running. Switching to a private event exposed it at once as a wait that never completes, which is the truthful symptom: the signal really was not arriving for that buffer. The fix is both halves -- a per-dispatch event, and commit+signal under one lock. The wait stays outside the lock, so dispatches still overlap on the GPU. This is the same correction 7079e95 already made to `commit_and_wait_with_timeout` and its MPSGraph sibling, with the reasoning recorded there. The packaged lane was missed. **The silence.** The function returned 0 from ten places and set the last-error channel from none, so every failure reached the caller as `last_error_kind=0` -- true, useless, and indistinguishable between "no ML encoder on this SDK" and "the device stopped answering". Each path now names itself: kind 2 for an ordinary per-op failure, kind 1 for the timeout, which is the kind that feeds the Python dispatch breaker, so a wedged device now stops being asked instead of paying this timeout once per packaged dispatch. The timeout path also quarantines pooled buffers, as the sibling waits do. Evidence, M1 Max, unsandboxed, 60 vs 20 fresh processes of the test written for exactly this race (test_apple_mlpkg_concurrency, 4 threads x 8 dispatches): unmodified runtime 18 pass / 2 fail / 0 hang (20 trials) fixed runtime 60 pass / 0 fail / 0 hang (60 trials) At the baseline's 10% rate, 60 clean trials has probability ~0.0018. The eight files that failed in the original sweep now run 101 passed / 0 skipped, and the reproducer's failure count stopped varying run to run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… fix Fourth fingerprint on this stack. Re-recorded on this M1 Max with --profile extended: same 18 decisions, zero routes moved, because fixing a race changes what happens under CONTENTION, not which route is faster. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sealed on this M1 Max after the packaged-dispatch race fix; e2e_fleet dashboards regenerated to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-60x slower
The MoE SwiGLU composite had three implementations and no single place that
chose between them: single_fused, lowp and composed were fall-through blocks
each ending in `except Exception: pass`, so which one ran was not predictable
from the inputs and a failure between them was invisible.
The defect is narrower and worse than untidy structure. `lowp` sat AHEAD of the
arbiter and preempted it unconditionally for any uniform f16/bf16 operand, so
production_route_for never got to decide for the common low-precision inference
shape. Measured on this M1 Max (best of 5 after warm-up, ms):
(T,K,H,N,E) dtype single_fused lowp composed
(64,128,256,128,4) f32 13.23 -- 1.27
(64,128,256,128,4) f16 -- 15.04 1.26
(256,256,256,256,8) f32 32.00 -- 1.89
(256,256,256,256,8) f16 -- 35.66 1.84
(1024,512,512,512,8) f16 -- 1571.06 26.31
composed wins every case and the gap widens with size. It is also the more
accurate: 6.3e-8 relative error against an fp32 reference where lowp is 2.6e-4,
because composed accumulates in fp32. So the low-precision default was ~12-60x
slower AND ~4000x less accurate than the route it displaced -- and lowp has no
ledger row at all: preferred by default, never measured into the arbiter.
The fix restores the arbiter rather than removing it. The first attempt deleted
the production_route_for call, which test_moe_dispatch_consumes_the_strict_exact_row
caught and was right to: that would have made the ledger row a declaration
nothing reads (Decision #29) and "fixed" a slow route by deleting the mechanism
whose job is to choose between routes (Decision #28). Checking the ledger
settled it -- on the committed ledger it answers `composed`, correctly. It was
never the problem.
_apple_moe_select_route is now the one selection point, returning
(route, reason). The arbiter decides between the routes it has evidence about;
lowp is opt-in until it earns a ledger row; quant outranks everything. Choosing
a route measured slower than the default lands in the dispatch fallback log
under the op's name (Decision #21).
M1 Max: 10 new tests; full sweep 3 failed / 16568 passed, and none of the three
is from this change -- two are the separate process-wide telemetry defect and
one is my build directory being named build-apple. mypy clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0626620507
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CI (Linux, no Metal) failed with `Apple GPU back-half is Darwin-only`. test_choosing_a_slower_route_is_recorded_not_silent is the only one of the ten that actually dispatches; it now carries `hardware_apple_gpu`, the marker its siblings in test_moe_swiglu_block.py use and that tests/conftest.py enforces. The other nine stay unmarked deliberately, and that they run on a host with no Metal at all is the point of the change: moving selection out of the dispatcher is what makes the decision testable without a device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Found while investigating the MoE telemetry flake. It is a separate defect from #719 and needs no runtime edit or ledger re-seal.
The finding
The MoE SwiGLU composite had three implementations and no single place that chose between them.
single_fused,lowpandcomposedwere fall-through blocks, each ending inexcept Exception: pass, so which one ran was not predictable from the inputs and a failure between them was invisible.The defect is narrower and worse than untidy structure:
lowpsat ahead of the arbiter and preempted it unconditionally for any uniform f16/bf16 operand, soproduction_route_fornever got to decide for the common low-precision inference shape.Measured on this M1 Max, best of 5 after warm-up, milliseconds:
composedwins every case and the gap widens with size (10× → 17× → 60×). It is also the more accurate: 6.3e-8 relative error against an fp32 reference wherelowpis 2.6e-4, becausecomposedaccumulates in fp32.So the low-precision default — the common inference path — was about 12–60× slower and 4000× less accurate than the route sitting directly below it. And
lowphas no ledger row at all: preferred by default, never measured into the arbiter.The fix restores the arbiter rather than removing it
My first attempt deleted the
production_route_forcall. The suite caught it (test_moe_dispatch_consumes_the_strict_exact_row) and was right to — that would have made the ledger row a declaration nothing reads (Decision #29), and would have "fixed" a slow route by deleting the mechanism whose job is to choose between routes (Decision #28).Checking the ledger directly settled it: on the committed ledger it answers
composed, correctly. It was never the problem. The problem was a path wired ahead of it._apple_moe_select_routeis now the one selection point, returning(route, reason):lowpis opt-in (TESSERA_APPLE_MOE_LOWP=1) until it earns a ledger row;quantoutranks everything — per-GEMM quantization is the one thing the single-kernel paths cannot express;except Exception: passswallows are gone, so a failure inside a chosen route is a diagnostic.Do we need all three pathways?
On this evidence, no — but keep them reachable. Neither alternative earns a default. Both are waiting on rewrites that could change that: the fused kernel is one-thread-per-token and needs a threadgroup-cooperative version, and
lowp's single command buffer may win on a resident streaming lane where the composed path's three dispatches dominate. The rule this sets is that such a path must be measured into the arbiter, not wired ahead of it.Tests
tests/unit/test_moe_route_selection.py(10): composed is the default for every dtype;quantoutranks the opt-ins; both alternatives are reachable but only by name; an opt-in the shape cannot honour says so; every reason is non-empty; choosing a slower route is recorded; the arbiter is still consulted (with the op and shape key asserted); an unreadable ledger is non-fatal with a reason; a ledger choice out of the fused kernel's range is declined rather than dispatched into.Timings are deliberately not asserted — that would be a flaky perf gate on shared CI. The measurement lives in the code comment and the Apple queue entry, where it can be re-run deliberately.
Sweep
M1 Max, unsandboxed: 3 failed / 16568 passed, and none of the three is from this change — two are
test_lowp_moe_composite_execute_compare, the separate process-wide telemetry defect (an unrelated op reports no device time either), and one istest_resolver_finds_repo_built_opt_without_env_or_path, which fails because my build directory is namedbuild-apple. mypy clean.🤖 Generated with Claude Code