Support fine-grained SVS index - #1027
Open
ethanglaser wants to merge 79 commits into
Open
ethanglaser wants to merge 79 commits into
ethanglaser wants to merge 79 commits into
Conversation
* 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>
|
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. |
…/VectorSimilarity into dev/eglaser-lockfree
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>
This reverts commit aedb77c.
…/VectorSimilarity into dev/eglaser-lockfree
razdoburdin
marked this pull request as ready for review
September 16, 2026 09:26
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
added 5 commits
September 16, 2026 04:50
…/VectorSimilarity into dev/eglaser-lockfree
ethanglaser
commented
Sep 16, 2026
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit ca35f7c. Configure here.
| std::lock_guard<std::shared_mutex> lock(this->pimplGuard_); | ||
| this->impl_ = std::move(built); | ||
| } | ||
| this->implMutationGuard_.unlock(); |
There was a problem hiding this comment.
Insert locks leak on exception
High Severity
addVectorsImpl locks implMutationGuard_ by hand around add_points and initImpl instead of RAII. If either call throws, the mutex is never released, so later inserts, deletes, GC, and setImpl block on that guard permanently.
Reviewed by Cursor Bugbot for commit ca35f7c. Configure here.
Collaborator
There was a problem hiding this comment.
false positive.
If the exception reaches this point uncaught, execution will terminate, so mutex lock leaks are not a concern
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
6 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.
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:711andtest_vecsim_svs.py:729come from changes in internal logic not being reflected in tests yet.test_vecsim_svs.py:711.test_vecsim_svs.py:729.Note
High Risk
Large concurrent-mutation and tiered-async rewrite across SVS/HNSW locking, ingestion, delete/relabel/GC paths, plus a new prebuilt binary dependency.
Overview
Upgrades SVS to 0.5.0rc2 prebuilts and switches CMake download URLs to the nightly concurrent builds; GCC shared-lib support now requires GLIBC ≥ 2.28 (older 2.26 tarball path removed), and
replace_external_iddetection targetssvs/concurrent/dynamic_index.h.SVS backend moves from
svs::index::vamanatosvs::concurrentmutable Vamana indexes, with segmented blocked storage/graph allocators, sequence-locked graphs, and reverse-edge accounting in size estimates. The implementation is held in ashared_ptrguarded by mutexes, supports lazy init viaready()/addVectorsIfInitialized, per-labelconsolidate, and maps relabel/GC to the concurrent APIs (compact()instead of separate consolidate in GC).Tiered SVS ingestion is redesigned: batch “update with swap journal” is replaced by per-vector insert jobs, a training batch (
ids_to_init_) that runsinitSVSIndex, async consolidate jobs after deletes, and richer relabel coordination (delayed inserts, takeover of pending consolidations). Tiered batch iteration no longer holds the main index lock for SVS queries.Shared tiered infrastructure:
TieredInsertJob, invalid-job handling,removeIngestedVectorFromFlat, and defaultindexSize/getDistanceFromlive inVecSimTieredIndex;ScopedLockslets HNSW lock flat + HNSW data guard for size while tiered SVS skips backend query locks. HNSW tiered code delegates insert-job bookkeeping to the base class.Tests/bindings: memory estimates include reverse edges; flow tests call
run_gc()before memory checks; PythonrunGC()calls the index directly so non-tiered SVS compacts too.Reviewed by Cursor Bugbot for commit ca35f7c. Bugbot is set up for automated code reviews on this repo. Configure here.