Move memory allocation out of elkan_L2_sse #280

bjzhjing · 2023-12-08T04:02:13Z

elkan_L2_sse() does malloc/free each time when it's called by the loop in train_encoded() during data train for most of IVF index building. Memory allocation triggers the process of vma allocation, physical page allocation, page mapping setup and memory cgroup statistic update in kernel side, so more frequent allocation means more overhead. Move it out of the loop and do it only once when IndexFlatElkan is initialized to save CPU cycles.

issue #279

sre-ci-robot · 2023-12-08T04:02:23Z

Welcome @bjzhjing! It looks like this is your first PR to zilliztech/knowhere 🎉

mergify · 2023-12-08T04:02:49Z

@bjzhjing 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

liliu-z · 2023-12-08T09:18:43Z

Thanks for this PR, we will take a look ASAP.

liliu-z · 2023-12-08T09:18:47Z

/assign @alexanderguzhva
Hi Alex, do you mind to take a check? Thanks!

liliu-z · 2023-12-08T09:19:05Z

/kind enhancement

alexanderguzhva · 2023-12-08T12:20:16Z

thirdparty/faiss/faiss/utils/distances.h

@@ -422,6 +422,7 @@ void compute_PQ_dis_tables_dsub2(
 * @param y    database vectors, size ny * d
 * @param ids  result array ids
 * @param val  result array value
+ * @param data the pointer of memory for symmetric matrix data


Please call this variable tmp_buffer in order to stress out its meaning. It is not a data, it is a temporary storage.

Thanks for helping review, Alex! The change is done.

alexanderguzhva · 2023-12-08T12:21:47Z

thirdparty/faiss/faiss/IndexFlatElkan.cpp


 namespace faiss {

-IndexFlatElkan::IndexFlatElkan(idx_t d, MetricType metric, bool is_cosine, bool use_elkan)
+IndexFlatElkan::IndexFlatElkan(idx_t d, MetricType metric, bool is_cosine, bool use_elkan, float* data)


?! data which is passed as a parameters is not used anywhere

Yeah, it's not required to be set by callers. Remove it from the interface.

alexanderguzhva · 2023-12-08T12:23:28Z

thirdparty/faiss/faiss/IndexFlatElkan.h

@@ -29,9 +29,17 @@ namespace faiss {
 // Elkan algo was introduced into Knowhere in #2178, #2180 and #2258. 
 struct IndexFlatElkan : IndexFlat {
    bool use_elkan = true;
+    float* data = nullptr;


std::vector<float> tmp_buffer_for_elkan;

Changed it.

thirdparty/faiss/faiss/IndexFlatElkan.h

alexanderguzhva · 2023-12-08T12:27:55Z

thirdparty/faiss/faiss/utils/distances.cpp

    if (nx == 0 || ny == 0) {
        return;
    }

    const size_t bs_y = 1024;
-    float* data = (float*)malloc((bs_y * (bs_y - 1) / 2) * sizeof(float));
+    bool allocate = false;


Please use std::unique_ptr<float[]> + new for allocating a local temporary buffer

Call std::make_unique<float[]>() to allocate memory for tmp_buffer in IndexFlatElkan constructor. Remove the checking from elkan_L2_sse() for use_elkan can help ensure the memory is allocated when come there.

bjzhjing · 2023-12-13T12:29:34Z

@alexanderguzhva Hi Alex, I use smart pointer instead of std::vector to implement the tmp_buffer finally, for I IndexFlatElkan::search is defined with 'const', which means it's not allowed to modify the class variables. Thus, it's required to use const_cast<std::vector&> for convert while passing tmp_buffer to elkan_L2_sse, there is address change, which make things a bit complex. Smart pointer can help avoid the issue and make the code more clean and safe.

Please help review. Thanks very much!

alexanderguzhva · 2023-12-18T18:49:14Z

@bjzhjing Two more notes

Please add a comment about the situation to the top of IndexFlatElkan . Basically, in the provided form IndexFlatElkan is no longer thread-safe for the search. So, two threads cannot use 'IndexFlatElkan::search()' simultaneosly on the same instance. This is fine, bcz IndexFlatElkan is used only during the index construction. But the note about the implementation detail and why this was done should be included.
The needed size of the temporary buffer depends on the const size_t bs_y = 1024; magic constant inside elkan_L2_sse(). So, notes on the sizes need to be added to the description of the elkan_L2_sse() implementation. Maybe, bs_y size needs to be exposed as a function parameter as well.

elkan_L2_sse() does malloc/free each time when it's called by the loop in train_encoded() during data train for most of IVF index building. Memory allocation triggers the process of vma allocation, physical page allocation, page mapping setup and memory cgroup statistic update in kernel side, so more frequent allocation means more overhead. Move it out of the loop and do it only once when IndexFlatElkan is initialized to save CPU cycles. Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

For the memory allocation is moved to IndexFlatElkan construction, replace bs_y with the parameter defined and passed by Index. Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

bjzhjing · 2023-12-25T09:18:36Z

@bjzhjing Two more notes

Please add a comment about the situation to the top of IndexFlatElkan . Basically, in the provided form IndexFlatElkan is no longer thread-safe for the search. So, two threads cannot use 'IndexFlatElkan::search()' simultaneosly on the same instance. This is fine, bcz IndexFlatElkan is used only during the index construction. But the note about the implementation detail and why this was done should be included.

The needed size of the temporary buffer depends on the const size_t bs_y = 1024; magic constant inside elkan_L2_sse(). So, notes on the sizes need to be added to the description of the elkan_L2_sse() implementation. Maybe, bs_y size needs to be exposed as a function parameter as well.

Greetings @alexanderguzhva! I'm just back from a bad cold, and sorry for the late response. I've addressed your above comments. Please help review, thanks!

PwzXxm · 2023-12-27T02:59:01Z

/lgtm
/approve

sre-ci-robot · 2023-12-27T02:59:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bjzhjing, PwzXxm

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [PwzXxm]

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

sre-ci-robot requested review from cydrain and zhengbuqian December 8, 2023 04:02

sre-ci-robot added the size/M label Dec 8, 2023

mergify bot added dco-passed do-not-merge/missing-related-issue labels Dec 8, 2023

sre-ci-robot assigned alexanderguzhva Dec 8, 2023

sre-ci-robot added the kind/enhancement label Dec 8, 2023

mergify bot added ci-passed and removed do-not-merge/missing-related-issue labels Dec 8, 2023

alexanderguzhva requested changes Dec 8, 2023

View reviewed changes

bjzhjing force-pushed the reduce-elkan-mem-alloc branch from 1eba4fd to 814a0fd Compare December 9, 2023 08:33

sre-ci-robot added size/S and removed size/M labels Dec 9, 2023

mergify bot added ci-passed and removed ci-passed labels Dec 9, 2023

bjzhjing requested a review from alexanderguzhva December 12, 2023 00:07

bjzhjing force-pushed the reduce-elkan-mem-alloc branch from 814a0fd to 294a96c Compare December 13, 2023 10:36

mergify bot removed the ci-passed label Dec 13, 2023

mergify bot added the ci-passed label Dec 14, 2023

bjzhjing added 2 commits December 25, 2023 01:12

Expose bs_y as a parameter of elkan_L2_sse

3d2246b

For the memory allocation is moved to IndexFlatElkan construction, replace bs_y with the parameter defined and passed by Index. Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>

bjzhjing force-pushed the reduce-elkan-mem-alloc branch from 294a96c to 3d2246b Compare December 25, 2023 09:14

sre-ci-robot removed the size/S label Dec 25, 2023

sre-ci-robot added the size/M label Dec 25, 2023

mergify bot removed the ci-passed label Dec 25, 2023

sre-ci-robot assigned PwzXxm Dec 27, 2023

sre-ci-robot added the lgtm label Dec 27, 2023

sre-ci-robot added the approved label Dec 27, 2023

mergify bot added the ci-passed label Dec 27, 2023

sre-ci-robot merged commit 2a7848a into zilliztech:main Dec 27, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move memory allocation out of elkan_L2_sse #280

Move memory allocation out of elkan_L2_sse #280

bjzhjing commented Dec 8, 2023 •

edited

Loading

sre-ci-robot commented Dec 8, 2023

mergify bot commented Dec 8, 2023

liliu-z commented Dec 8, 2023

liliu-z commented Dec 8, 2023

liliu-z commented Dec 8, 2023

alexanderguzhva Dec 8, 2023

bjzhjing Dec 9, 2023

alexanderguzhva Dec 8, 2023

bjzhjing Dec 9, 2023

alexanderguzhva Dec 8, 2023

bjzhjing Dec 9, 2023

alexanderguzhva Dec 8, 2023

bjzhjing Dec 9, 2023 •

edited

Loading

bjzhjing commented Dec 13, 2023

alexanderguzhva commented Dec 18, 2023

bjzhjing commented Dec 25, 2023

PwzXxm commented Dec 27, 2023

sre-ci-robot commented Dec 27, 2023

Move memory allocation out of elkan_L2_sse #280

Move memory allocation out of elkan_L2_sse #280

Conversation

bjzhjing commented Dec 8, 2023 • edited Loading

sre-ci-robot commented Dec 8, 2023

mergify bot commented Dec 8, 2023

liliu-z commented Dec 8, 2023

liliu-z commented Dec 8, 2023

liliu-z commented Dec 8, 2023

alexanderguzhva Dec 8, 2023

Choose a reason for hiding this comment

bjzhjing Dec 9, 2023

Choose a reason for hiding this comment

alexanderguzhva Dec 8, 2023

Choose a reason for hiding this comment

bjzhjing Dec 9, 2023

Choose a reason for hiding this comment

alexanderguzhva Dec 8, 2023

Choose a reason for hiding this comment

bjzhjing Dec 9, 2023

Choose a reason for hiding this comment

alexanderguzhva Dec 8, 2023

Choose a reason for hiding this comment

bjzhjing Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

bjzhjing commented Dec 13, 2023

alexanderguzhva commented Dec 18, 2023

bjzhjing commented Dec 25, 2023

PwzXxm commented Dec 27, 2023

sre-ci-robot commented Dec 27, 2023

bjzhjing commented Dec 8, 2023 •

edited

Loading

bjzhjing Dec 9, 2023 •

edited

Loading