Feat/isolated pool parallelism v2#521
Conversation
Instead of processing tables in chunks on the global Rayon pool (where cross-table work-stealing causes contention), partition tables into S groups using greedy LPT scheduling and give each group its own isolated Rayon ThreadPool with cores/S threads. Key changes: - shard_parallelism(): configurable S (default cores/8, env SHARD_PARALLELISM) - partition_tables_by_cost(): greedy LPT by rows * columns - std::thread::scope + pool.install() for per-partition isolation - SendPtr for disjoint transcript access across partitions - Consumes cached LDEs via build_round1 (no recomputation)
|
/bench 3 |
Codex Code Review
|
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: 8e25d7d · Baseline: cached · Runner: self-hosted bench |
Review: Feat/isolated pool parallelism v2OverviewReplaces the chunk-based rayon parallel loop with an isolated thread-pool architecture for Rounds 2-4. Tables are partitioned using greedy LPT scheduling into Issues[High] Thread panics bypass error handling — [Medium] [Medium] Thread pools are created fresh per [Low] Cost formula mismatch in [Low] |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| unsafe impl<T> Send for SendPtr<T> {} | ||
| #[cfg(feature = "parallel")] | ||
| unsafe impl<T> Sync for SendPtr<T> {} |
There was a problem hiding this comment.
[Medium] Sync impl is unsound for this type.
Sync means two threads can call get_mut(idx) concurrently. If they ever pass the same idx (even by mistake), you get two live &mut T pointing to the same location — instant UB with no compiler protection.
The current usage is safe because partitions are disjoint, but the type itself makes a stronger promise than it can keep. Consider limiting the surface:
| unsafe impl<T> Send for SendPtr<T> {} | |
| #[cfg(feature = "parallel")] | |
| unsafe impl<T> Sync for SendPtr<T> {} | |
| unsafe impl<T: Send> Send for SendPtr<T> {} | |
| // NOT Sync — concurrent get_mut with the same idx would be UB. | |
| // Safety is enforced at the call site by disjoint partition indices. |
Or, better, use a newtype that is Send but not Sync, and only share &SendPtr by passing it into each thread's closure by value (cloning just the pointer), so the compiler prevents cross-thread aliasing at the API level.
| }) | ||
| .collect(); | ||
|
|
||
| handles.into_iter().map(|h| h.join().unwrap()).collect() |
There was a problem hiding this comment.
[High] Thread panics silently bypass ProvingError.
h.join().unwrap() re-panics if the worker thread panicked. There are dozens of unwrap()/expect() calls inside prove_rounds_2_to_4 and build_round1 (see the TODO comment at line 1559). Any one of them will now crash the process instead of returning a ProvingError.
The old chunk loop used ? to propagate errors. This change regresses that:
| handles.into_iter().map(|h| h.join().unwrap()).collect() | |
| handles.into_iter().map(|h| h.join().expect("worker thread panicked — check for unwrap/expect inside prove_rounds_2_to_4")).collect() |
Longer term, consider catching panics with std::panic::catch_unwind inside the worker and mapping them to a ProvingError::InternalPanic variant, or (better) eliminating the unwrap()s so workers can only return Result.
| let pools: Vec<rayon::ThreadPool> = (0..s) | ||
| .map(|_| { | ||
| rayon::ThreadPoolBuilder::new() | ||
| .num_threads(threads_per_pool) | ||
| .build() | ||
| .expect("failed to create Rayon thread pool") | ||
| }) |
There was a problem hiding this comment.
[Medium] Pools are recreated on every multi_prove call.
ThreadPoolBuilder::build() spawns threads_per_pool OS threads per pool, so each call creates s × threads_per_pool threads and tears them down at the end. If multi_prove is called repeatedly (benchmarks, batch provers), this becomes significant overhead.
Consider building the pools once (lazily via OnceLock or passed in as a parameter) and reusing them across calls.
1 participant
No description provided.