Feat/isolated pool parallelism#480
Conversation
Proposes replacing the global Rayon pool chunks-of-K approach in multi_prove Rounds 2-4 with S isolated thread pools, each processing a load-balanced partition of tables on dedicated cores. Targets 3-4x improvement for Rounds 2-4 on 32+ core machines without changing the proof format or verifier.
7-task plan for replacing the global Rayon pool in multi_prove Rounds 2-4 with S isolated thread pools. Reviewed and fixed for borrow checker safety, instruments TLS correctness, and unsafe soundness.
Replace the chunk-based parallel proving (K workers on global pool) with isolated Rayon ThreadPool partitions. Tables are partitioned into S groups via LPT scheduling (partition_tables_by_cost), each group runs on a dedicated ThreadPool with cores/S threads, eliminating cross-table contention on the global Rayon pool. Key changes: - Drop Phase A/C pool_sets before Rounds 2-4, allocate S fresh r24_pool_sets - std::thread::scope spawns S OS threads, each calling pool.install(...) - SendPtr wrapper enables safe disjoint mutable access to per-table transcripts - Proofs collected with original indices, sorted for deterministic output order - instruments feature: indexed timings sorted to match proof order - Non-parallel fallback: sequential loop using r24_pool_sets[0]
Ensures each partition has at least 2 tables, avoiding wasteful pool creation for single-table partitions.
Move pool-return (into_columns + move-back) before the error match, so subsequent tables in the same partition get valid buffers even if a prior table's proving fails.
|
/bench 3 |
Codex Code Review
No other concrete security/logic regressions stood out in the PR diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // SAFETY: The caller guarantees disjoint access — no two threads dereference the | ||
| // same index. The raw pointer itself is just an address; sending it is safe. | ||
| #[cfg(feature = "parallel")] | ||
| unsafe impl<T> Send for SendPtr<T> {} |
There was a problem hiding this comment.
Medium — Unsound Send/Sync impl: missing T: Send bound
unsafe impl<T> Send for SendPtr<T> is unsound because it allows wrapping non-Send types (e.g. *mut Rc<Transcript>) and sending them across threads. The bound should require T: Send.
Ironically, the plan doc (2026-04-08-isolated-pool-parallelism.md) gets this right:
unsafe impl<T: Send> Send for SendPtr<T> {}| unsafe impl<T> Send for SendPtr<T> {} | |
| unsafe impl<T: Send> Send for SendPtr<T> {} |
| #[cfg(feature = "parallel")] | ||
| unsafe impl<T> Send for SendPtr<T> {} | ||
| #[cfg(feature = "parallel")] | ||
| unsafe impl<T> Sync for SendPtr<T> {} |
There was a problem hiding this comment.
Same issue — Sync impl also needs the T: Send bound (since we're sharing the ability to produce &mut T across threads, the pointed-to type must be Send).
| unsafe impl<T> Sync for SendPtr<T> {} | |
| unsafe impl<T: Send> Sync for SendPtr<T> {} |
| for chunk_start in (0..num_airs).step_by(k) { | ||
| let chunk_end = (chunk_start + k).min(num_airs); | ||
| let chunk_size = chunk_end - chunk_start; | ||
| let s = shard_parallelism().min(num_airs / 2).max(1); |
There was a problem hiding this comment.
Medium — Bug: num_airs / 2 cap incorrectly limits parallelism
Integer division means num_airs / 2 floors to 0 when num_airs = 1 (saved by .max(1)), but produces 1 for num_airs = 2 or 3, so s = 1 always in those cases — no parallelism at all. For num_airs = 4, s is capped at 2 instead of 4.
The plan spec says .min(num_airs), which is the correct cap (at most one partition per table). Even if the goal is "at least 2 tables per partition", the right cap is still .min(num_airs) — partition_tables_by_cost already handles empty partitions gracefully.
| let s = shard_parallelism().min(num_airs / 2).max(1); | |
| let s = shard_parallelism().min(num_airs).max(1); |
| }) | ||
| .collect(); | ||
|
|
||
| handles.into_iter().map(|h| h.join().unwrap()).collect() |
There was a problem hiding this comment.
Low — Panics instead of propagating ProvingError on thread panic
If any worker thread panics (e.g. due to a Rayon internal failure or an unexpected unwrap), .unwrap() here re-panics in the calling thread, bypassing the Result<_, ProvingError> error channel that the rest of multi_prove uses. The old chunked code collected errors as Results and surfaced them to the caller.
Not critical since ProvingError cases inside the partition are handled via results.push(Err(e)) and only genuine panics would hit this path — but worth noting the behavior change.
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: 777c2c4 · Baseline: cached · Runner: self-hosted bench |
Review: Isolated Pool Parallelism (Rounds 2-4)Good architectural direction — replacing the global-pool chunking with isolated Issues foundMedium — Two inline comments on
Medium —
Low —
No issues with
|
|
It seems this is affecting Rounds 2-4. We could also try applying this same technique for Round 1. |
Just tested this, no changes. |
2 participants
No description provided.