From b0b526746b7489228236090047f3e5f455021a44 Mon Sep 17 00:00:00 2001 From: Joan Fontanals Martinez Date: Wed, 21 May 2025 13:03:36 +0200 Subject: [PATCH 1/5] refactor to use RAII when locking mutexes --- src/python_bindings/bindings.cpp | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/python_bindings/bindings.cpp b/src/python_bindings/bindings.cpp index 29360918d..4db2fcbf7 100644 --- a/src/python_bindings/bindings.cpp +++ b/src/python_bindings/bindings.cpp @@ -258,9 +258,10 @@ class PyHNSWLibIndex : public PyVecSimIndex { if (ind >= n_queries) { break; } - indexGuard.lock_shared(); - results[ind] = queryFunc((const char *)items.data(ind), param, query_params); - indexGuard.unlock_shared(); + { + std::shared_lock lock(indexGuard); + results[ind] = queryFunc((const char *)items.data(ind), param, query_params); + } } }; std::thread thread_objs[n_threads]; @@ -395,23 +396,21 @@ class PyHNSWLibIndex : public PyVecSimIndex { [&](const py::array &data, const py::array_t &labels) { while (true) { - bool exclusive = true; - barrier.lock(); + std::lock_guard barrier_guard(barrier); int ind = global_counter++; if (ind >= n_vectors) { - barrier.unlock(); break; } + // Use RAII for shared mutex with appropriate lock type if (ind % block_size != 0) { - indexGuard.lock_shared(); - exclusive = false; + // Read lock for normal operations + std::shared_lock index_guard(indexGuard); + this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); } else { - // Lock exclusively if we are performing resizing due to a new block. - indexGuard.lock(); + // Exclusive lock for block resizing operations + std::unique_lock index_guard(indexGuard); + this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); } - barrier.unlock(); - this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); - exclusive ? indexGuard.unlock() : indexGuard.unlock_shared(); } }; std::thread thread_objs[n_threads]; From 11a2d0c2d367085559b09e9b94f4b86ca08384b1 Mon Sep 17 00:00:00 2001 From: Joan Fontanals Martinez Date: Wed, 21 May 2025 15:28:19 +0200 Subject: [PATCH 2/5] capture mutex shared pointer by value to ensure validity at del time --- src/python_bindings/bindings.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/python_bindings/bindings.cpp b/src/python_bindings/bindings.cpp index 4db2fcbf7..3393c7544 100644 --- a/src/python_bindings/bindings.cpp +++ b/src/python_bindings/bindings.cpp @@ -236,7 +236,7 @@ class PyVecSimIndex { class PyHNSWLibIndex : public PyVecSimIndex { private: - std::shared_mutex indexGuard; // to protect parallel operations on the index. + std::shared_ptr indexGuard; // to protect parallel operations on the index. template // size_t/double for KNN/range queries. using QueryFunc = std::function; @@ -259,7 +259,7 @@ class PyHNSWLibIndex : public PyVecSimIndex { break; } { - std::shared_lock lock(indexGuard); + std::shared_lock lock(*indexGuard); results[ind] = queryFunc((const char *)items.data(ind), param, query_params); } } @@ -282,12 +282,14 @@ class PyHNSWLibIndex : public PyVecSimIndex { VecSimParams params = {.algo = VecSimAlgo_HNSWLIB, .algoParams = {.hnswParams = HNSWParams{hnsw_params}}}; this->index = std::shared_ptr(VecSimIndex_New(¶ms), VecSimIndex_Free); + this->indexGuard = std::make_shared(); } // @params is required only in V1. explicit PyHNSWLibIndex(const std::string &location) { this->index = std::shared_ptr(HNSWFactory::NewIndex(location), VecSimIndex_Free); + this->indexGuard = std::make_shared(); } void setDefaultEf(size_t ef) { @@ -404,11 +406,11 @@ class PyHNSWLibIndex : public PyVecSimIndex { // Use RAII for shared mutex with appropriate lock type if (ind % block_size != 0) { // Read lock for normal operations - std::shared_lock index_guard(indexGuard); + std::shared_lock index_guard(*indexGuard); this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); } else { // Exclusive lock for block resizing operations - std::unique_lock index_guard(indexGuard); + std::unique_lock index_guard(*indexGuard); this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); } } @@ -458,12 +460,14 @@ class PyHNSWLibIndex : public PyVecSimIndex { } PyBatchIterator createBatchIterator(const py::object &input, VecSimQueryParams *query_params) override { + py::array query(input); - auto del = [&](VecSimBatchIterator *pyBatchIter) { + // Passing indexGuardPtr by value, so that the refCount of the mutex + auto del = [indexGuardPtr = this->indexGuard](VecSimBatchIterator *pyBatchIter) { VecSimBatchIterator_Free(pyBatchIter); - this->indexGuard.unlock_shared(); + indexGuardPtr->unlock_shared(); }; - indexGuard.lock_shared(); + indexGuard->lock_shared(); auto py_batch_ptr = std::shared_ptr( VecSimBatchIterator_New(index.get(), (const char *)query.data(0), query_params), del); return PyBatchIterator(index, py_batch_ptr); From 5afe2881545914f98614c549b7efbecd5a227733 Mon Sep 17 00:00:00 2001 From: Joan Fontanals Martinez Date: Wed, 21 May 2025 19:17:49 +0200 Subject: [PATCH 3/5] Avoid deadlock involving GIL, and indexGuard --- src/python_bindings/bindings.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/python_bindings/bindings.cpp b/src/python_bindings/bindings.cpp index 3393c7544..00e5c80f0 100644 --- a/src/python_bindings/bindings.cpp +++ b/src/python_bindings/bindings.cpp @@ -462,6 +462,7 @@ class PyHNSWLibIndex : public PyVecSimIndex { VecSimQueryParams *query_params) override { py::array query(input); + py::gil_scoped_release py_gil; // Passing indexGuardPtr by value, so that the refCount of the mutex auto del = [indexGuardPtr = this->indexGuard](VecSimBatchIterator *pyBatchIter) { VecSimBatchIterator_Free(pyBatchIter); From 8c85e06d1556e2ee51da1186b782ffa8a3a5b25a Mon Sep 17 00:00:00 2001 From: Joan Fontanals Martinez Date: Thu, 22 May 2025 09:12:06 +0200 Subject: [PATCH 4/5] revert change to barrier and guard scoped locks --- src/python_bindings/bindings.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/python_bindings/bindings.cpp b/src/python_bindings/bindings.cpp index 00e5c80f0..41cb4bc85 100644 --- a/src/python_bindings/bindings.cpp +++ b/src/python_bindings/bindings.cpp @@ -236,8 +236,8 @@ class PyVecSimIndex { class PyHNSWLibIndex : public PyVecSimIndex { private: - std::shared_ptr indexGuard; // to protect parallel operations on the index. - template // size_t/double for KNN/range queries. + std::shared_ptr indexGuard; // to protect parallel operations on the index. + template // size_t/double for KNN/range queries. using QueryFunc = std::function; @@ -398,21 +398,24 @@ class PyHNSWLibIndex : public PyVecSimIndex { [&](const py::array &data, const py::array_t &labels) { while (true) { - std::lock_guard barrier_guard(barrier); + bool exclusive = true; + barrier.lock(); int ind = global_counter++; if (ind >= n_vectors) { + barrier.unlock(); break; } - // Use RAII for shared mutex with appropriate lock type if (ind % block_size != 0) { // Read lock for normal operations - std::shared_lock index_guard(*indexGuard); - this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); + indexGuard->lock_shared(); + exclusive = false; } else { // Exclusive lock for block resizing operations - std::unique_lock index_guard(*indexGuard); - this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); + indexGuard->lock(); } + barrier.unlock(); + this->addVectorInternal((const char *)data.data(ind), labels.at(ind)); + exclusive ? indexGuard->unlock() : indexGuard->unlock_shared(); } }; std::thread thread_objs[n_threads]; From 7ac126c8fa572880a3f0061a3e86ee876b8a3b13 Mon Sep 17 00:00:00 2001 From: Joan Fontanals Martinez Date: Thu, 22 May 2025 13:16:54 +0200 Subject: [PATCH 5/5] add small comment --- src/python_bindings/bindings.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/python_bindings/bindings.cpp b/src/python_bindings/bindings.cpp index 41cb4bc85..c8bdba618 100644 --- a/src/python_bindings/bindings.cpp +++ b/src/python_bindings/bindings.cpp @@ -236,8 +236,10 @@ class PyVecSimIndex { class PyHNSWLibIndex : public PyVecSimIndex { private: - std::shared_ptr indexGuard; // to protect parallel operations on the index. - template // size_t/double for KNN/range queries. + std::shared_ptr + indexGuard; // to protect parallel operations on the index. Make sure to release the GIL + // while locking the mutex. + template // size_t/double for KNN/range queries. using QueryFunc = std::function;