MOD-14916 Devirtualize HNSW / brute-force search hot path

[ofiryanai]

Summary

Devirtualizes the HNSW / brute-force search hot path so the per-candidate distance and vector-fetch calls compile down to direct calls instead of going through IndexCalculatorInterface and RawDataContainer vtables.

The SIMD distance kernels themselves are unchanged — what changes is the scalar "glue" around them (vtable dispatch, an out-of-line getElement, and a runtime bounds check), which is exercised on the order of visited_candidates × M times per query and was preventing the optimizer from inlining the fetch path.

Net diff: 7 files, +27 / −10 lines. Public APIs are unchanged.

Changes

1. Cache the raw distance function in VecSimIndexAbstract (vec_sim_index.h, calculator.h)

   • Adds IndexCalculatorInterface::getDistFunc() to expose the underlying dist_func_t<DistType>.
   • VecSimIndexAbstract caches that pointer at construction time as cachedDistFunc and calls it directly in calcDistance, skipping the indexCalculator virtual dispatch on every per-candidate distance call.

2. Devirtualize getDataByInternalId (hnsw.h, brute_force.h)

   • VecSimIndexAbstract::vectors is constructed as a DataBlocksContainer and never reassigned, so both HNSW and brute-force statically downcast and qualify the call (DataBlocksContainer::getElement) to bypass the RawDataContainer vtable.

3. Inline DataBlocksContainer::getElement (data_blocks_container.{h,cpp})

   • Moves the implementation into the header so the index-arithmetic (id / block_size, id % block_size) and the vector-block lookup are visible to the compiler at every call site and can be folded into the surrounding loop.
   • Switches the inner block lookup from blocks.at(...) to blocks[...]; the existing assert(id < element_count) guards the same precondition in debug builds.

The abstractions (IndexCalculatorInterface, RawDataContainer) are kept intact; this only short-circuits them on the in-memory HNSW/BF path where the concrete type is statically known.

Why it helps

HNSW search performs roughly visited_candidates × M paired calls of getDataByInternalId(...) followed by calcDistance(...) per query, so a few cycles per call compound quickly:

• One vtable lookup avoided per calcDistance call.
• One vtable lookup + one non-inlinable function call + one runtime bounds check avoided per getDataByInternalId call.
• With getElement and getDataByInternalId both visible to the optimizer, the indexing arithmetic and the SIMD kernel can share registers and stay in the same call frame inside processCandidate.

Benchmarks

End-to-end replay of a production-representative FT.AGGREGATE / FT.SEARCH workload (vector index, ~1M vectors) using memtier_benchmark, 4 threads × 10 connections, 60 s, comparing a build with this PR applied to a stock build:

Workload	Stock — ops/sec	This PR — ops/sec	Stock — p99	This PR — p99
FT.AGGREGATE	124	272	1,352 ms	549 ms
FT.SEARCH	1.95	2.62	954 ms	369 ms

≈ +119 % throughput and −54 % avg latency on the aggregate workload, with the largest gains in the tail (p99 / p99.9). The replay path is noisy run-to-run, so the overall throughput delta should be read as "in the same ballpark, consistently above stock"; the per-call latency reduction in the HNSW hot path is the primary, repeatable signal. The SIMD kernels are unchanged — the wins come entirely from removing the scalar glue around them.

Risk

• No public-API changes; all changes are confined to the in-memory HNSW/BF path.
• DataBlocksContainer::getElement keeps the same assert(id < element_count) precondition; the only difference vs. the previous implementation is dropping std::deque::at's redundant range check (the outer assert already covers the same precondition).
• The new getDistFunc() virtual is implemented in DistanceCalculatorCommon (the only production calculator) and in the dummy test calculator; no other implementations exist in tree.
• Existing unit tests cover the hot path (tests/unit/test_components.cpp, tests/unit/test_hnsw*.cpp).

Backports

After merge, plan to backport to 8.4 and 8.2 branches under the same MOD-14916 ticket.

Ticket

• MOD-14916

---

Pull Request opened by Augment Code with guidance from the PR author

---

Note

Medium Risk
Touches core HNSW/brute-force search hot paths by bypassing virtual dispatch and changing how distances/vectors are fetched; while intended as perf-only, mistakes could cause crashes (e.g., null cached dist func) or subtle behavior differences under edge configurations.

Overview
Devirtualizes the HNSW and brute-force search hot path to reduce per-candidate overhead during queries.

VecSimIndexAbstract now caches the raw dist_func_t via a new IndexCalculatorInterface::getDistFunc() and uses it directly in calcDistance, avoiding virtual dispatch on every distance calculation. HNSW and brute-force getDataByInternalId now downcast vectors to DataBlocksContainer and call DataBlocksContainer::getElement directly, and DataBlocksContainer::getElement is inlined in the header (dropping deque::at bounds checking in favor of an assert) to enable better inlining/index-arithmetic optimization.

^{Reviewed by Cursor Bugbot for commit 341ed12. Bugbot is set up for automated code reviews on this repo. Configure here.}

[jit-ci]

🛡️ Jit Security Scan Results

✅ No security findings were detected in this PR

---

^{Security scan by Jit}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.89%. Comparing base (7f01bfa) to head (341ed12).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
- Coverage   96.91%   96.89%   -0.03%     
==========================================
  Files         129      129              
  Lines        7651     7656       +5     
==========================================
+ Hits         7415     7418       +3     
- Misses        236      238       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[GuyAv46]

When is it nullptr? We later call it unchecked in calcDistance, so only for testing?

[ofiryanai]

I think this is what fixed the svs sanitizer failure. checking

[ofiryanai]

Confirmed — it's nullptr in production for SVS, not just tests.

src/VecSim/index_factories/svs_factory.cpp:32-33:

IndexComponents<svs_details::vecsim_dt<DataType>, float> components = {
    nullptr, preprocessors}; // calculator is not in use in svs.

SVS uses its own internal distance kernels (via the MetricType template param from the Intel SVS library) and never calls VecSimIndexAbstract::calcDistance. Same reason it also doesn't populate this->vectors — it has its own index_storage_type. So the unchecked call in calcDistance is safe: only HNSW/BF reach it, and both always have a valid indexCalculator.

The ? : is only needed in the constructor because we now eagerly cache the dist-func at construction time. Before this PR, indexCalculator was just stored as-is (SVS got away with storing a nullptr that was never dereferenced). Now we dereference it once up front, so SVS's path needs the guard.

[GuyAv46]

So, we keep calcDistance as is, and expect it to crash if the dist func is set to nullptr and later we attempt to call it. Right?
Consider documenting that

[ofiryanai]

I'll add docs/assert as soon as the benchmarks finish, I don't want to cancel them now