Make the ParallelGuard depth thread-local - #276
Conversation
`PARALLEL_DEPTH` was a global `AtomicUsize`, so any thread entering a guarded region made every `step_resolution` job in the pool bounce off `is_in_parallel` and re-queue itself, even though those jobs would have run on threads that were free to take long work. The inversion the guard protects against is narrower than that: it happens only when a thread blocked inside a guarded `par_iter_mut` steals a long job and so cannot resume its parent. Stolen jobs run on the thread that stole them, so a thread-local depth is exactly the condition to test. Other threads keep picking up long jobs while a guarded region is active. Replaces the atomic with a `thread_local!` `Cell<usize>`; the guard holds an `EnteredSpan` and is therefore `!Send`, so it always drops on the thread that incremented the counter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B8hUmfq86DwNurgutaiL8e
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughParallel depth tracking in ChangesParallel depth tracking
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PARALLEL_DEPTHwas a globalstatic AtomicUsize, so any thread entering a guarded region made every pendingstep_resolutionjob in the pool observeis_in_parallel() == trueandsend_retryitself back to the scheduler — including jobs that were about to run on threads with no guard active and plenty of capacity for long work.The inversion the guard actually protects against is narrower than that. It happens only when a thread blocked inside a guarded
par_iter_mutsteals a long job and therefore cannot resume its parent. Stolen jobs run on the OS thread that stole them, so a thread-local depth is exactly the right condition to test. With this change, other threads keep picking up long jobs while a guarded region is active.Changes
ext/src/utils.rs—PARALLEL_DEPTHbecomes athread_local!Cell<usize>.ParallelGuard::new/Dropbump the calling thread's own counter andis_in_parallel()reads only that thread's value.No changes were needed at the three call sites (
ext/src/resolution.rs:764,ext/src/resolution.rs:836,ext/src/nassau.rs:960) — the predicate is unchanged, just correctly scoped.The guard holds a
tracing::span::EnteredSpan, which is!Send, soParallelGuardis!Sendtoo and always drops on the thread that incremented the counter. The counter therefore cannot underflow. This reasoning is recorded in a doc comment on the struct.This is a scheduling optimization only — it has no effect on computed results.
Verification
cargo build,cargo clippy --all-targets(clean),cargo fmt --check(clean)test_tempdir_lockintests/save_load_resolution.rsfails identically on unmodifiedmaster(verified by stashing and re-running). It is ashould_panictest expecting a file-lock conflict that does not materialize when the suite runs as root in a container, and is unrelated to this change.Related
The
hpcbranch approaches the same symptom differently:71a21b6removesutils::paralleloutright, on the grounds that under the relaxed dependency graph many more step jobs are ready at once and the global counter turned that into a retry storm, while the heavy nassau matrix builds are now GPU-offloaded. That retry storm is the same problem this PR fixes at its root, so it may be worth revisiting whether the removal is still preferable to keeping the thread-local version ifhpcmerges back.🤖 Generated with Claude Code
https://claude.ai/code/session_01B8hUmfq86DwNurgutaiL8e
Generated by Claude Code
Summary by CodeRabbit