[8.4] SVS shared thread pool with rental model (MOD-9881, MOD-17090)#1003
Merged
Conversation
* **Step 1: Rename SVS thread pool internal methods to match shared pool 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. * introduce VecSim_UpdateThreadPoolSize api * imp new VecSimSVSThreadPoolImpl * imp VecSimSVSThreadPool * add resize handling * log ctx * default paralleism to 1 * fix VecSim_UpdateThreadPoolSize to include current thread * revert resize * better parallelism docs * rent private thpool tests fix fp16 tests * deprecatred num_threads * assert pool size align bm * minor fixes * address review * use no - op deleter * poolSize static * move VecSimSVSThreadPoolImpl as a sublass of VecSimSVSThreadPoolImpl * review comment * maange resize life cycle * simplify register to ckean up * remove assert * better test * remove shared pointer * addres cursor comment * address review comments (cherry picked from commit 7f01bfa) (cherry picked from commit fa1518d)
…lic API (#972) * use allocator for svs thpool * remove double counting introduce VecSim_GlobalStatsInfo * return globalmem * add GLOBAL_MEMORY_STRING * cleanups * fix size sstimation * clean ups * MOD-15578 Fix SHARED_SVS_THREADPOOL_MEMORY doc comment The doc string claimed the field is exposed only in SVS tiered indexes, but `SVSIndex::debugInfoIterator()` emits it for flat SVS indexes too (and inside the BACKEND_INDEX section of tiered SVS). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * MOD-15578 Guard SVS-only code in unit_test_utils for HAVE_SVS=0 builds * Drop the unconditional `#include "VecSim/algorithms/svs/svs_utils.h"` at the top of `unit_test_utils.cpp` (added in 3c2023a). The header pulls in `<svs/core/distance.h>`, `<svs/index/vamana/dynamic_index.h>`, etc. — none of which are available when `USE_SVS=OFF`. The HAVE_SVS-guarded include lower in the file already provides the symbols `validateSVSIndexAttributesInfo` needs. * Wrap `compareSVSIndexInfoToIterator` (definition + declaration in the header) and its call site inside `chooseCompareIndexInfoToIterator` with `#if HAVE_SVS`. The function references `VecSimSVSThreadPool`, which is undefined when SVS is disabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * MOD-15578 Address PR #972 review comments - svs_factory.cpp: mark unused 'p' variable with [[maybe_unused]] to silence -Wunused-variable under -Werror (Copilot review). - vec_utils.h: rewrite SHARED_SVS_THREADPOOL_MEMORY_STRING doc comment to match actual emission behavior — emitted by SVSIndex::debugInfoIterator() for both non-tiered (top-level) and tiered (BACKEND_INDEX) SVS responses (Copilot review). * MOD-15578 Address PR #972 review: rename shared-memory API + cover resize - Rename VecSim_GetGlobalMemory -> VecSim_GetSharedMemory and the GLOBAL_MEMORY field -> SHARED_MEMORY; "global" wrongly implied it included per-index memory. Doc now states it excludes per-index memory. - Simplify VecSimSVSThreadPoolImpl::slots_ to vecsim_stl::vector<SlotPtr>, dropping the SlotVecAllocator alias and the explicit allocator spelling in the ctor; add the now-needed vecsim_stl.h include. - Fix the SVS debugInfoIterator field count (was a stale 23; true count was 25, now 26 with the new shared-memory field) and pin a breakdown comment so it can't drift again. - Add SVSTest.sharedMemoryTracksThreadPoolResize: assert shared memory grows when the pool grows and shrinks when it shrinks, not just that the SHARED_MEMORY / SHARED_SVS_THREADPOOL_MEMORY / VecSim_GetSharedMemory readouts agree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * MOD-15578 Address PR #972 review: remove unused global_value in tests * MOD-15578 Reserve SVS info-iterator capacity for C API SHARED_MEMORY field SVSIndex::debugInfoIterator() reserved 26 fields, but the C API wrapper VecSimIndex_DebugInfoIterator appends one more (SHARED_MEMORY) after the method returns, forcing a reallocation on the top-level path. Bump the capacity hint to 27 and document the +1. Addresses Cursor Bugbot finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * MOD-15578 Fix tiered testInfoIterator to use C API iterator compareTieredIndexInfoToIterator expects the top-level SHARED_MEMORY field that VecSimIndex_DebugInfoIterator (C API) appends, but the tiered tests fed it a C++-method iterator (tiered_index->debugInfoIterator()) which omits that field, causing an off-by-one field-count failure (HNSW tiered 16 vs 17, SVS tiered 18 vs 19). Obtain the iterator via the C API like the non-tiered tests and the real RediSearch consumer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * MOD-15578 Clarify SVS info-iterator capacity-hint comment Lead with 27 = 26 + 1 so the comment no longer reads as a 26-vs-27 mismatch against numberOfInfoFields. No functional change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * MOD-15578 Collapse SHARED_SVS_THREADPOOL_MEMORY into SHARED_MEMORY Address PR #972 review (alonre24): the SVS-specific SHARED_SVS_THREADPOOL_MEMORY field always reported the same bytes as the generic top-level SHARED_MEMORY field, since the SVS thread pool is the only process-wide contributor today. The duplicate breakdown carried no extra information, so drop it (YAGNI) and keep only the generic SHARED_MEMORY field + VecSim_GetSharedMemory() API. If a second global contributor is added later, an itemized breakdown can be reintroduced then with real differentiation. * svs.h: remove the SHARED_SVS_THREADPOOL_MEMORY field emission in SVSIndex::debugInfoIterator(); capacity hint 27 -> 26 (15 SVS-specific fields instead of 16). * vec_utils.{h,cpp}: drop SHARED_SVS_THREADPOOL_MEMORY_STRING. * test_svs.cpp: rename debugInfoSharedMemoryEqualsSharedSVSThreadPoolMemory -> debugInfoSharedMemoryMatchesApi; assert SHARED_MEMORY appears once and equals VecSim_GetSharedMemory(). * unit_test_utils.cpp: SVS field count 26 -> 25; drop the comparator branch and getSVSFields() entry for the removed field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: meiravgri <meirav.grimberg@redis.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit ca937db) (cherry picked from commit 9872e35)
Defer creation of the shared SVS thread pool's OS worker threads until the first SVS index is created, instead of spawning them eagerly on `VecSim_UpdateThreadPoolSize()` at module init. Today every Redis with `WORKERS=N` configured spawns N-1 SVS worker threads at startup even in deployments that never create an SVS index; this eliminates that cost. The change extends the existing deferred-resize protocol to a second safe point. `VecSimSVSThreadPoolImpl::resize()` now defers in two cases: * No SVS index attached yet -> record the requested size; apply it on the first `onIndexAttached()` call (lazy thread spawn). * Shrink while jobs are in flight (`pending_jobs_ > 0`) -> record; apply when `endScheduledJob()` drops the counter to 0 (existing behaviour). * Otherwise -> apply immediately via `resize_locked()`. The two deferral cases share `deferred_size_` and never overlap (no jobs can be in flight before the first index attaches). A new gate field `has_attached_index_` distinguishes the states; it is set on the first `onIndexAttached()` call from the per-index `VecSimSVSThreadPool` ctor. `VecSim_UpdateThreadPoolSize` still sets the write mode eagerly — only the SVS pool sizing is deferred. The C API surface is unchanged, and no RediSearch integration changes are required. Test-only `resetForTest()` (under BUILD_TESTS) restores the singleton to a clean baseline; it asserts `pending_jobs_ == 0` so misuse fails loudly instead of corrupting the deferred-protocol bookkeeping. Tests: * SVSTest.ThreadPoolLazyInit (new) — verifies UpdateThreadPoolSize does not allocate worker threads before any SVS index exists, and that the first index creation triggers the deferred spawn at the requested size. * SVSThreadPoolTest.UpdateThreadPoolSizeModeTransitions — validates the lazy -> eager transition (size stays 1 until an index attaches). * SVSThreadPoolTest fixture / SVSTest.NumThreadsParamIgnored / SVSTest.sharedMemoryTracksThreadPoolResize — call onIndexAttached() up front so resizes apply eagerly where the test needs it. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 6468271) (cherry picked from commit d65c251)
VecSim_GetSharedMemory() previously returned the shared SVS thread pool allocator's self-accounting baseline (~8 bytes) as soon as the singleton was constructed. The singleton is constructed at module init via VecSim_UpdateThreadPoolSize (RediSearch's workersThreadPool_CreatePool), so the baseline was reported even on deployments with no SVS index — and RediSearch folds VecSim_GetSharedMemory() into FT.INFO vector_index_sz_mb, making that field nonzero with no vector index. Gate getSharedAllocationSize() on has_attached_index_ (read under pool_mutex_) so it reports 0 until the first SVS index attaches, for both reasons it might be touched early: never-constructed, or constructed only to record a requested pool size. Thread-spawn timing is unchanged; only the reported size changes, so there is no CONFIG-SET-vs-attach race. Update ThreadPoolLazyInit to assert the baseline is exactly 0 before any SVS index attaches. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 45daaf3) (cherry picked from commit be0aeae)
4 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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 0b975a1. Configure here.
| task_type task; | ||
| std::shared_ptr<ControlBlock> controlBlock; | ||
| JobsRegistry *jobsRegistry; | ||
| bool isScheduled; // true if this job holds a pending-job reservation on the thread pool |
There was a problem hiding this comment.
Task exception permanently leaks pending_jobs counter
Medium Severity
If jobPtr->task() throws in ExecuteMultiThreadJobImpl, delete_job(job) is never reached, so the ~SVSMultiThreadJob destructor never runs and endScheduledJob() is never called. This permanently increments pending_jobs_, causing all future pool shrink requests to be deferred indefinitely and never applied. The isScheduled / endScheduledJob() mechanism is new in this PR, introducing this failure mode where the deferred-resize protocol becomes permanently stuck.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0b975a1. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Confirmed — real bug, and worse than described: ExecuteMultiThreadJobImpl is invoked from RediSearch's plain-C pthread worker pool (no C++ handler anywhere on that call stack), so an uncaught exception here calls std::terminate() — a process crash, not just a stuck pending_jobs_ counter. task() realistically can throw: updateSVSIndexWrapper/SVSIndexGCWrapper → updateSVSIndex() → createImpl/setImpl, which already contain explicit throw std::logic_error(...) (non-empty impl_, failed dynamic_cast) plus ordinary bad_alloc risk.
Important: this isn't introduced by this backport. isScheduled/endScheduledJob() are verbatim from #925, already merged and live on main, 8.6, and 8.6-rse. Bugbot flags it as new only because it's diffing against 8.4's pre-#925 baseline.
Not fixing this in the backport PR — patching only here would diverge from upstream and this needs a real fix (wrap the task() call, decide failure semantics) on main first, then flow down through the other branches like everything else. Tracking separately.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.4 #1003 +/- ##
==========================================
- Coverage 97.04% 96.99% -0.05%
==========================================
Files 126 126
Lines 7470 7599 +129
==========================================
+ Hits 7249 7371 +122
- Misses 221 228 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alonre24
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes in the pull request
Backport of the SVS shared thread pool to the
8.4branch. With this change all SVS indexes share a single pool sized to the RediSearch worker pool (N threads total), with a rental-based execution model.Four clean cherry-picks from
main(no code changes needed):fa1518d8refactor: SVS shared thread pool with rental model (the fix)bbaa34a0track shared pool memory (textual dependency of MOD-15579 Lazy initialization of the shared SVS thread pool #971)947a37b7lazy pool initialization — no OS threads spawned until the first SVS index attaches, so deployments without SVS indexes see no new threads0b975a1ereport 0 shared memory until first index attachesWhich issues this PR fixes
Main objects this PR modified
src/VecSim/algorithms/svs/svs_utils.h— sharedVecSimSVSThreadPoolImplsingleton, rental model, lazy init, deferred resizesrc/VecSim/algorithms/svs/svs.h,svs_tiered.h—setParallelism/getParallelism/getPoolSize, scheduled-job reservation protocolsrc/VecSim/vec_sim.cpp/.h,vec_sim_common.h— new C APIVecSim_UpdateThreadPoolSize(),VecSim_GetSharedMemory()Mark if applicable
Validation: full unit suite on this branch — 1927 tests, 0 failures (includes new
test_svs_threadpool.cppsuite: resize grow/shrink, shrink-while-rented, scheduled-job deferred shrink, lazy init). Same 4-commit chain and validation approach as the already-merged 8.6 backport.Merge note: please rebase-merge (not squash) — the 4 commits are individual cherry-picks from
main, each carrying its(cherry picked from ...)line for traceability on the release branch.Companion RediSearch backport (submodule bump + worker-pool integration, RediSearch#8956) follows after this merges: targets
8.4.🤖 Generated with Claude Code
Note
High Risk
Changes core SVS/tiered concurrency, global thread lifecycle, and public C API; incorrect rental or deferred resize could cause crashes, deadlocks, or thread over-subscription in production.
Overview
SVS threading moves from per-index pools to a process-wide shared pool sized via
VecSim_UpdateThreadPoolSize()(replacesSVSParams.num_threads, which now logs a deprecation warning). Per-indexsetParallelism/getParallelismandgetPoolSize()replacesetNumThreads/getNumThreads/getThreadPoolCapacity.The pool uses a rental model (
ThreadSlotoccupancy,RentedThreadsRAII): workers are rented forparallel_for, with lazy OS thread spawn on first index attach and deferred shrink while scheduled tiered jobs hold reservations (beginScheduledJob/endScheduledJob,createScheduledJobs).New C API:
VecSim_GetSharedMemory()and aSHARED_MEMORYdebug field onVecSimIndex_DebugInfoIteratorso shared pool bytes are not double-counted in per-index stats.VecSim_UpdateThreadPoolSize(0)sets write-in-place mode;>0sets async and resizes the pool.Tests and benchmarks updated;
test_svs_threadpool.cppcovers resize, deferred shrink, and concurrent rental.Reviewed by Cursor Bugbot for commit 0b975a1. Bugbot is set up for automated code reviews on this repo. Configure here.