Skip to content

MOD-15579 Lazy initialization of the shared SVS thread pool#971

Open
dor-forer wants to merge 19 commits into
mainfrom
dor-forer-MOD-15579-svs-thpool-lazy
Open

MOD-15579 Lazy initialization of the shared SVS thread pool#971
dor-forer wants to merge 19 commits into
mainfrom
dor-forer-MOD-15579-svs-thpool-lazy

Conversation

@dor-forer
Copy link
Copy Markdown
Collaborator

@dor-forer dor-forer commented May 26, 2026

Make initialization of the shared SVS thread pool lazy: OS worker threads are spawned only when the first SVS index is created, not when VecSim_UpdateThreadPoolSize is called. This eliminates the cost of pre-allocated SVS workers in deployments that configure RediSearch workers but never create an SVS index (today, every Redis with WORKERS=N configured spawns N-1 SVS worker threads at module init regardless of whether SVS is ever used).

The change extends the existing deferred-resize protocol to cover a second "safe point". VecSimSVSThreadPoolImpl::resize() now defers application in two cases:

  • No SVS index attached yet → recorded; applied on first onIndexAttached() (lazy thread spawn).
  • Shrink while jobs are in flight (pending_jobs_ > 0) → recorded; applied when endScheduledJob() drops the counter to 0 (existing behaviour).
  • Otherwise → applied 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). One new gate field, has_attached_index_, distinguishes the two states. Set on the first onIndexAttached() call from the per-index VecSimSVSThreadPool ctor.

A test-only resetForTest() hook (under BUILD_TESTS) restores the singleton to a clean baseline by releasing the slots vector capacity and clearing the new gate; intended for unit tests that need to observe lazy semantics in a process where prior tests may have already attached indexes.

VecSim_UpdateThreadPoolSize continues to set the write mode (InPlace/Async) eagerly — only the SVS pool sizing is now deferred. The C API surface is unchanged.

RediSearch integration impact: none required — VecSim_UpdateThreadPoolSize(N) is still the single entry point for both module init and CONFIG SET search-workers M. Pre-index calls now just record the size; post-index calls behave exactly as before.

Main objects this PR modified

  1. VecSimSVSThreadPoolImpl::resize() — now lazy until first index attaches.
  2. VecSimSVSThreadPoolImpl::onIndexAttached() — new entry point called from per-index VecSimSVSThreadPool ctor; flips has_attached_index_ and applies any deferred size.
  3. VecSimSVSThreadPoolImpl::has_attached_index_ — new member field gating spawn vs. defer.
  4. VecSimSVSThreadPoolImpl::resetForTest() — new BUILD_TESTS-only hook.
  5. VecSim_UpdateThreadPoolSize() — comment updated; behaviour now lazy via the new resize() semantics.

Tests

  • SVSTest.ThreadPoolLazyInit (new) — verifies VecSim_UpdateThreadPoolSize does not allocate worker threads before any SVS index exists, and that the first SVS index creation triggers the deferred spawn at the previously requested size.
  • SVSThreadPoolTest.UpdateThreadPoolSizeModeTransitions — rewritten to validate the lazy → eager transition (size stays at 1 until an index attaches; subsequent transitions are immediate).
  • SVSThreadPoolTest fixture — SetUp() calls onIndexAttached() so the existing pool-isolation tests retain eager resize() semantics.
  • SVSTest.NumThreadsParamIgnored — calls onIndexAttached() upfront for the same reason.
  • SVSTest.debugInfoGlobalMemoryEqualsSharedSVSThreadPoolMemoryASSERT_GT on global memory moved to after CreateNewIndex (the lazy spawn fires there).

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Supersedes #969 — ownership transferred to @dor-forer.


Note

Medium Risk
Changes when OS threads and pool memory are created (module init vs first SVS index), but defers to existing deferred-shrink safety and is covered by new/updated unit tests; C API entry point unchanged.

Overview
Defers spawning SVS worker OS threads until the first SVS index is created. VecSim_UpdateThreadPoolSize still sets write mode immediately, but pool sizing is only recorded in deferred_size_ until onIndexAttached() runs from the per-index VecSimSVSThreadPool constructor.

resize() now gates on has_attached_index_: before any index attaches it stores the target size; after attach it uses the existing resize_locked() path (including shrink-while-jobs-in-flight deferral via pending_jobs_). The two deferral cases share deferred_size_ and are documented as non-overlapping.

Adds BUILD_TESTS-only resetForTest() so unit tests can observe lazy semantics without a process-wide singleton teardown. Tests cover lazy init, lazy→eager UpdateThreadPoolSize transitions, and fixture hooks that call onIndexAttached() where tests resize the pool without building a real index.

Reviewed by Cursor Bugbot for commit 09d0e83. Bugbot is set up for automated code reviews on this repo. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented May 26, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

@dor-forer dor-forer changed the base branch from meiravg_svs_thpool_alloactor to dor-forer-MOD-15578-track-svs-thpool-memory May 26, 2026 14:26
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.97%. Comparing base (a923fa7) to head (09d0e83).

Additional details and impacted files
@@                               Coverage Diff                               @@
##           dor-forer-MOD-15578-track-svs-thpool-memory     #971      +/-   ##
===============================================================================
- Coverage                                        97.08%   96.97%   -0.12%     
===============================================================================
  Files                                              141      130      -11     
  Lines                                             8144     7843     -301     
===============================================================================
- Hits                                              7907     7606     -301     
  Misses                                             237      237              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dor-forer dor-forer force-pushed the dor-forer-MOD-15578-track-svs-thpool-memory branch from 31c0e0a to 0adca87 Compare May 26, 2026 14:51
@dor-forer dor-forer force-pushed the dor-forer-MOD-15579-svs-thpool-lazy branch from 6b4b160 to 8fe69ac Compare May 26, 2026 15:04
dor-forer and others added 2 commits May 26, 2026 18:09
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>
* 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>
@dor-forer dor-forer force-pushed the dor-forer-MOD-15578-track-svs-thpool-memory branch from 0adca87 to 9d082f0 Compare May 26, 2026 15:23
@dor-forer dor-forer force-pushed the dor-forer-MOD-15579-svs-thpool-lazy branch from 8fe69ac to 760524d Compare May 26, 2026 15:23
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 760524d. Configure here.

Comment thread src/VecSim/algorithms/svs/svs_utils.h Outdated
@dor-forer dor-forer force-pushed the dor-forer-MOD-15579-svs-thpool-lazy branch from 760524d to 75228a3 Compare May 27, 2026 07:41
- 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).
dor-forer and others added 9 commits June 1, 2026 14:57
…size

- 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>
…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>
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>
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>
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>
* `VecSimSVSThreadPoolImpl::resetForTest()`: assert `pending_jobs_ == 0`
  before clearing state so misuse fails loudly instead of corrupting the
  deferred-protocol bookkeeping.
* `SVSTest.NumThreadsParamIgnored`: restore the singleton via
  `resetForTest()` (clears `has_attached_index_` + slots) instead of
  leaving `has_attached_index_` set for subsequent tests.
* `SVSTest.ThreadPoolLazyInit`: tighten post-`VecSimIndex_New` checks
  from `EXPECT_*` to `ASSERT_*` so a regression in the lazy-spawn
  contract halts the test on the first failure instead of cascading.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dor-forer dor-forer force-pushed the dor-forer-MOD-15579-svs-thpool-lazy branch from f278e8d to 09d0e83 Compare June 3, 2026 15:06
Base automatically changed from dor-forer-MOD-15578-track-svs-thpool-memory to main June 4, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants