Support fine-grained SVS index by razdoburdin · Pull Request #1058 · RedisAI/VectorSimilarity

razdoburdin · 2026-09-23T08:47:13Z

Reopening of #1027
Adds support for using newly-added separate fine-grained SVS index with minor corresponding test revisions

Includes SVS 0.5.0 rc1 binaries, which will eventually be swapped out for the official release binaries after validation.

Redisearch CI failures at test_vecsim_svs.py:711 and test_vecsim_svs.py:729 come from changes in internal logic not being reflected in tests yet.

Each deletion now submit a single label consolidation job, that can change size of the reverse edges lists. This breaks the test_vecsim_svs.py:711.
Some of the submitted consolidation jobs can be unfinished before GC launch. This breaks the test_vecsim_svs.py:729.

Note

High Risk
Touches core indexing, async job ordering, and SVS binary/API upgrades across tiered HNSW/SVS paths where races or consolidation timing could affect query correctness or Redisearch SVS tests.

Overview
Upgrades SVS integration to 0.5.0rc10 prebuilt binaries and switches the implementation to Intel’s concurrent Vamana APIs (SegmentedBlocked storage/graph, lazy atomic impl_, compact() GC, per-label consolidate, safer relabel via replace_external_id).

Tiered SVS is reworked for fine-grained concurrency: training still batches into an initial backend build, but steady-state ingestion uses per-vector insert jobs (plus async consolidate jobs after deletes) instead of the old swap journal / monolithic batch update. Relabel coordinates pending inserts (Delayed/Parked) and can take over in-flight consolidations. Tiered HNSW shares the same base insert-job bookkeeping, batch-iterator merge logic, and scoped lock hooks for size/capacity queries.

Build wiring drops older glibc SVS tarballs, bumps the minimum GLIBC to 2.28 for GCC shared libs, and probes replace_external_id in the new concurrent dynamic header path.

^{Reviewed by Cursor Bugbot for commit c117c24. Bugbot is set up for automated code reviews on this repo. Configure here.}

* MOD-14916 Devirtualize distance + getElement on HNSW search hot path MOD-14916 / LTK perf investigation. Two virtual dispatches per HNSW candidate added between v2.10.21 and v8.2.6 account for a measurable share of the KNN regression observed in the LTK benchmarks (-38% throughput on Intel). Both are removed here with the minimum possible change. V1 - distance computation: Every calcDistance() call goes through IndexCalculatorInterface's vtable to reach DistanceCalculatorCommon, which then calls the underlying SIMD function pointer. The intermediate vtable hop is pure indirection; the concrete calculator class is fixed for the life of an index. Expose the underlying dist_func via a new pure-virtual getDistFunc() on IndexCalculatorInterface, implemented by DistanceCalculatorCommon. Cache the returned function pointer in VecSimIndexAbstract at construction time and call it directly in calcDistance(), bypassing the virtual dispatch. V2 - vector fetch: HNSWIndex::getDataByInternalId and BruteForceIndex::getDataByInternalId call this->vectors->getElement(id), which is virtual through the RawDataContainer base. DataBlocksContainer is the only concrete implementation, and this->vectors is always a DataBlocksContainer (created and owned by VecSimIndexAbstract's constructor). Use a static_cast to DataBlocksContainer* plus a qualified call to DataBlocksContainer::getElement to skip the vtable lookup. No behavior change; per-candidate distance and neighbor-fetch calls on HNSW / brute force search paths become direct function-pointer / direct-member calls. Headers in index_factories, hnsw_serializer, and brute_force_factory compile cleanly. * MOD-14916 Inline DataBlocksContainer::getElement on HNSW search hot path Follow-up to the previous V1/V2 devirt commit. The static_cast+qualified call in getDataByInternalId removed the vtable lookup but left the larger cost on the table: DataBlocksContainer::getElement was still defined in data_blocks_container.cpp, so every per-candidate neighbor fetch still paid a real out-of-line function call and a bounds-checked blocks.at() lookup. Without LTO the compiler could neither inline the body nor hoist the div/mod in the HNSW hot loop. Move the definition into the header as inline and drop the .at() bounds check to match the v2.10.21 baseline, which used unchecked operator[] and was fully inlined into processCandidate. Also add a getDistFunc() override to DistanceCalculatorDummy in test_components.cpp so BUILD_TESTS still compiles after the pure virtual added in the previous commit. (cherry picked from commit 4ca500a)

Remove null characters from end of file

…arity into lockfree

Upstream renamed the three thread-control methods on SVSIndexBase: getNumThreads -> getParallelism setNumThreads -> setParallelism getThreadPoolCapacity -> getPoolSize Adopt those names here ahead of merging upstream/main. This is a pure rename -- 32 lines across 6 files, mechanically verified by reversing the substitution and diffing against the parent commit. Method bodies deliberately keep this branch's threadpool API (size/resize/capacity), since VecSimSVSThreadPool here is still the per-index owned pool. Upstream reworked it into a process-wide singleton with thread renting, sized via VecSim_UpdateThreadPoolSize(); that is genuine divergence to reconcile in the merge, not something a rename should paper over. The point is to remove this conflict class before merging. The names collided on roughly half the affected lines without producing conflict markers, so git resolved some toward upstream and some toward here, yielding a tree that referenced methods it no longer declared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Resolutions, by class: Thread pool. Upstream replaced the per-index owned VecSimSVSThreadPool with a process-wide singleton that rents threads, sized via VecSim_UpdateThreadPoolSize() and defaulting to parallelism 1. Took upstream throughout: the getParallelism/ setParallelism/getPoolSize bodies in svs.h (this branch's threadpool_.capacity() no longer exists), the constructor, and scheduleSVSIndexInit/GC, which now use createScheduledJobs() so the pool's reserve/release accounting stays balanced. Kept this branch's initSVSIndexWrapper as the callback -- Dmitry renamed updateSVSIndexWrapper, and the name upstream passes is no longer defined. Distance calculators. Upstream's DistanceDispatch supersedes this branch's getDistFunc()/cachedDistFunc (PR #937): same vtable-avoidance goal, but generalized to stateful calculators, which the new SQ8 DistanceCalculatorWithNorm needs, plus asymmetric query distance. Took upstream wholesale for calculator.h, vec_sim_index.h, and test_components.cpp; this branch had touched those files only for the superseded caching, and no references to the old API remain. Concurrent index. Kept this branch throughout, since that is the point of it: SVSIndexBase::addVector (upstream dropped it; svs_tiered.h still needs it), ready(), atomic num_marked_deleted, the SegmentedBlocked/concurrent-namespace retargeting in svs_utils.h and svs_extensions.h, and the write-in-place delete-then-add path. That path keeps only updateJobMutex and not upstream's added mainIndexGuard -- the concurrent backend serializes writes against readers itself. Upstream's executeInsertJob-adjacent conflict was a mis-alignment: the text it offered belongs to the batch-drain function that initSVSIndex() replaces. Carried upstream changes that would otherwise have been lost to that restructuring: the GCJob::before_run_gc tracing hook (fired before taking updateJobMutex, so a test callback cannot deadlock against it) and the empty-batch guard around the backend write in initSVSIndex(), where setParallelism(0) is not a valid request against the shared pool. Tests. Took upstream's expectations: SVSParams.num_threads is now deprecated and ignored with a warning, so deriving expected capacity from it is no longer valid. deps/ScalableVectorSearch. Restored as a proper submodule gitlink at upstream's 7786d43b, discarding commit 8265a7a's symlink into a developer's home directory, which was dangling for everyone else. Not verified by a build: this host has GCC 11.4 and no container runtime, and SVS needs GCC 13+. Audited statically instead -- no conflict markers, no orphaned references to removed APIs, and every SVSIndexBase method used in svs_tiered.h is declared in svs.h. That last check is the one the first attempt at this merge failed: half the thread-API collisions produced no conflict markers, so git resolved some lines toward each side and left the tree calling methods it no longer declared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…/VectorSimilarity into dev/eglaser-lockfree

alonre24

Thanks for 315a89b. Queries and deletes no longer wait on the initial build, and the bookkeeping for deletes and overwrites during the build looks right to me.

However, the main thread still blocks for the whole build on HSET with RediSearch's default config. Details inline. In short: initSVSIndexWrapper still holds updateJobMutex exclusively for the build, and every addVector during the build takes the direct-to-backend path, which waits on that lock.

The fix looks small: keep buffering while init runs, as main did. On main, async addVector always went to flat and only scheduled an update; flatBufferLimit only capped updateTriggerThreshold. So letting flat grow past the limit during the build is no regression vs. main.

Could you also add the test from my earlier comment (item 4)? From UpdateJob::before_add_to_svs, call addVector with flat at or above flatBufferLimit (the default case) and assert it returns within a timeout. It would have caught this.

alonre24 · 2026-10-14T09:46:33Z

-                    this->swaps_journal.emplace_back(SKIP_LABEL, id, id);
+
+        this->flatIndexGuard.lock_shared();
+        if (this->frontendIndex->indexSize() >= flat_buffer_bound) {


This is where the main thread still blocks during the initial build.

While initInProgress is set, tryBufferForTraining returns nullopt, so we get here with the backend not ready yet. At that point flat holds the whole training batch:

RediSearch passes flatBufferLimit = tieredVecSimIndexBufferLimit, which defaults to DEFAULT_BLOCK_SIZE (1024).

The training threshold defaults to 10240 for compressed indexes. Uncompressed indexes use 1024, and more vectors keep arriving before a worker picks up the init job.

So indexSize() >= flat_buffer_bound is true, and the std::shared_lock(updateJobMutex) below waits for initSVSIndexWrapper to release its exclusive lock, i.e. for the entire build.

Suggestion: restore the ready() check that was dropped here earlier. It's needed again now that tryBufferForTraining can return nullopt before the backend is ready:

if (svs_index->ready() && this->frontendIndex->indexSize() >= flat_buffer_bound) {

Writes during init then go to the buffered insert-job path. Those jobs wait on updateJobMutex on a worker, not on the main thread. Flat grows past the limit only for the duration of the build, the same as on main, where async mode never had a hard cap.

done. I added the related test as well

alonre24 · 2026-10-14T09:46:33Z

-        const bool in_backend = svs_index->isLabelExists(old_label);
-        if (!in_flat && !in_backend) {
-            return VecSimRelabel_OldLabelMissing;
+        std::shared_lock<std::shared_mutex> lock(this->updateJobMutex);


Same issue for relabel: it takes updateJobMutex shared, so a relabelVector on the main thread during the initial build waits for the whole build. Is that acceptable, or should relabel also avoid waiting on init?

I think that is acceptable.

alonre24 · 2026-10-14T09:46:33Z

-                main_shared_lock.unlock();
-                std::lock_guard lock(this->mainIndexGuard);
+                svs_index->setParallelism(1);
                svs_index->setImpl(std::move(impl));


Minor: setImpl now publishes the backend before the final exclusive flat section runs. Until that section drops the copied rows from flat, they are in both tiers:

a query can see the same single-value label from both tiers;

deleteVector on such a label removes it from flat and from the backend, and returns 2 for a single-value index. The final section's deleteVectors then finds nothing.

The window is short, but moving setImpl into the final std::lock_guard flat_lock section closes it at no real cost, since publishing is just a CAS.

good catch.
actually, moving setImpl wasn't enough. In this case there was a short window there deleted label was still visible in the backend.
I added a test for this case as well.

alonre24 · 2026-10-14T09:46:33Z

-            if (this->backendIndex->indexSize() == 0) {
-                // If backend index is empty, we need to initialize it first.
+            // Keep tracking surviving snapshot IDs; new arrivals use insert jobs.
+            initInProgress = true;


Nit: initInProgress (and initDeletedLabels.reserve() above) are written under a shared flatIndexGuard. That's safe today, because every reader of these fields holds the lock exclusively and only one init runs at a time, but it's easy to break later. Please either take the lock exclusively for these writes, make initInProgress atomic, or add a comment documenting the invariant.

A comment is added

cursor

Stale Bugbot comment from a previous run.

alonre24

Thanks, the init fix looks good: the main thread no longer waits on the build, setImpl is now published only after the writes made during the build are applied, and the new tests cover both. Two follow-ups inline. The first one is about worker threads getting tied up during the build.

alonre24 · 2026-10-15T15:21:33Z

+            }
+
+            // Insert job to the queue and signal the workers' updater.
+            this->submitSingleJob(new_insert_job);


Insert jobs submitted during the initial build tie up workers.

While initInProgress is set, this submits the insert job right away. executeInsertJobWrapper takes updateJobMutex shared, and initSVSIndexWrapper holds it exclusively for the whole build. So every idle worker that picks up one of these jobs blocks until the build ends.

RediSearch submits index jobs with THPOOL_PRIORITY_LOW, but priority only orders the queue; a worker that has already picked up a job stays stuck. With steady HSETs during a build that can take tens of seconds, every worker the build itself isn't using soon sits blocked on one of these jobs. Queries, which run on the same pool, then wait for the build to end. On main, writes during an update only buffered and created no jobs, so this is a regression there.

Suggestion: while initInProgress is set, still create the job and register it in labelToInsertJobs as now, but don't submit it yet. Add it to a list kept under flatIndexGuard (like initDeletedLabels). Then have initSVSIndexWrapper submit the list after it releases updateJobMutex, the same way it schedules the consolidate jobs for the deleted labels (and the way relabel resubmits parked jobs).

alonre24 · 2026-10-15T15:21:33Z

+                // with the given label.
+                if (initInProgress && !this->labelToInsertJobs.count(label)) {
+                    // A captured row has no insert job; cancel it through the journal.
+                    ret = -removeLabelFromFlat(label);


Could you add a test for this branch? It's for overwriting a label that was copied into the build and has no insert job yet. The new tests cover additions and deletes during the build, but not overwrites. I traced it by hand and it looks correct, but it's the trickiest part of the bookkeeping. Something like: pause at UpdateJob::after_snapshot, overwrite a copied label (single-value), then check that addVector returns 0 and that after init the label has exactly the new vector.

cursor

Cursor Bugbot has reviewed your changes using high 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 cad9c48. Configure here.}

nonirosenfeldredis

Review of 6e63a369 (rc10; modify after init drain logic) and 6b9fed86 (fix Init scheduled under exclusive flat lock).

nonirosenfeldredis · 2026-10-19T17:53:14Z

+        if (!insert_jobs.empty()) {
+            const size_t num_jobs = insert_jobs.size();
+            std::atomic<size_t> next_job{0};
+            index->GetSVSIndex()->runParallel(


Insert jobs now run synchronously in the init job instead of going through submitJobs. Two concerns:

Exceptions leak jobs. parallel_for rethrows worker failures as ThreadingException. If an executeInsertJobWrapper call throws, the remaining insert_jobs are never executed or deleted. Their labelToInsertJobs entries stay dangling and the exception escapes the job callback. The old submitJobs path had no such failure mode. Can we catch around the loop and clean up or resubmit the leftover jobs?

Latency on the in-place path. With initSVSIndexWrapper(this, 1) from tryBufferForTraining, the thread calling addVector now runs every buffered insert job inline. That seems intended, but please say so in the commit message or a comment. I believe the nested parallel_for inside executeInsertJob is safe only because init resets setParallelism(1) before this point (n==1 runs inline, no rental). A comment saying so would help.

Also <atomic> and <algorithm> look like they're relied on transitively. Please include them. No tests changed in this commit. Do any existing tests assume insert jobs are queued after init?

My mistake. Insert-job exceptions were already unrecoverable (no handling in executeInsertJob and none in the old submitJobs path), so cleanup here makes no sense. Please ignore point 1. Point 2 and the includes still apply.

Latency on the in-place path. With initSVSIndexWrapper(this, 1) from tryBufferForTraining, the thread calling addVector now runs every buffered insert job inline. That seems intended, but please say so in the commit message or a comment. I believe the nested parallel_for inside executeInsertJob is safe only because init resets setParallelism(1) before this point (n==1 runs inline, no rental). A comment saying so would help.

done

Also <atomic> and <algorithm> look like they're relied on transitively. Please include them. No tests changed in this commit. Do any existing tests assume insert jobs are queued after init?

done

nonirosenfeldredis · 2026-10-19T17:53:14Z

+            this->flatIndexGuard.unlock_shared();
+            this->flatIndexGuard.lock();
+            if (initInProgress && this->frontendIndex->indexSize() >=
+                                      std::max(MIN_MAIN_LOCK_LIMIT, trainingTriggerThreshold)) {


The handoff looks right to me. initInProgress is only true while init holds updateJobMutex exclusively, and it's cleared before release, so the shared_lock wait is safe. Dropping flatIndexGuard first is also needed for init's publish step.

Nits and questions:

MIN_MAIN_LOCK_LIMIT is hard to read. Could it be named for what it does, e.g. INIT_BACKPRESSURE_LIMIT, with a comment explaining why 100000?

Writers can now block for the whole init build once the flat buffer reaches the limit. Is that acceptable for callers on the main thread?

There's no test for this path. Could we use the tracing callbacks (before_publish or similar) to hold init open and assert that addVector waits and then succeeds?

A loop would be clearer than recursing into addVector.

MIN_MAIN_LOCK_LIMIT is hard to read. Could it be named for what it does, e.g. INIT_BACKPRESSURE_LIMIT, with a comment explaining why 100000?

done

Writers can now block for the whole init build once the flat buffer reaches the limit. Is that acceptable for callers on the main thread?

it's not a regression vs. main.
in the main branch main thread was blocked during initialization as well, because of holding mainIndexGuard shared .

There's no test for this path. Could we use the tracing callbacks (before_publish or similar) to hold init open and assert that addVector waits and then succeeds?

added

A loop would be clearer than recursing into addVector.

a loop would be more complicated, because the backend state changes after this wait, i.e svs_index->ready() becomes true, and loop would introduce one more dimension of locking danse. Looks like recursion is a clear way to reuse the existing code.

nonirosenfeldredis · 2026-10-19T17:53:14Z

+            ret = std::max(this->frontendIndex->addVector(blob, label) - deleted, 0);
+            run_init = ids_to_init_.size() >= this->trainingTriggerThreshold;
+        }
+        if (run_init && !in_place) {


LGTM. Moving scheduleSVSIndexInit() out of the exclusive flatIndexGuard scope is right. Duplicate scheduling is still guarded by indexUpdateScheduled.test_and_set().

ofiryanai and others added 30 commits April 20, 2026 17:42

initial

4e0412e

optimization

3f987c1

fix threadpool

f2fe02f

minor fix

f806dca

minor cleaning

28503c2

clean up

f2596b3

remove dead code

6dc0722

fixes; tests pass

402691f

refactoring

139161f

fixes for GC

f8efbbe

copy blob before flat unlock

cfca2f5

Remove null characters from end of file

dcbdf54

Merge pull request #1 from mihaic/lockfree-fix-end

fe7dd9c

Remove null characters from end of file

fixes for gc

aeae737

Merge branch 'lockfree' of https://github.com/razdoburdin/VectorSimil…

47508f2

…arity into lockfree

fix writer starvation for compaction

07cd22f

adopt sigle label consolidation

f24187b

vecsim separate fin-grain index enabling

f4a8f52

CI fixes

baff6de

clean-up commented and unused dead code

325f7b5

harmonize test

1051095

Merge branch 'main' into dev/eglaser-lockfree

63758e1

minor revisions for CI

b8b57e3

fix build and tests

b530842

Merge branch 'dev/eglaser-lockfree' of https://github.com/ethanglaser…

7b25c70

…/VectorSimilarity into dev/eglaser-lockfree

fix ub

7e66fea

use SVS rc

7213909

alonre24 added the bm-basics-svs-fp32-single label Oct 8, 2026

don't lock mainthread while training

315a89b

alonre24 reviewed Oct 14, 2026

View reviewed changes

razdoburdin added 5 commits October 15, 2026 04:01

buffer added vectors during initSVSIndex

6f76269

close delete-after-impl_publish window

3b2f08e

update initDeletedLabel comment

9521bcf

update relabelVector comment

9092cb6

format

9ce1c6d

cursor Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread src/VecSim/algorithms/svs/svs.h

add test

46dfcd1

alonre24 reviewed Oct 15, 2026

View reviewed changes

razdoburdin and others added 4 commits October 16, 2026 02:04

don't create insert jobs while initilizing svs; add tests

24cb709

format

548930a

SVS rc8

7878b1c

SVS rc9

b455ebc

ethanglaser force-pushed the dev/eglaser-lockfree branch from 720d8df to b455ebc Compare October 16, 2026 22:14

ethanglaser mentioned this pull request Oct 16, 2026

bench fixes and SVS rc10 #1077

Closed

2 tasks

razdoburdin added 2 commits October 19, 2026 01:58

rc10; modify after init drain logic

6e63a36

format

cad9c48

cursor Bot reviewed Oct 19, 2026

View reviewed changes

Comment thread src/VecSim/algorithms/svs/svs_tiered.h Outdated

fix Init scheduled under exclusive flat lock

6b9fed8

nonirosenfeldredis reviewed Oct 19, 2026

View reviewed changes

razdoburdin added 2 commits October 19, 2026 12:28

adress comments: add more descriptions and tests

36ff941

format

c117c24

nonirosenfeldredis enabled auto-merge October 19, 2026 19:47

nonirosenfeldredis approved these changes Oct 19, 2026

View reviewed changes

nonirosenfeldredis added this pull request to the merge queue Oct 19, 2026

Merged via the queue into main with commit a59dd72 Oct 19, 2026
29 checks passed

nonirosenfeldredis deleted the dev/eglaser-lockfree branch October 19, 2026 21:18

Conversation

razdoburdin commented Sep 23, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razdoburdin Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nonirosenfeldredis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

razdoburdin commented Sep 23, 2026 •

edited by cursor Bot

Loading

razdoburdin Oct 15, 2026 •

edited

Loading

cursor Bot left a comment •

edited

Loading