Aisaq improvments by liorf95 · Pull Request #1637 · zilliztech/knowhere

liorf95 · 2026-05-20T09:24:27Z

Related to #1611

sre-ci-robot · 2026-05-20T09:24:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liorf95
To complete the pull request process, please assign liliu-z after the PR has been reviewed.
You can assign the PR to them by writing /assign @liliu-z in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1. Shorten build time for AiSAQ in scale configuration • In this version build process will no longer rearrange the index, it will only generate the rearranged data, this data shall be used during search for reading the PQ vectors in a rearranged manner. • Use multiple threads when generating the rearranged data during build. 2. Inline PQ optimization - Utilize sector unused space for additional inline PQ vectors when possible (this will not increase the index footprint on the disk). 3. When rearranged is enabled, instead of saving both aligned rearranged PQ vectors file and original PQ vectors file, only the first one is saved. 4. Set AiSAQ default beamwidth value from 8 to 2 to avoid excessive disk load. 5. Improved RAM estimation for index build with GPU 6. Fixed consumed memory size calculation function. 7. Fixed AiSAQ thread context leak when index 'size' API is called and all PQ vectors are stored inline. 8. Fixed context release to be done before notifying that it is free. 9. Fixed index file existence check. 10. Fixed index build with COSINE metric 11. Fixed context resource release order to avoid race condition. Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Hiroshi Murayama <hiroshi4.murayama@kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

mergify · 2026-05-20T10:09:52Z

@liorf95 e2e jenkins job failed, comment /run-e2e can trigger the job again.

alexanderguzhva

btw, are there any changes on the backward compatibility?

alexanderguzhva · 2026-05-20T13:26:51Z

        knowhere::KnowhereConfig::SetSimdType(knowhere::KnowhereConfig::SimdType::AVX2);
-        knowhere::KnowhereConfig::SetBuildThreadPoolSize(default_build_thread_num);
-        knowhere::KnowhereConfig::SetSearchThreadPoolSize(default_search_thread_num);
+        knowhere::KnowhereConfig::SetBuildThreadPoolSize(48);


please keep these numbers as is, and add environment overrides, if needed. This is related only to these two numbers of threads. We also run these benchmarks internally.
Alternatively, just please introduce a new benchmark .cpp file, which is more suitable for your needs.

alexanderguzhva · 2026-05-20T13:27:53Z

    SetUp() override {
        T0_ = elapsed();
-        set_ann_test_name("sift-128-euclidean");
+        const char* dataset = std::getenv("DATASET");


I'd use not DATASET, but BENCH_DATASET, in order to specialize environment variables and ensure that theirs names are related to the benchmark code. DATASET is a too generic name.

Same for others

alexanderguzhva · 2026-05-20T13:32:12Z

 constexpr size_t default_pool_size = default_max_nr / default_max_events;

+struct AioInfo {
+    long aio_nr;


please initialize fields with default values

alexanderguzhva · 2026-05-20T13:33:06Z

+	size_t available_aio = (size_t)(aio_info.aio_max_nr - aio_info.aio_nr);
+	if(num_ctx*max_events > available_aio) {
+		LOG_KNOWHERE_INFO_ << " max events requested is too high: " << max_events << " setting to: " << available_aio/num_ctx;
+		max_events= max_events_ = available_aio/num_ctx;


please use several a = b; instead

alexanderguzhva · 2026-05-20T13:35:17Z

      auto item = params.find(name);
      if (item == params.end()) {
-        throw std::invalid_argument("Invalid parameter name.");
+        throw std::invalid_argument("Invalid parameter name."+name);


throw std::invalid_argument("Invalid parameter name (" + name + ")");

alexanderguzhva · 2026-05-20T13:37:58Z

+                vamana_reader.read((char *)&nnbrs, sizeof(uint32_t));               
+
+                // sanity checks on nnbrs
+                assert(nnbrs > 0);


please confirm that these are assert() calls, not a C++ check-and-throw

Yes, these are asserts.

alexanderguzhva · 2026-05-20T13:40:37Z

                                inline_pq_vectors, max_node_len);
-
+
+    uint64_t *vamana_reader_node_to_pos_map = nullptr;


if this a major upgrade, then maybe it is time to upgrade these naked pointers and T* a = new b[]; calls into std::unique_ptr<T[]> a = std::make_unique<T[]>(b); C++ calls. Custom deallocators can also be passed (like std::unique_ptr<FILE, decltype(&fclose)>)

alexanderguzhva · 2026-05-20T13:44:00Z

 #include <stdio.h>
 #include <memory>

+bool Avx2SupportedCPU = true;


are these two used anywhere?

Apparently not.

sre-ci-robot requested review from alexanderguzhva and foxspy May 20, 2026 09:24

sre-ci-robot added the size/XXL label May 20, 2026

mergify Bot added the needs-dco label May 20, 2026

liorf95 force-pushed the aisaq_improvments branch from 486d2dd to fb3c3b2 Compare May 20, 2026 09:40

liorf95 added 5 commits May 20, 2026 09:43

After pre-commit fixes.

4a564a1

Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Hiroshi Murayama <hiroshi4.murayama@kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

After clang-tidy fix.

396d2c8

Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Hiroshi Murayama <hiroshi4.murayama@kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

After clang-tidy fix.

09d763d

Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Hiroshi Murayama <hiroshi4.murayama@kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

Rollback CMAKE_CUDA_ARCHITECTURES to be RAPIDS.

f46ea53

Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com> Signed-off-by: Gili Buzaglo <Gili.Buzaglo@il.kioxia.com> Signed-off-by: Hiroshi Murayama <hiroshi4.murayama@kioxia.com> Signed-off-by: Lior Friedman <lior.friedman@il.kioxia.com>

liorf95 force-pushed the aisaq_improvments branch from fb3c3b2 to f46ea53 Compare May 20, 2026 09:44

mergify Bot added dco-passed and removed needs-dco labels May 20, 2026

alexanderguzhva reviewed May 20, 2026

View reviewed changes

		inline_pq_vectors, max_node_len);


		uint64_t *vamana_reader_node_to_pos_map = nullptr;

Conversation

liorf95 commented May 20, 2026

Uh oh!

sre-ci-robot commented May 20, 2026

Uh oh!

mergify Bot commented May 20, 2026

Uh oh!

alexanderguzhva 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants