Conversation
…l terminology** Rename `setNumThreads`/`getNumThreads` → `setParallelism`/`getParallelism` and `getThreadPoolCapacity` → `getPoolSize` across VectorSimilarity. Public info API fields (`numThreads`, `lastReservedThreads`, `NUM_THREADS`, `LAST_RESERVED_NUM_THREADS`) remain unchanged. No behavioral changes.
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
Codecov Report❌ Patch coverage is Please upload reports for the commit 3c69a48 to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
==========================================
- Coverage 96.98% 96.93% -0.06%
==========================================
Files 129 129
Lines 7574 7578 +4
==========================================
Hits 7346 7346
- Misses 228 232 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Assign work to rented workers (partitions 1..n-1) | ||
| for (size_t i = 0; i < num_workers; ++i) { | ||
| rented[i].assign({&f, i + 1}); | ||
| } |
There was a problem hiding this comment.
Missing bounds check when rent returns fewer threads
Medium Severity
The old parallel_for threw a ThreadingException when n > size_. The new code removed this guard and instead relies on rent() returning enough threads, checked only by assert. In release builds (asserts disabled), if rent() returns fewer threads than n - 1, the loop at rented[i].assign(...) accesses slots_ out of bounds — undefined behavior, likely a crash.
Reviewed by Cursor Bugbot for commit 3445381. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3c69a48. Configure here.
| is_two_level_lvq{isTwoLevelLVQ(params.quantBits)}, | ||
| threadpool_{std::max(size_t{SVS_VAMANA_DEFAULT_NUM_THREADS}, params.num_threads)}, | ||
| impl_{nullptr} { | ||
| threadpool_{SVSIndexBase::getSharedThreadPool()}, impl_{nullptr} { |
There was a problem hiding this comment.
Initial parallelism zero breaks standalone SVS indexes
High Severity
VecSimSVSThreadPool initializes parallelism_ to 0, and size() now returns parallelism_ instead of the actual pool size. For standalone (non-tiered) SVS indexes created via SVSFactory::NewIndex, no code calls setParallelism() after construction. SVS internally uses pool.size() for task partitioning — with size() == 0, operations like graph building and data copying produce zero partitions, silently doing no work. The old code initialized the pool from params.num_threads, ensuring a valid default.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3c69a48. Configure here.
1 participant
Step 1: Rename SVS thread pool internal methods to match shared pool terminology
Rename
setNumThreads/getNumThreads→setParallelism/getParallelismandgetThreadPoolCapacity→getPoolSizeacross VectorSimilarity. Public info API fields (numThreads,lastReservedThreads,NUM_THREADS,LAST_RESERVED_NUM_THREADS) remain unchanged. No behavioral changes.Describe the changes in the pull request
A clear and concise description of what the PR is solving.
Which issues this PR fixes
Main objects this PR modified
Mark if applicable
Note
Medium Risk
Touches SVS concurrency primitives and adds a new public C API for resizing the shared pool; mistakes could cause deadlocks/asserts or incorrect parallel execution under load.
Overview
SVS thread management is refactored to use a singleton shared thread pool (
SVSIndexBase::getSharedThreadPool) with a rental model, so multiple SVS operations can borrow disjoint worker threads concurrently while allowing safe pool grow/shrink.Per-index controls are renamed from threads to parallelism (
setParallelism/getParallelism) and separated from shared pool sizing (getPoolSize), with updated asserts/semantics for single-vector vs bulk operations and tiered SVS background jobs.Adds a new public API
VecSim_UpdateThreadPoolSize()that toggles write mode (in-place for0, async otherwise) and resizes the shared SVS pool; benchmarks/tests are updated to validate the new parallelism/pool-size behavior and stricter debug assertions.Reviewed by Cursor Bugbot for commit 3c69a48. Bugbot is set up for automated code reviews on this repo. Configure here.