Make index blocksize flexible #248

razdoburdin · 2025-12-16T09:26:24Z

This PR adds ability to set custom index blocksize
Reopening of #235

rfsaliev · 2025-12-16T12:05:35Z

Hi!
Thank you for the PR.
Looks good but I see some issues:

Extra dependencies on SVS internal headers (svs/lib/...) added to the runtime API in index_blocksize.h - I would avoid such dependencies by replacing:
- usage svs::lib::PowerOfTwo with just a size_t BlockSizeExponent(); and size_t BlockSizeBytes() const { return 1 << blocksize_exp_; }
- replace IndexBlockSize .ctor with a static factory method e.g. static Status IndexBlockSize::make(size_t blocksize_exp) noexcept; - as it was made to other runtime API classes.
The DynamicVamanaIndex::add(..., IndexBlockSize); method has unclear semantic, because blocksize argument can be applied at the first call only, but not at subsequential calls.

Instead I would propose to setup block size in a build() method:

One of solutions can be blocksize field in VamanaIndex::BuildParams - but it seems block size makes no sense in non-dynamic indices.
Another option: define a dedicated structure with parameters for dynamic indices: e.g. blocksize, reuse_empty_slots, etc.

rfsaliev

Please also read my standalone comment to the PR as well.

rfsaliev · 2025-12-16T12:07:28Z

bindings/cpp/include/svs/runtime/index_blocksize.h

+#include <cstddef>
+
+#include <svs/lib/exception.h>
+#include <svs/lib/misc.h>


Please avoid SVS internals dependencies in SVS Runtime

rfsaliev · 2025-12-16T12:08:11Z

bindings/cpp/include/svs/runtime/index_blocksize.h

+  public:
+    explicit IndexBlockSize(size_t blocksize_exp) {
+        if (blocksize_exp > kMaxBlockSizeExp) {
+            throw ANNEXCEPTION("Blocksize is too large!");


SVS runtime API should be noexcept

the IndexBlockSize structure is removed

rfsaliev · 2025-12-16T12:09:55Z

bindings/cpp/src/dynamic_vamana_index.cpp

+        return runtime_error_wrapper([&] {
+            svs::data::ConstSimpleDataView<float> data{x, n, impl_->dimensions()};
+            std::span<const size_t> lbls(labels, n);
+            impl_->add(data, lbls, blocksize);


Please convert SVS Runtime API data types (IndexBlockSize) to SVS internal type (PowerOfTwo) here.
As it was made for data argument

rfsaliev · 2025-12-16T12:11:21Z

include/svs/index/vamana/dynamic_index.h

        return slots;
    }

+    lib::PowerOfTwo blocksize_bytes() const {


I do not think it makes sense to modify SVS internals just to implement a query for SVS runtime API

fixed
The getter now calls for parameter filed, not the SVS internals

razdoburdin · 2025-12-18T14:59:32Z

Thanks for comments.

I have modified the PR, according to your request:

I have added a new structure VamanaIndex::DynamicIndexParams, being passed to build() constructor. It contains the only field blocksize_exp now, but it can be expanded.
In this case the dedicated IndexBlockSize is no longer needed, so I have removed it. The DynamicVamanaIndex class is no responsible for checking the validity of input parameters by calling check_params method.

rfsaliev

LGFM
just few comments/suggestions

rfsaliev · 2026-01-07T12:29:39Z

bindings/cpp/src/dynamic_vamana_index.cpp

 ) noexcept {
    using Impl = DynamicVamanaIndexLeanVecImpl;
    *index = nullptr;
    return runtime_error_wrapper([&] {


Does it make sense to call DynamicVamanaIndex::check_params() here as it is done in DynamicVamanaIndex::build()?

rfsaliev · 2026-01-07T12:30:00Z

bindings/cpp/src/dynamic_vamana_index.cpp

 ) noexcept {
    using Impl = DynamicVamanaIndexLeanVecImpl;
    *index = nullptr;
    return runtime_error_wrapper([&] {


Same as above.

rfsaliev · 2026-01-07T12:32:07Z

bindings/cpp/src/svs_runtime_utils.h

+    template <svs::threads::ThreadPool Pool>
+    static StorageType init(
+        const svs::data::ConstSimpleDataView<float>& data,
+        Pool& pool,
+        svs::lib::PowerOfTwo blocksize_bytes
+    ) {
+        auto parameters = svs::data::BlockingParameters{.blocksize_bytes = blocksize_bytes};
+        typename StorageType::allocator_type alloc(parameters);
+        StorageType result(data.size(), data.dimensions(), alloc);
+        svs::threads::parallel_for(
+            pool,
+            svs::threads::StaticPartition(result.size()),
+            [&](auto is, auto SVS_UNUSED(tid)) {
+                for (auto i : is) {
+                    result.set_datum(i, data.get_datum(i));
+                }
+            }
+        );
+        return result;
+    }


IMHO, would be better to modify the existing init() method rather than add an overload.
Is there any call to StorageFactory<SimpleDatasetType<ElementType>>::init() without blocksize_bytes argument?

ahuber21 · 2026-01-07T14:32:12Z

@razdoburdin could you give an example how this is used in a comment or the PR description?

init

b939a5b

razdoburdin requested review from ahuber21, ethanglaser and ibhati as code owners December 16, 2025 09:26

Merge branch 'main' into flex_blocksize

dce3bc8

rfsaliev requested changes Dec 16, 2025

View reviewed changes

move blocksize from add() to build()

52d4352

rfsaliev reviewed Jan 7, 2026

View reviewed changes

Make index blocksize flexible #248

Are you sure you want to change the base?

Make index blocksize flexible #248

Uh oh!

Conversation

razdoburdin commented Dec 16, 2025

Uh oh!

rfsaliev commented Dec 16, 2025

Uh oh!

rfsaliev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razdoburdin commented Dec 18, 2025

Uh oh!

rfsaliev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahuber21 commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants