Support fine-grained SVS index - #1027
Draft
ethanglaser wants to merge 21 commits into
Draft
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. |
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
5 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.
Describe the changes in the pull request
A clear and concise description of what the PR is solving.
Which issues this PR fixes
Main objects this PR modified
Mark if applicable