Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ofiryanai merged 2 commits into from
Apr 19, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f512944
MOD-14916 Devirtualize distance + getElement on HNSW search hot path
ofiryanai Apr 17, 2026
341ed12
MOD-14916 Inline DataBlocksContainer::getElement on HNSW search hot path
ofiryanai Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/VecSim/algorithms/brute_force/brute_force.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ class BruteForceIndex : public VecSimIndexAbstract<DataType, DistType> {
size_t indexCapacity() const override;
std::unique_ptr<RawDataContainer::Iterator> getVectorsIterator() const;
const DataType *getDataByInternalId(idType id) const {
return reinterpret_cast<const DataType *>(this->vectors->getElement(id));
// `vectors` is always a DataBlocksContainer; skip the RawDataContainer vtable.
return reinterpret_cast<const DataType *>(
static_cast<const DataBlocksContainer *>(this->vectors)
->DataBlocksContainer::getElement(id));
}
VecSimQueryReply *topKQuery(const void *queryBlob, size_t k,
VecSimQueryParams *queryParams) const override;
Expand Down
4 changes: 3 additions & 1 deletion src/VecSim/algorithms/hnsw/hnsw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,9 @@ labelType HNSWIndex<DataType, DistType>::getEntryPointLabel() const {

template <typename DataType, typename DistType>
const char *HNSWIndex<DataType, DistType>::getDataByInternalId(idType internal_id) const {
return this->vectors->getElement(internal_id);
// `vectors` is always a DataBlocksContainer; skip the RawDataContainer vtable on the hot path.
return static_cast<const DataBlocksContainer *>(this->vectors)
->DataBlocksContainer::getElement(internal_id);
}

template <typename DataType, typename DistType>
Expand Down
5 changes: 0 additions & 5 deletions src/VecSim/containers/data_blocks_container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@ RawDataContainer::Status DataBlocksContainer::addElement(const void *element, si
return Status::OK;
}

const char *DataBlocksContainer::getElement(size_t id) const {
assert(id < element_count);
return blocks.at(id / this->block_size).getElement(id % this->block_size);
}

RawDataContainer::Status DataBlocksContainer::removeElement(size_t id) {
assert(id == element_count - 1); // only the last element can be removed
blocks.back().popLastElement();
Expand Down
6 changes: 5 additions & 1 deletion src/VecSim/containers/data_blocks_container.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ class DataBlocksContainer : public VecsimBaseObject, public RawDataContainer {

Status addElement(const void *element, size_t id) override;

const char *getElement(size_t id) const override;
// Inlined so the hot search path (via getDataByInternalId) can fold the indexing arithmetic.
const char *getElement(size_t id) const override {
assert(id < element_count);
return blocks[id / block_size].getElement(id % block_size);
}

Status removeElement(size_t id) override;

Expand Down
5 changes: 5 additions & 0 deletions src/VecSim/spaces/computer/calculator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class IndexCalculatorInterface : public VecsimBaseObject {
virtual ~IndexCalculatorInterface() = default;

virtual DistType calcDistance(const void *v1, const void *v2, size_t dim) const = 0;

// Raw distance function; cached by the index to skip the vtable on the hot path.
virtual spaces::dist_func_t<DistType> getDistFunc() const = 0;
};

/**
Expand Down Expand Up @@ -56,4 +59,6 @@ class DistanceCalculatorCommon
DistType calcDistance(const void *v1, const void *v2, size_t dim) const override {
return this->dist_func(v1, v2, dim);
}

spaces::dist_func_t<DistType> getDistFunc() const override { return this->dist_func; }
};
16 changes: 13 additions & 3 deletions src/VecSim/vec_sim_index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ struct VecSimIndexAbstract : public VecSimIndexInterface {
RawDataContainer *vectors; // The raw vectors data container.
private:
IndexCalculatorInterface<DistType> *indexCalculator; // Distance calculator.
spaces::dist_func_t<DistType> cachedDistFunc; // Cached dist func, used on the hot path.
PreprocessorsContainerAbstract *preprocessors; // Storage and query preprocessors.

size_t inputBlobSize; // The size of input vectors/queries blob in bytes. May differ from dim *
Expand Down Expand Up @@ -124,8 +125,11 @@ struct VecSimIndexAbstract : public VecSimIndexInterface {
metric(params.metric),
blockSize(params.blockSize ? params.blockSize : DEFAULT_BLOCK_SIZE), lastMode(EMPTY_MODE),
isMulti(params.multi), isDisk(params.isDisk), logCallbackCtx(params.logCtx),
indexCalculator(components.indexCalculator), preprocessors(components.preprocessors),
inputBlobSize(params.inputBlobSize), storedDataSize(params.storedDataSize) {
indexCalculator(components.indexCalculator),
cachedDistFunc(components.indexCalculator ? components.indexCalculator->getDistFunc()
: nullptr),
Comment on lines +128 to +130

@GuyAv46 GuyAv46 Apr 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@ofiryanai ofiryanai Apr 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

@ofiryanai ofiryanai Apr 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 GuyAv46 Apr 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 ofiryanai Apr 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

preprocessors(components.preprocessors), inputBlobSize(params.inputBlobSize),
storedDataSize(params.storedDataSize) {
assert(VecSimType_sizeof(vecType));
assert(storedDataSize);
assert(inputBlobSize);
Expand All @@ -146,10 +150,16 @@ struct VecSimIndexAbstract : public VecSimIndexInterface {
/**
* @brief Calculate the distance between two vectors based on index parameters.
*
* Uses the cached dist func to avoid the indexCalculator vtable on the hot path.
*
* @note Precondition: @c cachedDistFunc must be non-null. Subclasses that construct
* this index with a null @c indexCalculator (e.g. SVS, which uses its own
* internal distance kernels) must not call this method.
*
* @return the distance between the vectors.
*/
DistType calcDistance(const void *vector_data1, const void *vector_data2) const {
return indexCalculator->calcDistance(vector_data1, vector_data2, this->dim);
return cachedDistFunc(vector_data1, vector_data2, this->dim);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/test_components.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ class DistanceCalculatorDummy : public DistanceCalculatorInterface<DistType, dum
virtual DistType calcDistance(const void *v1, const void *v2, size_t dim) const {
return this->dist_func(7);
}

// Dummy uses a non-standard dist func signature, so the standard slot is unavailable.
spaces::dist_func_t<DistType> getDistFunc() const override { return nullptr; }
};

} // namespace dummyCalcultor
Expand Down
Loading