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
Large changes to tiered SVS/HNSW concurrency, deletion/consolidation, and relabel ordering against a new SVS binary; incorrect job or lock ordering could cause duplicate/missing vectors or query skew.

Overview
Upgrades the bundled Intel SVS prebuilt libraries to 0.5.0rc7 (nightly URLs, GLIBC ≥ 2.28 for GCC, updated replace_external_id header probe) and moves the SVS backend onto svs::concurrent mutable Vamana indexes with segmented blocked storage/graph allocators.

SVSIndex now lazily publishes the impl via an atomic pointer, routes inserts/deletes/consolidation through the concurrent API (including consolidate per label and compact() GC), and filters tombstone IDs in search results.

Tiered SVS is reworked for fine-grained ingestion: vectors leave the flat buffer via per-vector insert jobs (not a monolithic batch “update” with swap journals), backend init still batches training snapshots, and async consolidate jobs run after deletes; relabel coordinates delayed/parked insert jobs and consolidate takeover. Tiered HNSW shares the same TieredInsertJob / TieredIndex_BatchIterator and locking hooks (ScopedLocks). Python runGC now calls the index directly; tests add runGC after deletes and adjust memory estimates for concurrent graph reverse edges.

^{Reviewed by Cursor Bugbot for commit 315a89b. 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

…torSimilarity into dev/eglaser-lockfree

This reverts commit af5c9ec.

cursor

Stale Bugbot comment from a previous run.

cursor

Stale Bugbot comment from a previous run.

alonre24

Overall structure looks good. Please go through my comments and questions, in particular:

initialization blocking the main thread (regression vs. main)
races between GC/compaction and reads (getDataByLabel, query label translation, batch iterator)
the locking scheme (updateJobMutex vs. mainIndexGuard) and the busy re-queue of Deferred jobs
missing concurrency test coverage

alonre24 · 2026-10-03T09:15:25Z

+        if constexpr (!isMulti) {
+            // SVS index does not support overriding vectors with the same label
+            // so we have to delete them first if needed
+            deleted_num = deleteVectorsImpl(labels, n);


I'm not sure that this is true anymore... @nonirosenfeldredis can you chime in?

that's right. we have VecSimUpdateCode updateVectors method implemented in each algo , including svs and tiered svs

SVS still doesn't support overriding vectors.
The updateVectors method in svs.h, you have mentioned, removes the vector first than adds a new one.

alonre24 · 2026-10-03T09:31:38Z

-        for (size_t i = 0; i < n; i++) {
-            if (impl_->has_id(labels[i])) {
-                entries_to_delete.push_back(labels[i]);
+    int deleteVectorsImpl(const labelType *labels, size_t n) {


Do we still need this version of deleting multiple labels? Where is that called?

addVectorsImpl call deleteVectorsImpl
and addVectorsImpl may be called with n > 1 through the python-bindings

Why do Python bindings still use it if it isn't reachable? Consider adjusting the bindings so they exercise the real flows.

alonre24 · 2026-10-03T10:31:38Z

        { // lock frontendIndex from modifications
-            std::shared_lock flat_lock{this->flatIndexGuard};
+            // The whole initialization is done under flatIndexGuard
+            std::lock_guard flat_lock{this->flatIndexGuard};


Regression: in async mode the main thread blocks for the whole initial build.
This holds flatIndexGuard exclusively (and updateJobMutex) across createImpl: compression training, entry point, and the full build_graph over the training batch (10K vectors by default, up to 100K). For that whole time, addVector/deleteVector (through tryBufferForTraining) and queries wait on this lock.
On main, updateSVSIndex copied the batch under a shared flat lock, released it, built the impl under mainIndexGuard shared, and took exclusive locks only to publish the impl and clean up flat. It used the swap/delete journals to reconcile changes made during the build.
Can we keep that shape? Copy under the lock, build without it, then lock briefly to setImpl and drop the moved ids from flat, applying any deletes/overwrites that happened in the meantime.

The scheme being used on main is possible, but it introduces additional level of bookkeeping machinery. In my experiments I observed performance degradation with it.

But have you taken into consideration that the redis main thread would be blocked during training since incoming HSET commands would wait for this lock? Let's discuss that

alonre24 · 2026-10-03T10:39:22Z

            return;
        }
        index->executeTracingCallback("GCJob::before_run_gc");
+        std::lock_guard<std::shared_mutex> lock(index->updateJobMutex);


GC no longer takes mainIndexGuard (the doc comment above still says it does). For SVS, nothing takes mainIndexGuard exclusive any more, so the shared locks the base class still takes protect nothing: getDataByLabel, indexLabelCount, debugInfo, debugInfoIterator in vec_sim_tiered_index.h.

Suggestion: use mainIndexGuard in place of updateJobMutex, which already has the same role: exclusive for init/GC/in-place add, shared for insert/consolidate/direct add/relabel. That makes those base-class locks meaningful again (it fixes the getDataByLabel vs. compaction race, and makes debugInfo safe during GC), and it matches HNSW and main.
A plain rename deadlocks, though. Init takes main -> flat exclusive, while the base-class readers take flat -> main shared. Init would need std::scoped_lock(mainIndexGuard, flatIndexGuard), like main does. GC and in-place add never take flat while holding main, so they're fine.
Trade-off: GC/init would compete with more shared holders (base-class readers, and queries if lockMainIndexForQuery is re-enabled). That doesn't fix the multi-value label-map race, since both sides hold the lock shared.

GC issues you have found a fixed. We are going to update svs asap.
GC now takes mainIndexGuard to protect baseclass members.

alonre24 · 2026-10-04T07:53:47Z

@@ -4013,11 +4302,12 @@ TYPED_TEST(SVSTieredIndexTestBasic, runGCAPI) {
    auto jobs_before_gc = mock_thread_pool.jobQ.size();
    // Run the GC API call, expect that we will clean up the SVS index.
    VecSimTieredIndex_GC(tiered_index);


Test coverage for the concurrency changes. None of the races raised in this review would be caught by the current tests:

runGCParallel mixes add, delete and GC, then checks only final sizes.

parallelSearch, parallelBatchIteratorSearch and parallelRangeSearch never delete or run GC.

The getDataByLabel tests are single-threaded.

CI runs only AddressSanitizer (san: address).

Suggested additions:

Batch iterator across GC (deterministic): read one batch, then delete some labels, run GC and wait for it, then read the remaining batches. Assert no duplicates across batches, no deleted labels, and every distance equal to the distance recomputed from the label's stored vector.

Queries during compaction (stress test): query threads run topKQuery/rangeQuery in a loop while the main thread deletes labels from a separate range and keeps triggering GC. For every result, assert the label was inserted at some point (catches internal ids returned as labels). For labels that are never deleted, also assert the distance matches the stored vector (catches a label paired with the wrong distance).

getDataByLabel during GC and during inserts, single- and multi-value, comparing against the expected vectors on labels that are never deleted. Under ASan, this catches the span copied after compaction and the unlocked label-map iteration.

Init doesn't block the main thread: from the UpdateJob::before_add_to_svs tracing callback, start a thread that calls addVector/topKQuery, and assert it finishes within a timeout.

A ThreadSanitizer CI job, at least nightly. The concurrent index is template code compiled into VecSim, so it would be instrumented. Parts of SVS that come prebuilt may need suppressions.

I updated tests for 1-3

alonre24 · 2026-10-04T11:46:09Z

-     * rename landing inside its window would be invisible to it and the vector would reach the
-     * backend under the old label.
     */
    VecSimRelabelCode relabelVector(labelType old_label, labelType new_label) override {


@nonirosenfeldredis can you please review the relabelVector changes in tiered SVS? That covers the Delayed/Deferred insert-job handling and the consolidate takeover (takeOverConsolidateOf).

I reviewed this at PR head 6d3ffa7d. I see two blocking correctness issues and a few additional concerns:

Possible UAF in takeOverConsolidateOf. It saves raw pointers to executing jobs under consolidateJobsGuard, releases that guard, and then calls waitForJob(job). The worker can set executing=false, remove the job from the registry, and delete it before or during the next atomic load by the waiter. The existing test manually toggles executing and does not exercise real worker deletion. Please preserve job lifetime while waiting; for example, retain the shared registry guard through the wait, since the worker clears executing before acquiring the exclusive registry guard, or use ref-counted job state.

Concurrent add can leave stale and new vectors under one single-value label. Relabel releases flatIndexGuard before the backend rename. Example: old -> V1 is backend-only; relabel verifies new is free and unlocks flat; concurrent add buffers new -> V2 and its backend delete finds no new; then relabel changes backend old -> new. After both calls return, flat has new -> V2 and backend has stale new -> V1 until the insert job eventually runs. Queries can use the stale score and lose recall. Delete operations on old or new have analogous missed-tier/resurrection interleavings. The shared updateJobMutex does not prevent this because the normal async add/delete paths do not participate in it.

A coarse correctness fix would be to retain the exclusive flat guard through consolidation, backend relabel, rollback, and re-enabling delayed jobs. A more scalable fix is per-label mutation locking, acquired in canonical order for old/new and used by add, delete, relabel, and job publication.

Additional points:

When a label exists in both tiers, a query can run after flat old -> new but before backend old -> new, observe both IDs, and lose a top-k slot. Retaining the query/flat guard across the full transition would make the operation linearizable to queries.

Only NewLabelTaken is handled from the backend. OldLabelMissing is treated as success. That is valid for a flat-only source, but wrong if the backend source was observed and concurrently deleted; preserve the original tier state and distinguish these cases.

If consolidate() or backend relabel throws after jobs become Delayed and flat is renamed, there is no rollback guard. Jobs can remain deferred indefinitely. An RAII rollback/un-delay guard would make this safe.

Tests I would add: real-worker consolidation deletion under ASan/TSan; deterministic add/delete and query hooks between flat and backend phases; and exception injection verifying that no job remains Delayed.

Fixed

addVector and relabelVector are serialized, because they are both launched from the main thread.

cursor

Stale Bugbot comment from a previous run.

cursor

Stale Bugbot comment from a previous run.

cursor

Stale Bugbot comment from a previous run.

cursor

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{❌ 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 38c48eb. Configure here.}

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

razdoburdin and others added 6 commits September 29, 2026 04:16

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

93ca504

…torSimilarity into dev/eglaser-lockfree

update to SVS rc4 binaries

39d98be

guard tests

4ce4ad7

Revert "guard tests"

368dbd6

This reverts commit af5c9ec.

update SVS to rc5

46e6da9

add more tests

0cc22dc

cursor Bot reviewed Sep 30, 2026

View reviewed changes

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

merge main

731b6ed

cursor Bot reviewed Sep 30, 2026

View reviewed changes

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

SVS rc6

261b0f1

alonre24 reviewed Oct 4, 2026

View reviewed changes

razdoburdin force-pushed the dev/eglaser-lockfree branch from 6d3ffa7 to 261b0f1 Compare October 5, 2026 08:24

cursor Bot reviewed Oct 5, 2026

View reviewed changes

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

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

razdoburdin added 3 commits October 5, 2026 02:02

merge main

46fc96b

format

732e5e5

refactor switch-case in relabelVector

f14687a

cursor Bot reviewed Oct 5, 2026

View reviewed changes

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

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

razdoburdin added 2 commits October 5, 2026 05:15

remove unused index_

7578eee

reduce flatIndexGuard scope

a34064d

cursor Bot reviewed Oct 5, 2026

View reviewed changes

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

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

razdoburdin and others added 6 commits October 6, 2026 00:03

adopt set once for svs pimpl

5b9be33

fixes and additional tests

673174b

format

a54595b

merge main

c68edce

GC takes mainIndexGuard to protect baseclass methods

a9616ac

bump to SVS rc7

38c48eb

cursor Bot reviewed Oct 7, 2026

View reviewed changes

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

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

alonre24 added bm-svs-train-fp32 bm-basics-svs-fp32-single labels Oct 8, 2026

don't lock mainthread while training

315a89b

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

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!

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!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

razdoburdin commented Sep 23, 2026 •

edited by cursor Bot

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading