Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix memory leak for hnsw #137

Merged
merged 15 commits into from
Nov 24, 2024
Merged

fix memory leak for hnsw #137

merged 15 commits into from
Nov 24, 2024

Conversation

inabao
Copy link
Collaborator

@inabao inabao commented Nov 14, 2024

  • change the memory allocation of VisitListPool from new to allocate.

fixed: #70

jinjiabao.jjb added 2 commits November 14, 2024 11:54
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@inabao inabao requested review from wxyucs and LHT129 November 16, 2024 08:06
@inabao inabao self-assigned this Nov 16, 2024
@inabao inabao added the kind/bug Something isn't working label Nov 16, 2024
@@ -73,6 +71,11 @@ HierarchicalNSW::HierarchicalNSW(SpaceInterface* s,

void
HierarchicalNSW::reset() {
if (visited_list_pool_) {
visited_list_pool_->~VisitedListPool();
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we can overloading new/delete operator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -89,6 +92,12 @@ bool
HierarchicalNSW::init_memory_space() {
// release the memory allocated by the init_memory_space function that was called earlier
reset();
auto visited_list_mem = allocator_->Allocate(sizeof(VisitedListPool));
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 20, 2024
jinjiabao.jjb added 3 commits November 20, 2024 17:10
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@@ -73,6 +71,10 @@ HierarchicalNSW::HierarchicalNSW(SpaceInterface* s,

void
HierarchicalNSW::reset() {
if (visited_list_pool_) {
VisitedListPool::operator delete(visited_list_pool_, allocator_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete visitored_list_pool_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because an allocator is used, the default new and delete cannot be used.

Copy link
Collaborator

@jiaweizone jiaweizone Nov 21, 2024

Choose a reason for hiding this comment

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

Ok, maybe we can add New and Delete function to Allocator, like this:

class Allocator {
public:
    void* Allocate(size_t size) { ... }
    void Deallocate(void* p) { ... }

    template<typename T, typename ...Args>
    T* New(Args&&... args) {
        void* p = Allocate(sizeof(T));
        return (T*) ::new (p) T(std::forward<Args>(args)...);
    }

    template<typename T>
    void Delete(T* p) {
        p->~T();
        Deallocate(static_cast<void*>(p));
    }
}

// ....

void HierarchicalNSW::init_memory_space() {
    // ...
    visited_list_pool_ = allocator_->New<VisitedListPool>(max_elements_, allocator_);
}

void HierarchicalNSW::reset() {
    if (visited_list_pool_) {
        allocator_->Delete(visited_list_pool_);
    }
    // ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I have modified it.

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

void
operator delete(void* p, vsag::Allocator* alloc) noexcept {
Copy link
Collaborator

Choose a reason for hiding this comment

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

void operator delete(void* p) noexcept {
     alloc->Deallocate(p);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

jinjiabao.jjb added 3 commits November 20, 2024 23:08
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@@ -77,11 +79,20 @@ using VisitedListPtr = std::shared_ptr<VisitedList>;

class VisitedListPool {
public:
VisitedListPool(int initmaxpools, uint64_t numelements1, vsag::Allocator* allocator)
: allocator_(allocator) {
VisitedListPool(uint64_t numelements1, vsag::Allocator* allocator)
Copy link
Collaborator

Choose a reason for hiding this comment

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

numelements1 typo ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have renamed it to max_element_count

jinjiabao.jjb added 2 commits November 22, 2024 11:20
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@@ -161,7 +161,7 @@ class StaticHierarchicalNSW : public AlgorithmInterface<float> {

cur_element_count_ = 0;

visited_list_pool_ = new VisitedListPool(1, max_elements, allocator_);
visited_list_pool_ = new (allocator_) VisitedListPool(max_elements, allocator_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

visited_list_pool_ = allocator_->New(max_elements, allocator_);

@@ -187,7 +187,7 @@ class StaticHierarchicalNSW : public AlgorithmInterface<float> {
}
allocator_->Deallocate(element_levels_);
allocator_->Deallocate(linkLists_);
delete visited_list_pool_;
VisitedListPool::operator delete(visited_list_pool_, allocator_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

miss change ?

@@ -935,8 +935,8 @@ class StaticHierarchicalNSW : public AlgorithmInterface<float> {
throw std::runtime_error(
"Cannot Resize, max element is less than the current number of elements");

delete visited_list_pool_;
visited_list_pool_ = new VisitedListPool(1, new_max_elements, allocator_);
VisitedListPool::operator delete(visited_list_pool_, allocator_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@@ -1527,8 +1527,7 @@ class StaticHierarchicalNSW : public AlgorithmInterface<float> {
std::vector<std::mutex>(max_elements).swap(link_list_locks_);
std::vector<std::mutex>(MAX_LABEL_OPERATION_LOCKS).swap(label_op_locks_);

delete visited_list_pool_;
visited_list_pool_ = new VisitedListPool(1, max_elements, allocator_);
visited_list_pool_ = new (allocator_) VisitedListPool(max_elements, allocator_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
src/algorithm/hnswlib/hnswalg.cpp Show resolved Hide resolved
}

private:
std::deque<VisitedListPtr> pool;
vsag::Vector<VisitedListPtr> pool;
Copy link
Collaborator

Choose a reason for hiding this comment

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

pool -> pool_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/index/hgraph_index.cpp Show resolved Hide resolved
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

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

lgtm

@inabao inabao merged commit 7217fa3 into main Nov 24, 2024
2 checks passed
@inabao inabao deleted the ob-memory-leak-analyze branch November 24, 2024 14:48
inabao added a commit that referenced this pull request Nov 25, 2024
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resize index core dump
3 participants