[8.6] SVS shared thread pool with rental model (MOD-9881, MOD-17089)#998
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)
…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)
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)
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)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.6 #998 +/- ##
======================================
Coverage ? 96.90%
======================================
Files ? 129
Lines ? 7710
Branches ? 0
======================================
Hits ? 7471
Misses ? 239
Partials ? 0 ☔ 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.6branch. 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):7f01bfa1refactor: SVS shared thread pool with rental model (the fix)ca937dbftrack shared pool memory (textual dependency of MOD-15579 Lazy initialization of the shared SVS thread pool #971)64682716lazy pool initialization — no OS threads spawned until the first SVS index attaches, so deployments without SVS indexes see no new threads45daaf32report 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 — 2150 tests, 0 failures (includes new
test_svs_threadpool.cppsuite: resize grow/shrink, shrink-while-rented, scheduled-job deferred shrink, lazy init).Companion RediSearch backports (submodule bump + worker-pool integration, RediSearch#8956) follow after this merges: targets
8.6and8.6-rse.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.🤖 Generated with Claude Code
Note
High Risk
Changes core SVS concurrency, tiered background jobs, and new C API contracts; incorrect pool sizing or deferred resize could affect stability under load with many indexes.
Overview
Replaces per-index SVS OS thread pools with a process-wide shared pool sized to match RediSearch workers. Indexes keep a lightweight per-index parallelism setting; workers are rented for each
parallel_forinstead of each index owningNthreads (fixes deployments with thousands of indexes exhausting thread limits).API and behavior:
SVSParams.num_threadsis deprecated (warning only). Pool size and async vs in-place mode are driven byVecSim_UpdateThreadPoolSize()(0→ write-in-place,>0→ async + resize).VecSim_GetSharedMemory()and aSHARED_MEMORYdebug field report pool bytes without double-counting per-index stats. Thread pool lazy init: no worker threads until the first SVS index attaches; deferred shrink while scheduled tiered jobs hold reservations (beginScheduledJob/endScheduledJob,createScheduledJobs).Tiered SVS: update/GC scheduling uses the shared pool snapshot; tracing hooks and tests cover shrink-during-job safety.
Reviewed by Cursor Bugbot for commit 8fa67b7. Bugbot is set up for automated code reviews on this repo. Configure here.