[8.4] MOD-14916 Devirtualize HNSW / brute-force search hot path - #939
Merged
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)
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
ofiryanai
marked this pull request as ready for review
April 20, 2026 12:55
GuyAv46
approved these changes
Apr 20, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.4 #939 +/- ##
==========================================
- Coverage 97.06% 97.04% -0.03%
==========================================
Files 126 126
Lines 7465 7470 +5
==========================================
+ Hits 7246 7249 +3
- Misses 219 221 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ofiryanai
enabled auto-merge
April 20, 2026 13:36
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Apr 20, 2026
Merged
4 tasks
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
2 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.
Description
Backport of #937 to
8.4.Pull Request opened by Augment Code with guidance from the PR author
Note
Medium Risk
Touches core search hot paths by bypassing virtual dispatch for vector access and distance computation; incorrect assumptions about container/calculator types could cause crashes or subtle correctness regressions.
Overview
Devirtualizes core VecSim search paths to reduce per-distance overhead in HNSW and brute-force queries.
Indexes now cache the raw distance function pointer (
cachedDistFunc) fromIndexCalculatorInterface::getDistFunc()and call it directly inVecSimIndexAbstract::calcDistance, andDataBlocksContainer::getElementis inlined while HNSW/BFgetDataByInternalIdexplicitly downcastsvectorstoDataBlocksContainerto avoidRawDataContainervtable dispatch. Unit tests are updated to implementgetDistFunc()for a dummy calculator (returningnullptrfor non-standard signatures).Reviewed by Cursor Bugbot for commit b267246. Bugbot is set up for automated code reviews on this repo. Configure here.