Skip to content

Support fine-grained SVS index - #1027

Open
ethanglaser wants to merge 67 commits into
RedisAI:mainfrom
ethanglaser:dev/eglaser-lockfree
Open

ethanglaser wants to merge 67 commits into
RedisAI:mainfrom
ethanglaser:dev/eglaser-lockfree

Conversation

@ethanglaser

@ethanglaser ethanglaser commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

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.

  1. 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.
  2. 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 ingestion, delete/consolidate concurrency, and locking around a new concurrent index dependency affect correctness of search and updates under load.

Overview
This PR moves VecSim onto SVS 0.5.0 rc2 prebuilt binaries and switches the backend from static Vamana types to svs::concurrent mutable indexes with segmented blocked storage/graph allocators so readers can query while writers append.

SVSIndex now keeps the implementation in a shared_ptr guarded by mutexes, can stay empty until the first insert (ready()), and routes adds/deletes/consolidation/compact through that concurrent API—including replace_external_id when the new headers provide it. Tiered SVS is reworked: instead of periodic batch snapshots with swap journals, it buffers training in the flat tier, initializes the backend once, then ingests remaining vectors via per-label insert jobs (with defer/retry while init runs) and schedules consolidate jobs after deletes so soft-deleted labels don’t block relabels. Query paths rely less on mainIndexGuard because the concurrent backend serializes internally.

Shared tiered plumbing (TieredInsertJob, invalid-job tracking, flat removal after ingest, indexSize/getDistanceFrom locking hooks via new ScopedLocks) is lifted into VecSimTieredIndex, with tiered HNSW adapted to match. Tests and size estimates account for reverse-edge overhead; Python runGC calls the index directly so non-tiered SVS compacts too.

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

ofiryanai and others added 21 commits April 20, 2026 17:42
* 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
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 RedisAI#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>
@CLAassistant

CLAassistant commented Aug 27, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 4 committers have signed the CLA.

✅ ofiryanai
❌ Dmitry Razdoburdin
❌ razdoburdin
❌ ahuber21


Dmitry Razdoburdin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ethanglaser
ethanglaser requested a review from rfsaliev August 27, 2026 05:10
@ethanglaser ethanglaser changed the title Dev/eglaser lockfree Support fine-grained SVS index Aug 27, 2026
Dmitry Razdoburdin and others added 28 commits September 14, 2026 00:20
Harmonize lockfree SVS and HNSW
Resolve conflicts:
- deps/ScalableVectorSearch: keep 3ee890d (already contains a7e3494 replace_external_id)
- svs_tiered.h: keep upstream's relabelVector override after the rewritten deleteVector

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@razdoburdin
razdoburdin marked this pull request as ready for review September 16, 2026 09:26

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 using high effort and found 4 potential issues.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 9e602e9. Configure here.

}

std::shared_lock<std::shared_mutex> lock(updateJobMutex);
return ret = svs_index->addVector(storage_blob.get(), label);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Full-buffer overwrite skips pending jobs

High Severity

When the frontend is at flat_buffer_bound, addVector writes straight to the SVS backend and only soft-deletes that backend copy. It never invalidates a pending insert job or removes the label from the frontend. A later ingest can put the old blob back (and addVectorsImpl can drop the new one), and the call returns 1 for an overwrite instead of 0.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e602e9. Configure here.

ret -= this->backendIndex->deleteVector(label);
ids_to_init_.insert(this->frontendIndex->indexSize());
const auto ft_ret = this->frontendIndex->addVector(blob, label);
ret = std::max(ret + ft_ret, 0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Init overwrite reports a new insert

Medium Severity

During the training/init buffer, a single-value overwrite calls deleteAndUpdateInitIds and then frontendIndex->addVector, which treats the write as a new row and returns 1. Callers that use 0 vs 1 to distinguish update from insert will count an extra vector until the backend is initialized.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e602e9. Configure here.

* case when built against an SVS without `replace_external_id`. All-or-nothing: on any code
* other than `VecSimRelabel_OK` both tiers are untouched.
*
* `updateJobMutex` is taken first, in the order `updateSVSIndex` takes its own locks. Holding

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Empty index skips compression retraining

High Severity

backendInitSubmited is set once when the first training batch is scheduled and never cleared. After the last backend vector is deleted and impl_ is reset, later async adds skip the training buffer and let a single insert job initImpl the compressed index from one point.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e602e9. Configure here.

impl->compact();
}
num_marked_deleted = 0;
num_marked_deleted.store(0, std::memory_order_relaxed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

GC compacts without consolidating deletes

Medium Severity

runGC now only calls compact() and zeroes num_marked_deleted. Per-label consolidate jobs are not drained first, so compact can run while deleted graph edges are still unpatched. SVS documents compact as the step that follows consolidate.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e602e9. Configure here.

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.

6 participants