introduce simple resource_pool #123

LHT129 · 2024-11-11T03:35:55Z

for visited_list, give an implement of visited_list_pool
we will have some other object like aio_context...
currently hgraph use visitlist from hnswlib, now make it global

inabao · 2024-11-16T07:59:36Z

src/utils/visited_list.h

+    explicit VisitedList(InnerIdType max_size, Allocator* allocator)
+        : max_size_(max_size), allocator_(allocator) {
+        this->list_ = reinterpret_cast<VisitedListType*>(
+            allocator_->Allocate((uint64_t)max_size * sizeof(VisitedListType)));


memory allocation may fail

we have safe allocator

inabao · 2024-11-16T08:01:16Z

src/utils/resource_object_pool.h

+
+public:
+    template <typename... Args>
+    explicit ResourceObjectPool(uint64_t init_size, Args... args) {


Q: Do we need to set a maximum size for the pool to prevent creating too many resources in high concurrency scenarios?

there are to strategy to manage the size, fixed or flexible, currently is flexible. maybe add fixed soon, if necessary

src/utils/resource_object_pool.h

jiaweizone · 2024-11-20T16:22:22Z

src/utils/resource_object_pool.h

+    }
+
+    std::shared_ptr<T>
+    GetOne() {


Here is a Take / Return semantics ?

jiaweizone · 2024-11-20T16:22:35Z

src/utils/resource_object_pool.h

+    }
+
+    void
+    ReleaseOne(std::shared_ptr<T>& obj) {


jiaweizone · 2024-11-20T16:24:25Z

src/utils/resource_object_pool.h

+    }
+
+    std::vector<std::shared_ptr<T>> pool_{};
+    size_t pool_size_{0};


pool_size_ is not necessary, can replace with pool_.size() ?

sometimes may not equal

Can you tell me more about it ?
for thread safe need use std::atomic<size_t>

src/utils/visited_list.h

codecov · 2025-01-15T03:16:35Z

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.

@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
- Coverage   90.24%   90.21%   -0.04%     
==========================================
  Files         118      121       +3     
  Lines        7372     7427      +55     
==========================================
+ Hits         6653     6700      +47     
- Misses        719      727       +8

Flag	Coverage Δ
cpp	`90.21% <92.72%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
common	`86.55% <ø> (ø)`
datacell	`90.10% <ø> (ø)`
index	`90.60% <ø> (-0.12%)`	⬇️
simd	`100.00% <ø> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f368e7...1ea9076. Read the comment docs.

src/utils/resource_object.h

src/utils/resource_object_pool.h

src/utils/visited_list.h

src/utils/visited_list_test.cpp

wxyucs

lgtm

inabao · 2025-01-17T07:04:08Z

src/utils/resource_object_pool.h

+        }
+    }
+
+    std::vector<std::shared_ptr<T>> pool_{};


- for visited_list, give an implement of visited_list_pool - we will have some other object like aio_context... - currently hgraph use visitlist from hnswlib, now make it global Signed-off-by: LHT129 <[email protected]>

jiaweizone · 2025-01-18T14:51:36Z

src/utils/resource_object_pool.h

+    TakeOne() {
+        std::lock_guard<std::mutex> lock(mutex_);
+        if (pool_.empty()) {
+            return this->constructor_();


minor: here can release the lock mutex_ and then construct the object

jiaweizone · 2025-01-18T15:02:35Z

src/utils/resource_object_pool.h

+    }
+
+    std::vector<std::shared_ptr<T>> pool_{};
+    size_t pool_size_{0};


Can you tell me more about it ?
for thread safe need use std::atomic<size_t>

jiaweizone · 2025-01-18T15:09:57Z

src/utils/resource_object_pool.h

+    }
+
+    void
+    SetConstructor(ConstructFuncType func) {


If there are objects that have already been taken, SetConstructor will have a behavior problem

LHT129 requested review from wxyucs, inabao and ShawnShawnYou November 11, 2024 03:35

LHT129 self-assigned this Nov 11, 2024

pull-request-size bot added the size/L label Nov 11, 2024

LHT129 added kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) and removed size/L labels Nov 11, 2024

LHT129 force-pushed the visit_table branch from a855909 to d2993eb Compare November 12, 2024 06:50

pull-request-size bot added the size/L label Nov 12, 2024

LHT129 marked this pull request as ready for review November 12, 2024 08:23

LHT129 requested a review from jiaweizone as a code owner November 12, 2024 08:23

LHT129 force-pushed the visit_table branch from d2993eb to f3589c3 Compare November 12, 2024 08:27

LHT129 changed the title ~~[WIP]introduce simple resource_pool~~ introduce simple resource_pool Nov 13, 2024

LHT129 mentioned this pull request Nov 13, 2024

add hgraph test & fix #125

Closed

LHT129 force-pushed the visit_table branch from f3589c3 to aec206f Compare November 14, 2024 09:01

inabao reviewed Nov 16, 2024

View reviewed changes

LHT129 force-pushed the visit_table branch 3 times, most recently from 18ce304 to b47a6c4 Compare November 20, 2024 07:23

jiaweizone reviewed Nov 20, 2024

View reviewed changes

LHT129 force-pushed the visit_table branch 3 times, most recently from d1fcb50 to 819012a Compare November 27, 2024 09:07

LHT129 force-pushed the visit_table branch 4 times, most recently from 518e82c to 80b1462 Compare December 5, 2024 05:02

LHT129 force-pushed the visit_table branch 3 times, most recently from b11eea9 to dbcc027 Compare December 9, 2024 07:30

LHT129 force-pushed the visit_table branch 5 times, most recently from 62110ce to 308d8c1 Compare December 17, 2024 09:37

LHT129 force-pushed the visit_table branch from 308d8c1 to 7d295d0 Compare December 20, 2024 06:17

wxyucs added the version/0.13 label Dec 24, 2024

LHT129 force-pushed the visit_table branch from 7d295d0 to f0035a9 Compare December 30, 2024 03:48

LHT129 force-pushed the visit_table branch from f0035a9 to ceee664 Compare January 15, 2025 02:41

LHT129 force-pushed the visit_table branch from ceee664 to 1be6db2 Compare January 16, 2025 03:47

wxyucs reviewed Jan 16, 2025

View reviewed changes

LHT129 force-pushed the visit_table branch 2 times, most recently from 535630d to 9fd743c Compare January 17, 2025 06:59

wxyucs approved these changes Jan 17, 2025

View reviewed changes

inabao reviewed Jan 17, 2025

View reviewed changes

src/utils/resource_object_pool.h Outdated

}

}

std::vector<std::shared_ptr<T>> pool_{};

Copy link

Collaborator

inabao Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Deque

LHT129 force-pushed the visit_table branch from 9fd743c to 843ea97 Compare January 18, 2025 06:32

introduce simple resource_pool

1ea9076

- for visited_list, give an implement of visited_list_pool - we will have some other object like aio_context... - currently hgraph use visitlist from hnswlib, now make it global Signed-off-by: LHT129 <[email protected]>

LHT129 force-pushed the visit_table branch from 843ea97 to 1ea9076 Compare January 18, 2025 06:36

jiaweizone reviewed Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce simple resource_pool #123

introduce simple resource_pool #123

LHT129 commented Nov 11, 2024

inabao Nov 16, 2024

LHT129 Jan 17, 2025

inabao Nov 16, 2024

LHT129 Nov 17, 2024

jiaweizone Nov 20, 2024

LHT129 Nov 25, 2024

jiaweizone Nov 20, 2024

LHT129 Nov 25, 2024

jiaweizone Nov 20, 2024

LHT129 Jan 17, 2025

jiaweizone Jan 18, 2025

codecov bot commented Jan 15, 2025 •

edited

Loading

wxyucs left a comment

inabao Jan 17, 2025

jiaweizone Jan 18, 2025

LHT129 Jan 19, 2025

jiaweizone Jan 18, 2025

jiaweizone Jan 18, 2025

introduce simple resource_pool #123

Are you sure you want to change the base?

introduce simple resource_pool #123

Conversation

LHT129 commented Nov 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

wxyucs 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

codecov bot commented Jan 15, 2025 •

edited

Loading