Remove AsThreadPool generic and take &RayonThreadPool directly#967
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Rayon thread-pool plumbing by removing the AsThreadPool generic + forward_threadpool! macro approach and standardizing pool-accepting APIs to take a borrowed &RayonThreadPool instead. This reduces monomorphization higher in the call stack and makes pool usage more explicit across providers/tools/disk build paths.
Changes:
- Removed
AsThreadPool,forward_threadpool!, andexecute_with_rayon; updated exports accordingly. - Updated PQ/kmeans/partition/quantization APIs to accept
&RayonThreadPooland adjusted call sites (including tests) to construct explicit pools. - Updated disk quantization generator and PQ generation context types to borrow the pool instead of carrying a generic pool type parameter.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-tools/src/utils/build_pq.rs | Creates a thread pool once and passes &RayonThreadPool into PQ generation calls. |
| diskann-providers/src/utils/rayon_util.rs | Removes execute_with_rayon/AsThreadPool infra; retains concrete RayonThreadPool and in-pool iterator helpers. |
| diskann-providers/src/utils/mod.rs | Stops re-exporting removed thread-pool abstractions/functions. |
| diskann-providers/src/storage/index_storage.rs | Updates tests to pass &RayonThreadPool instead of a thread-count integer. |
| diskann-providers/src/model/pq/pq_construction.rs | Degenericizes PQ construction APIs to take &RayonThreadPool. |
| diskann-providers/src/index/wrapped_async.rs | Updates tests to pass &RayonThreadPool. |
| diskann-providers/src/index/diskann_async.rs | Degenericizes train_pq and updates test call sites to pass &RayonThreadPool. |
| diskann-disk/src/utils/partition.rs | Degenericizes partitioning helper to take &RayonThreadPool. |
| diskann-disk/src/utils/math_util.rs | Degenericizes math utilities to take &RayonThreadPool. |
| diskann-disk/src/utils/kmeans.rs | Degenericizes kmeans utilities to take &RayonThreadPool. |
| diskann-disk/src/storage/quant/pq/pq_generation.rs | Removes pool type parameter from PQ generation context; now borrows &RayonThreadPool. |
| diskann-disk/src/storage/quant/generator.rs | Degenericizes quant data generator to take &RayonThreadPool; updates test call site accordingly. |
| diskann-disk/src/build/builder/quantizer.rs | Trains PQ quantizer by constructing a thread pool at call site and passing &RayonThreadPool. |
| diskann-disk/src/build/builder/core.rs | Updates merged index workflow partitioning call to the new signature. |
| diskann-disk/src/build/builder/build.rs | Updates PQ generation usage and generator invocation to the new borrowed-pool signatures. |
Comments suppressed due to low confidence (1)
diskann-providers/src/utils/rayon_util.rs:15
create_thread_poolis documented to treatnum_threads == 0as “use logical CPUs”, but the implementation always callsThreadPoolBuilder::num_threads(num_threads). If callers pass 0 (e.g., as an “auto” sentinel), this will rely on Rayon accepting 0; if Rayon treats 0 as invalid, this will return an error at runtime. Consider explicitly handlingnum_threads == 0by omitting.num_threads(...)(or mapping 0 toavailable_parallelism()) so behavior matches the docs and is stable across Rayon versions.
/// Creates a new thread pool with the specified number of threads.
/// If `num_threads` is 0, it defaults to the number of logical CPUs.
pub fn create_thread_pool(num_threads: usize) -> ANNResult<RayonThreadPool> {
let pool = rayon::ThreadPoolBuilder::new()
.num_threads(num_threads)
.build()
.map_err(|err| ANNError::log_thread_pool_error(err.to_string()))?;
Ok(RayonThreadPool(pool))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (77.46%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #967 +/- ##
==========================================
- Coverage 89.50% 89.49% -0.01%
==========================================
Files 448 448
Lines 84167 84118 -49
==========================================
- Hits 75333 75285 -48
+ Misses 8834 8833 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
3a31621 to
65f66b5
Compare
…h_rayon De-genericize all pool-accepting functions to take &RayonThreadPool directly. Remove PQGenerationContext/PQGeneration Pool type parameter (now borrows). Test callers that passed 1usize now create explicit 1-thread pools. Production callers that passed num_threads now create pools at call site. Addresses #905 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add RayonThreadPoolRef<'a> as a Copy wrapper around &rayon::ThreadPool. Update all ParallelIteratorInPool trait methods to take RayonThreadPoolRef<'_>. Update train_pq, generate_pq_pivots, and generate_pq_data_from_pivots signatures. Update all diskann-disk function signatures and struct fields. Convert call sites from &pool to pool.as_ref(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a1c7b06 to
cd7bb82
Compare
|
This closes #905 |
hildebrandmw
left a comment
There was a problem hiding this comment.
Thanks Jordan. One quick comment: can you please update the PR description to reflect that bring-your-own-threadpool is now supported! Cheers!
Bump version to 0.51.0 due to propagate changes to downstream consumers ## Breaking API changes (AI Generated) - **`ObjectPool` moved** (#975): now lives in `diskann-utils`. Update imports from `diskann::...::ObjectPool` → `diskann_utils::ObjectPool`. - **`AlignedSlice` removed** (#994): the `AlignedSlice` abstraction in `diskann-vector` is gone. Code that converted between vector representations through `AlignedSlice` should now use the `Poly` / `CastFromSlice` polymorphic interfaces directly (see `diskann-vector::conversion` and `diskann-quantization::alloc::poly`). Storage that previously held `AlignedSlice` values should hold `Poly<T, A>` instead. - **`AsThreadPool` generic removed** (#967): functions that previously took `pool: impl AsThreadPool` now take `pool: &RayonThreadPool`. Pass a borrow of an existing pool; remove the generic parameter from your call sites. - **`sgemm()` returns `Result`** (#997): in `diskann-linalg`, the new signature is: ```rust pub fn sgemm( atranspose: Transpose, btranspose: Transpose, m: usize, n: usize, k: usize, alpha: f32, a: &[f32], b: &[f32], beta: Option<f32>, c: &mut [f32], ) -> Result<(), SgemmError> ``` `SgemmError` has variants `InvalidMatrixDimensions { matrix_name, expected_rows, expected_cols, actual_len }` and `DimensionOverflow { matrix_name, rows, cols }`. Replace previous panic-on-bad-input assumptions with explicit handling. - **Benchmarks are stateful** (#995): the `Benchmark` impls in `diskann-benchmark` are no longer stateless unit structs. Each benchmark type now has a `::new()` constructor (often holding `PhantomData<T>` or plugin state), and registration uses an instance: ```rust // before benchmarks.register("name", MyBench); // after benchmarks.register("name", MyBench::<T>::new()); ``` If you wrote a custom benchmark, give it a `new()` and register an instance. Combined with #996, search-side benchmarks now compose `Plugins<Provider, Phase, Strategy>` and expose builder methods like `.search(plugin)` to register search plugins on the instance. - **`diskann-benchmark`: `async` → `graph-index`** (#1009): the benchmark category previously named `async` was renamed to `graph-index`. JSON config `type` values and example file names changed accordingly: - `async-build` → `graph-index-build` - `async-dynamic-run` → `graph-index-dynamic-run` - and the same prefix swap for `*-pq`, `*-sq`, `*-spherical-quantization`, etc. Update any benchmark config files, scripts, or CI that reference the old `async-*` names. - **`diskann-disk` buffer alignment decoupled from `block_size`** (#984): code that assumed I/O buffer alignment equals the disk block size should now configure alignment explicitly. ## Non-breaking - New cache-aware block-transposed Chamfer/MaxSim distance for f32/f16 (#863). - A/A benchmark documentation (#974); CI publish workflow improvements (#755, #1017); openssl bump (#973); `compute_closest_centers` allocation reduction (#980). - **`DistanceComputer` `'static` bound relaxed** (#1007) and **redundant `DistanceFunction` impls removed** (#1008) **Full Changelog**: v0.50.1...v0.51.0
Remove AsThreadPool trait, forward_threadpool! macro, and execute_with_rayon.
Addresses and Closes #905
Copilot summary of changes and usage:
Removed:
Added:
Updated:
Usage