support concurrent read and write for HNSW #178

inabao · 2024-12-02T11:21:12Z

No description provided.

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

src/index/hnsw.cpp

tests/test_multi_thread.cpp

src/algorithm/hnswlib/visited_list_pool.h

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

inabao · 2024-12-08T15:25:33Z

tests/test_multi_thread.cpp

@@ -211,7 +211,6 @@ TEST_CASE("HNSW Multi-threading", "[ft][hnsw]") {
                ->Float32Vectors(data.get() + i * dim)
                ->Owner(false);
            auto add_res = index->Add(dataset);
-            REQUIRE(add_res.has_value());


REQUIRE is not thread-safe. If add_res.has_value() is false, then add_res.value().size() will cause a core dump.

inabao · 2024-12-08T15:26:25Z

tests/test_multi_thread.cpp

@@ -241,7 +240,7 @@ TEST_CASE("HNSW Multi-threading", "[ft][hnsw]") {

    float recall = correct / max_elements;
    std::cout << index->GetStats() << std::endl;
-    REQUIRE(recall == 1);
+    REQUIRE(recall > 0.96);


Previously, it was not built concurrently, so the recall rate could be 1. However, a concurrent build cannot achieve such a high recall rate.

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

src/algorithm/hnswlib/hnswalg.cpp

jiaweizone · 2024-12-15T10:33:13Z

src/algorithm/hnswlib/hnswalg.cpp

+                                    int level,
+                                    const InnerIdType* neighbors,
+                                    size_t neigbor_count) {
+    linklistsizeint* ll_cur = getLinklistAtLevel(internal_id, level);


ditto, getLinklistAtLevel is not thread-safe

This part is read-only, and subsequent modifications will be placed under the protection of the lock

What happens when one thread reads while another thread modifies ?

I recommend you to complete the revision directly, otherwise the reviewer cannot make a judgment on thread safety

I have added the lock

jiaweizone · 2024-12-15T10:33:47Z

src/algorithm/hnswlib/hnswalg.cpp

+                                 int level,
+                                 InnerIdType neighbor,
+                                 size_t max_degree) {
+    linklistsizeint* ll_cur = getLinklistAtLevel(internal_id, level);


src/algorithm/hnswlib/hnswalg.cpp

jiaweizone · 2024-12-15T15:30:06Z

src/algorithm/hnswlib/hnswalg.cpp

-            continue;
+        for (size_t i = 0; i < cand_neighbors.size(); i++) {
+            auto id = cand_neighbors[i];
+            auto& in_edges = getEdges(id, level);


Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

jiaweizone · 2024-12-25T11:30:27Z

src/algorithm/hnswlib/hnswalg.cpp

+                                    const InnerIdType* neighbors,
+                                    size_t neigbor_count) {
+    std::unique_lock lock(link_list_locks_[internal_id]);
+    linklistsizeint* ll_cur = getLinklistAtLevel(internal_id, level);


getLinklistAtLevel is thread-safe ?
the sub lock link_list_locks_[internal_id] can not protect the data_level0_memory_ concurrent access
eg. data_level0_memory_->Resize()

The function getLinklistAtLevel does not access data_level0_memory_; it accesses the graph, while data_level0_memory_ stores the raw data.

jiaweizone · 2024-12-25T12:22:33Z

src/algorithm/hnswlib/hnswalg.cpp

+                                 InnerIdType neighbor,
+                                 size_t max_degree) {
+    std::unique_lock lock(link_list_locks_[internal_id]);
+    linklistsizeint* ll_cur = getLinklistAtLevel(internal_id, level);


…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

Signed-off-by: jinjiabao.jjb <[email protected]>

…d-and-write

codecov · 2024-12-31T07:01:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

see 39 files with indirect coverage changes

LHT129

LGTM

jiaweizone

LGTM

src/algorithm/hnswlib/hnswalg.cpp

wxyucs · 2025-01-02T03:16:44Z

src/algorithm/hnswlib/hnswalg.h

+    // Locks operations with element by label value
+    mutable std::shared_mutex resize_mutex_{};
+
+    mutable std::shared_mutex global_{};


global_ could make confused, rename to level_mutex_ or multi_level_mutex_

wxyucs · 2025-01-02T03:17:20Z

src/algorithm/hnswlib/hnswalg.h

+    mutable std::shared_mutex label_lookup_lock_{};  // lock for label_lookup_
    vsag::UnorderedMap<LabelType, InnerIdType> label_lookup_;


move these two line up, near other locks

wxyucs · 2025-01-02T03:17:42Z

src/algorithm/hnswlib/hnswalg.h

+    mutable std::shared_mutex resize_mutex_{};
+
+    mutable std::shared_mutex global_{};


add more comments about the scope of these locks

Signed-off-by: jinjiabao.jjb <[email protected]>

wxyucs

lgtm

Signed-off-by: jinjiabao.jjb <[email protected]>

jinjiabao.jjb added 3 commits December 2, 2024 15:49

modify

6c85257

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

73f8b2e

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

62a20d0

Signed-off-by: jinjiabao.jjb <[email protected]>

inabao requested review from jiaweizone and wxyucs as code owners December 2, 2024 11:21

pull-request-size bot added the size/L label Dec 2, 2024

inabao requested a review from LHT129 December 2, 2024 11:21

inabao self-assigned this Dec 2, 2024

inabao requested a review from ShawnShawnYou December 2, 2024 11:22

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

fe62e6e

…d-and-write

LHT129 requested changes Dec 6, 2024

View reviewed changes

src/index/hnsw.cpp Show resolved Hide resolved

tests/test_multi_thread.cpp Outdated Show resolved Hide resolved

src/algorithm/hnswlib/visited_list_pool.h Outdated Show resolved Hide resolved

jinjiabao.jjb added 6 commits December 7, 2024 13:46

modify

7b49ae0

Signed-off-by: jinjiabao.jjb <[email protected]>

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

f52018d

…d-and-write

modify

f05c0e3

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

b2103b5

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

45d5ac4

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

36231d1

Signed-off-by: jinjiabao.jjb <[email protected]>

inabao commented Dec 8, 2024

View reviewed changes

jinjiabao.jjb added 2 commits December 9, 2024 00:16

modify

5f4df3c

Signed-off-by: jinjiabao.jjb <[email protected]>

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

9e7a7d5

…d-and-write

jiaweizone reviewed Dec 15, 2024

View reviewed changes

modify

1ed17fc

Signed-off-by: jinjiabao.jjb <[email protected]>

inabao added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Dec 16, 2024

jinjiabao.jjb added 3 commits December 17, 2024 17:34

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

67c0658

…d-and-write

modify

95442d0

Signed-off-by: jinjiabao.jjb <[email protected]>

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

7837a65

…d-and-write

ShawnShawnYou approved these changes Dec 20, 2024

View reviewed changes

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

9ea928b

…d-and-write

jinjiabao.jjb added 2 commits December 22, 2024 21:16

modify

d91f96d

Signed-off-by: jinjiabao.jjb <[email protected]>

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

b92bddc

…d-and-write

wxyucs added the version/0.13 label Dec 24, 2024

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

140383a

…d-and-write

wxyucs added version/0.12 and removed version/0.13 labels Dec 24, 2024

jinjiabao.jjb added 2 commits December 24, 2024 18:46

modify

4883642

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

8dec512

Signed-off-by: jinjiabao.jjb <[email protected]>

jiaweizone reviewed Dec 25, 2024

View reviewed changes

jinjiabao.jjb added 2 commits December 26, 2024 15:10

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

ea7cdb2

…d-and-write

modify

8da3d2d

Signed-off-by: jinjiabao.jjb <[email protected]>

wxyucs added version/0.13 and removed version/0.12 labels Dec 26, 2024

jinjiabao.jjb added 4 commits December 31, 2024 11:07

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

4848f5f

…d-and-write

modify

390e9df

Signed-off-by: jinjiabao.jjb <[email protected]>

modify

c9c8b1c

Signed-off-by: jinjiabao.jjb <[email protected]>

Merge remote-tracking branch 'origin' into enable-concurrency-for-rea…

df342ff

…d-and-write

LHT129 approved these changes Dec 31, 2024

View reviewed changes

jiaweizone approved these changes Jan 1, 2025

View reviewed changes

src/algorithm/hnswlib/hnswalg.cpp Show resolved Hide resolved

wxyucs requested changes Jan 2, 2025

View reviewed changes

modify

ef254ae

Signed-off-by: jinjiabao.jjb <[email protected]>

wxyucs approved these changes Jan 2, 2025

View reviewed changes

modify

a3e15e0

Signed-off-by: jinjiabao.jjb <[email protected]>

inabao merged commit b59cec9 into main Jan 3, 2025
10 checks passed

inabao deleted the enable-concurrency-for-read-and-write branch January 3, 2025 08:43

inabao mentioned this pull request Jan 3, 2025

support concurrent insert/search in HNSW #274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support concurrent read and write for HNSW #178

support concurrent read and write for HNSW #178

inabao commented Dec 2, 2024

inabao Dec 8, 2024

inabao Dec 8, 2024

jiaweizone Dec 15, 2024

inabao Dec 16, 2024

jiaweizone Dec 17, 2024 •

edited

Loading

inabao Dec 24, 2024

jiaweizone Dec 15, 2024

inabao Dec 16, 2024

jiaweizone Dec 15, 2024

inabao Dec 16, 2024

jiaweizone Dec 25, 2024

inabao Dec 25, 2024

jiaweizone Dec 25, 2024

inabao Dec 25, 2024

codecov bot commented Dec 31, 2024 •

edited

Loading

LHT129 left a comment

jiaweizone left a comment

wxyucs Jan 2, 2025

inabao Jan 2, 2025

wxyucs Jan 2, 2025

inabao Jan 2, 2025

wxyucs Jan 2, 2025

inabao Jan 2, 2025

wxyucs left a comment

		mutable std::shared_mutex label_lookup_lock_{}; // lock for label_lookup_
		vsag::UnorderedMap<LabelType, InnerIdType> label_lookup_;

		mutable std::shared_mutex resize_mutex_{};

		mutable std::shared_mutex global_{};

support concurrent read and write for HNSW #178

support concurrent read and write for HNSW #178

Conversation

inabao commented Dec 2, 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

jiaweizone Dec 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

codecov bot commented Dec 31, 2024 • edited Loading

Codecov Report

LHT129 left a comment

Choose a reason for hiding this comment

jiaweizone left a 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

wxyucs left a comment

Choose a reason for hiding this comment

jiaweizone Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 31, 2024 •

edited

Loading