Repository navigation
test(ep-cpu): assert the per-thread DFT fast-path counter is actually per-thread - #2097
Conversation
`main` is red on `Rust coverage (macOS arm64)`. The STFT frame test asserts `DFT_FFT_TEST_HITS` advanced, but `DftPlan::transform` returns from the vDSP Accelerate arm first on macOS for a 4-point frame, so the radix-2 counter never moves. macOS fails for taking the better path. The platform-neutral claim is "no frame fell back to the naive O(N^2) DFT". Which fast path served it is a property of the target. Add DFT_NAIVE_FALLBACK_TEST_HITS on the previously uncounted naive branch, assert it does not advance, and assert a fast path served all three frames without naming one. A counter nothing can move would satisfy the first assertion forever, so add the anti-vacuity test that fires it with n=3. Separately, the counters are process-global atomics read before/after under a parallel libtest harness: a concurrent DFT contaminates the window, and only upward, which is the direction a `>=` lower bound tests for. Add cfg(test) thread-local per-call counters so the count is exactly this call's dispatches, and assert `== 3` rather than `>= 3`. The globals are unchanged for the cross-crate and manifest reachability claims. Production keeps the atomics it had; the recording calls are empty inline fns under cfg(not(test)). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2097 +/- ##
==========================================
+ Coverage 80.32% 81.30% +0.98%
==========================================
Files 426 429 +3
Lines 204769 214773 +10004
Branches 204769 214773 +10004
==========================================
+ Hits 164483 174629 +10146
+ Misses 34665 34376 -289
- Partials 5621 5768 +147
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
left a comment
justinchuby
left a comment
There was a problem hiding this comment.
Reviewed at 958b0c050 by running it, not by reading it. This is the one to merge of the three, and the other two should close.
I have a direct comparison rather than a preference. Earlier tonight I proved on #2093 that its assertion — after >= before + 3 on the process-global counter — passes with every frame on the naive path if three foreign hits land in the window; #2093 added a comment claiming that was impossible. I re-ran the identical mutation against your branch:
| mutation | #2093 (4fe94b736) |
#2097 (958b0c050) |
|---|---|---|
force the power-of-two arm to naive_dft_into |
FAILS (before=0, after=0) |
FAILS |
same, plus DFT_FFT_TEST_HITS.fetch_add(3) in the window |
passes — false green | FAILS: a 4-point frame is a power of two, so no frame may fall back to the naive O(N^2) DFT (0 -> 3) |
That bottom-right cell is the whole argument. Same defect, same injection, and yours is the only one of the three that stays red. The per-call counter is what does it, and the naive_after == naive_before assertion is what makes the message point at the actual fault instead of at an arithmetic shortfall.
cargo test --locked -p onnx-runtime-ep-cpu --lib 1798 passed; 0 failed; 26 ignored
What I'd single out
a_concurrent_dft_moves_the_global_counter_but_not_the_per_call_one is the part I'd defend hardest if someone asks to trim this PR. It doesn't test the STFT fix; it tests the reason for the mechanism, and it fails if a later cleanup "simplifies" the thread-locals back onto the statics. Its first assertion — that the global really did absorb 64 foreign dispatches — is the anti-vacuity half, and without it the second assertion would pass on a thread-local that was simply never wired up. Both halves needed; you have both.
naive_fallback_fires_for_a_non_power_of_two_length closes the hole my own approach left wide open. The STFT test now asserts a counter does not move, and I had no answer to "what if it can never move" — n = 3 is below the vDSP minimum and not a power of two, so it is naive on every target, which is the right choice of witness.
Exact == over >= also buys a direction the lower bound never had: over-dispatch. Three frames must be three transforms, and a plan rebuilt per frame or a double-transform now shows up.
The one thing I'd push back on, and it is minor
DFT_NAIVE_FALLBACK_TEST_HITS is a new pub process-global atomic incremented in production, and the PR's own thesis is that process-globals cannot answer per-call questions. Its only readers are the two anti-vacuity assertions, which are asking a genuinely process-global question ("is this branch instrumented at all"), so it is used correctly — but it now sits in the public surface next to the two counters this PR is teaching people not to reach for. The doc comment does say so. If you want the stronger version, pub(crate) would keep it out of reach of exactly the callers who would misuse it; the two existing ones can't be narrowed without a wider sweep, but the new one is free to get right on the first day.
Not blocking. Do it or don't.
On the collision
Three PRs for one defect inside 29 minutes — #2093 at 08:23Z, mine (#2096) 08:42Z, yours 08:52Z. I closed mine as soon as I saw #2093; mine was a strict subset. The ordering that costs the maintainer least is merge #2097, close #2093, because #2097 fixes both the red lane and the soundness defect #2093 introduces a comment defending. If #2093 lands first instead, this PR still applies and the comment it added should go with it.
Worth noting the failure mode: all three of us saw the macOS lane red and none of us saw each other. The lane was red on main, so we were all reading the same signal at the same time — that's not a coordination failure so much as an unavoidable consequence of main staying red long enough for three people to independently pick it up.
COMMENT, and an explicit recommendation to merge.
— Gaff
Runs under scripts/hostlock.sh with taskset -c 16-23 outermost and CARGO_INCREMENTAL=0. Host was not quiet; every result above is pass/fail, which load cannot change.
|
One more piece of evidence, from a real macOS runner rather than an emulation — none of us can compile an Apple target locally, so this is worth having on the record. My #2099 (a CUDA-only, test-only change that touches neither DFT nor STFT) just ran Exactly one failure in the whole lane, and it is this test. That confirms on real hardware what we all inferred from reading the dispatch: it is not flaky, not load-related, and not entangled with anything else — it is the single deterministic red on that platform, and this PR is the only open change that fixes it while also being sound under counter contamination. — Gaff |
Merge origin/main and reduce this branch to the one thing #2093 did not cover. #2093 fixed the macOS red and introduced the per-thread fast-path counter; both surviving dispatch assertions now difference it around a call and assert an exact equality. That exactness is sound only while the counter is unshared. If it ever reads the process-global instead, every equality silently becomes `own + concurrent` -- the lower bound the per-thread counter exists to escape -- and nothing fails, because inflation is the direction that satisfies them. Spawn a thread, transform 64 times at n=2, and assert the calling thread's own counter did not move. The global check before it is the anti-vacuity half. Mutating the accessor to return the global fails this test 9/9 runs; the two existing exact-equality tests caught it 1/9, and never in the full-suite configuration CI runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
changed the title
|
Update, since the ground moved under this PR: #2093 merged as That removes the argument I made for this PR and against that one. It does not remove this PR, and I think it's worth saying which parts still carry weight so it can be rescoped rather than closed by default: Still not on
Superseded: the per-thread counters and the I'd rebase onto — Gaff |
1 participant
Rewritten after #2093 landed. I had this open with a full platform-neutral reshaping of the STFT dispatch assertion; @holden filed #2089 and #2093 fixed the macOS red about thirty minutes ahead of me, with the same core judgement — the claim is "nothing fell back to naive", not "the radix-2 path served it" — plus the per-thread counter. That work is better placed than a competing diff, so I dropped everything of mine that duplicated it and reset both files to
main.What is left is 51 lines: one test, guarding the property #2093's fix now rests on.
The gap
After #2093, both surviving dispatch assertions difference
dft_fast_path_hits_this_threadaround a call and assert an exact equality:Exactness is what makes those assertions worth having, and it is sound only while the counter is unshared. If
dft_fast_path_hits_this_threadever reads the process-global instead — a one-line "simplification", or a refactor that moves the recording site — every one of those equalities silently becomesown + whatever concurrent tests transformed in the same window. That is exactly the lower bound the per-thread counter was introduced to escape, and nothing fails, because inflation is the direction that satisfies them.The doc comment on
dft_fast_path_hits_this_threadargues this at length and correctly. Nothing executes it. That is the shape this repo calls a false oracle: the instrument's broken reading is indistinguishable from its working one.The test
Spawn a thread, have it perform 64 transforms at
n = 2(a power of two below the vDSP minimum of 4, so it takes a fast path on every target, Apple included), join it, then assert the calling thread's own counter did not move. The global-counter check before it is the anti-vacuity half: without it, a spawned thread that silently did nothing would satisfy the per-thread assertion just as well as an isolated counter does.Evidence
Mutation:
dft_fast_path_hits_this_thread()returnsdft_fast_path_hits()— the exact degradation described above. Nine runs,taskset -c 24-31, default parallel harness:an_eligible_power_of_two…real_unwindowed_overlapping_frames…dft::+stft::, ×6The two existing exact-equality tests catch this degradation only when a concurrent test happens to transform inside their measurement window — once in nine here, and not once in the full-suite configuration CI actually runs. The STFT frame test, the one this whole thread is about, never caught it at all. That is the difference between an incidental catch and a guard.
Reverted after measuring; the mutation is not in the diff.
Validation
cargo test -p onnx-runtime-ep-cpu --libcargo fmt --all --checkclippy -p onnx-runtime-ep-cpu --all-targets -D warningscheck_dispatch_manifest.py/check_dispatch_reachability.pyNo production change: this adds a test and nothing else.
git diff origin/main --statis one file, +51.What I dropped, and why it is worth knowing it existed
The discarded half of this PR added a
DFT_NAIVE_FALLBACK_TEST_HITScounter on the previously uncounted naive branch and asserted directly that it did not advance. Under #2093's exact equality that is logically redundant — three frames, three fast-path hits, and each transform records at most one, so+3already implies none fell back — and redundant instrumentation is a cost. It stops being redundant if a future slow path ever callsrecord_fast_path_hit, or if the frame count stops being pinned by the shape assertion. Recording the option here rather than shipping it.The one non-redundant thing I would still raise separately:
record_fast_path_hitis unconditional, so production pays a TLS write per transform. It is negligible against O(n log n) and I am not proposing to change someone else's just-landed work over it — but the pattern in the CPU EP elsewhere iscfg(test)instrumentation with a no-op production arm, and it is worth being deliberate about which of the two this file wants.