Use search_pool to control iterator->Next() #1008

alwayslove2013 · 2024-12-26T04:04:31Z

issue: #997

The current AnnIterator function utilizes the search pool for concurrency control only when initializing the Iterator. Once the returned iterator is handed over to the upper layer, the ->next() calls are not subjected to any thread restrictions, which may lead to issues such as OMP conflicts.

To address this, we propose the following considerations:

All iterators will accept a use_knowhere_search_pool parameter during construction.

When set to True (the default), the iterator->next() will be scheduled by the knowhere_search_thread_pool.

When set to False, iterator->next() will not involve thread scheduling internally, so please take caution.

The initialization of iterator in the AnnIterator function will no longer be concurrent, which helps to avoid potential deadlocks.

Furthermore, to enhance performance for large_nq, we will try to streamline the initialization process for all iterators by deferring heavier pre-computation tasks to the first call of ->next().

sre-ci-robot · 2024-12-26T04:04:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

mergify · 2024-12-26T04:05:09Z

@alwayslove2013 🔍 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!.

alexanderguzhva · 2024-12-26T13:21:31Z

include/knowhere/index/index_node.h

@@ -202,7 +203,7 @@ class IndexNode : public Object {
            return GenResultDataSet(nq, std::move(range_search_result));
        }

-        auto its_or = AnnIterator(dataset, std::move(cfg), bitset);
+        auto its_or = AnnIterator(dataset, std::move(cfg), bitset, false);


Please add a comment about why this parameter is false

added.

The range_search function has utilized the search_pool to concurrently handle various queries.
To prevent potential deadlocks, the iterator for a single query no longer requires additional thread control over the next() call.

alexanderguzhva · 2024-12-26T13:24:14Z

include/knowhere/index/index_node.h

+// If use_knowhere_search_pool is True (the default), the iterator->Next() will be scheduled by the
+//   knowhere_search_thread_pool.
+//   If False, will Not involve thread scheduling internally, so please take caution.
+template <bool use_knowhere_search_pool = true>


What are the justifications for adding use_knowhere_search_pool as a template parameter instead of making a regular IndexIterator::use_knowhere_search_pool field? I'm not sure I follow

Just to reduce the judgment of use_knowhere_search_pool in each iterator->next() call.

I've re-read the code again and I don't believe I see a reason for making use_knowhere_search_pool a template parameter. Basically, it leads to increase of the size of the compiled code, also this parameter is not generic enough (like T in std::vector<T>) and it does not have a critical-performance impact. Please consider demoting use_knowhere_search_pool to a variable. Or I can accept the code as is.

Thanks for your suggestion~ I will use use_knowhere_search_pool as an iterator variable rather than a template param.

alexanderguzhva · 2024-12-26T13:29:11Z

src/common/comp/brute_force.cc

-                    distances_id.id = distances_id.id == -1 ? -1 : distances_id.id + xb_id_offset;
+                if (xb_id_offset != 0) {
+                    for (auto& distances_id : distances_ids) {
+                        distances_id.id = distances_id.id == -1 ? -1 : distances_id.id + xb_id_offset;


just to confirm: this is evaluated as distances_id.id = a ? b : c, not `distances_id.id = (a ? b : c) + xb_id_offset'?

@cqy123456 help check~

yes, -1 or distances_id.id + xb_id_offset

codecov · 2024-12-28T06:10:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.96%. Comparing base (3c46f4c) to head (cf9d25d).
Report is 280 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #1008       +/-   ##
=========================================
+ Coverage      0   73.96%   +73.96%     
=========================================
  Files         0       82       +82     
  Lines         0     6972     +6972     
=========================================
+ Hits          0     5157     +5157     
- Misses        0     1815     +1815

see 82 files with indirect coverage changes

PwzXxm · 2024-12-30T07:46:57Z

src/common/comp/brute_force.cc

+    try {
+        for (int i = 0; i < nq; ++i) {
+            auto compute_dist_func = [=]() -> std::vector<DistId> {
+                auto xb = base_dataset->GetTensor();


maybe add comments on why we cannot do it outside

alexanderguzhva · 2024-12-30T17:06:16Z

also, please squash the commits into a single one. Thanks.

Signed-off-by: min.tian <[email protected]>

liliu-z · 2024-12-31T05:14:19Z

include/knowhere/index/index_node.h

-                futs.emplace_back(
-                    ThreadPool::GetGlobalSearchThreadPool()->push([&, idx = i]() { task_with_ordered_iterator(idx); }));
+                futs.emplace_back(ThreadPool::GetGlobalSearchThreadPool()->push([&, idx = i]() {
+                    ThreadPool::ScopedSearchOmpSetter setter(1);


Can we rewrite the push to include this omp setter instead of calling it everywhere

@cqy123456 Is this feasible? Are there any scenarios where we do not need the OmpSetter?

liliu-z · 2024-12-31T05:15:34Z

include/knowhere/index/index_node.h

          sign_(larger_is_closer ? -1 : 1) {
    }

    std::pair<int64_t, float>
    Next() override {
        if (!initialized_) {
-            throw std::runtime_error("Next should not be called before initialization");
+            initialize();


Curios why we need this

Throwing error is a bit too strict?

liliu-z · 2024-12-31T05:31:28Z

include/knowhere/index/index_node.h

-        // dims out of 30000 total dims) sharing at least 1 common non-zero dimension.
-        results_.reserve(distances.size() * 0.3);
-        for (size_t i = 0; i < distances.size(); i++) {
-            if (distances[i] != 0) {


Why we do this? I think this PrecomputedDistanceIterator is used for both dense/sparse BF
@zhengbuqian

submitted a new commit to reduce misunderstanding.

Signed-off-by: min.tian <[email protected]>

sre-ci-robot requested review from chasingegg and liliu-z December 26, 2024 04:04

sre-ci-robot added the size/XL label Dec 26, 2024

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

alwayslove2013 force-pushed the iterator_next_with_search_pool branch 3 times, most recently from ee16112 to 3c70c4b Compare December 26, 2024 07:05

alexanderguzhva reviewed Dec 26, 2024

View reviewed changes

alwayslove2013 force-pushed the iterator_next_with_search_pool branch 2 times, most recently from b331267 to 98048fc Compare December 28, 2024 04:54

mergify bot added the ci-passed label Dec 28, 2024

alwayslove2013 requested a review from alexanderguzhva December 30, 2024 01:57

PwzXxm reviewed Dec 30, 2024

View reviewed changes

mergify bot added ci-passed and removed ci-passed labels Dec 30, 2024

alwayslove2013 force-pushed the iterator_next_with_search_pool branch from 7a5a27d to 6ce1b7f Compare December 31, 2024 02:53

mergify bot removed the ci-passed label Dec 31, 2024

Use search_pool to control iterator->Next()

92f234b

Signed-off-by: min.tian <[email protected]>

alwayslove2013 force-pushed the iterator_next_with_search_pool branch from 6ce1b7f to 92f234b Compare December 31, 2024 02:56

mergify bot added the ci-passed label Dec 31, 2024

liliu-z reviewed Dec 31, 2024

View reviewed changes

mergify bot removed the ci-passed label Dec 31, 2024

Not use template in PrecomputedDistanceIterator

cf9d25d

Signed-off-by: min.tian <[email protected]>

alwayslove2013 force-pushed the iterator_next_with_search_pool branch from 465f005 to cf9d25d Compare December 31, 2024 08:06

mergify bot added the ci-passed label Dec 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use search_pool to control iterator->Next() #1008

Use search_pool to control iterator->Next() #1008

alwayslove2013 commented Dec 26, 2024 •

edited

Loading

sre-ci-robot commented Dec 26, 2024

mergify bot commented Dec 26, 2024

alexanderguzhva Dec 26, 2024

alwayslove2013 Dec 27, 2024

alexanderguzhva Dec 26, 2024

alwayslove2013 Dec 27, 2024

alexanderguzhva Dec 30, 2024

alwayslove2013 Dec 31, 2024

alexanderguzhva Dec 26, 2024

alwayslove2013 Dec 27, 2024

PwzXxm Dec 30, 2024

codecov bot commented Dec 28, 2024 •

edited

Loading

PwzXxm Dec 30, 2024

alwayslove2013 Dec 30, 2024

alexanderguzhva commented Dec 30, 2024

liliu-z Dec 31, 2024

alwayslove2013 Dec 31, 2024

liliu-z Dec 31, 2024

alwayslove2013 Dec 31, 2024

liliu-z Dec 31, 2024

alwayslove2013 Dec 31, 2024

Use search_pool to control iterator->Next() #1008

Are you sure you want to change the base?

Use search_pool to control iterator->Next() #1008

Conversation

alwayslove2013 commented Dec 26, 2024 • edited Loading

sre-ci-robot commented Dec 26, 2024

mergify bot commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 28, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderguzhva commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwayslove2013 commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading