From 5af1a718f30be9c1de393fc75bcc0e1f04303123 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Thu, 18 Jul 2024 12:14:58 +0300 Subject: [PATCH 01/24] resolve conflicts after rebase --- src/VecSim/algorithms/hnsw/hnsw.h | 96 ++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 6615a06b5..5d77dd2c4 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -78,6 +78,93 @@ struct ElementMetaData { }; #pragma pack() // restore default packing +// Helper method that swaps the last element in the ids list with the given one (equivalent to +// removing the given element id from the list). +bool removeIdFromList(vecsim_stl::vector &element_ids_list, idType element_id); + +struct LevelData { + // A list of ids that are pointing to the node where each edge is *unidirectional* + vecsim_stl::vector *incomingEdges; + // Total size of incoming links to the node (both uni and bi directinal). + linkListSize totalIncomingLikns; + linkListSize numLinks; + // Flexible array member - https://en.wikipedia.org/wiki/Flexible_array_member + // Using this trick, we can have the links list as part of the LevelData struct, and avoid + // the need to dereference a pointer to get to the links list. + // We have to calculate the size of the struct manually, as `sizeof(LevelData)` will not include + // this member. We do so in the constructor of the index, under the name `levelDataSize` (and + // `elementGraphDataSize`). Notice that this member must be the last member of the struct and + // all nesting structs. + idType links[]; + + explicit LevelData(std::shared_ptr allocator) + : incomingEdges(new (allocator) vecsim_stl::vector(allocator)), + totalIncomingLikns(0), numLinks(0) {} + + linkListSize getNumLinks() const { return this->numLinks; } + idType getLinkAtPos(size_t pos) const { + assert(pos < numLinks); + return this->links[pos]; + } + const vecsim_stl::vector &getIncomingEdges() const { return *incomingEdges; } + std::vector copyLinks() { + std::vector links_copy; + links_copy.assign(links, links + numLinks); + return links_copy; + } + // Sets the outgoing links of the current element. + // Assumes that the object has the capacity to hold all the links. + void setLinks(vecsim_stl::vector &links) { + numLinks = links.size(); + memcpy(this->links, links.data(), numLinks * sizeof(idType)); + } + template + void setLinks(candidatesList &links) { + numLinks = 0; + for (auto &link : links) { + this->links[numLinks++] = link.second; + } + } + void popLink() { this->numLinks--; } + void setNumLinks(linkListSize num) { this->numLinks = num; } + void setLinkAtPos(size_t pos, idType node_id) { this->links[pos] = node_id; } + void appendLink(idType node_id) { this->links[this->numLinks++] = node_id; } + void newIncomingUnidirectionalEdge(idType node_id) { this->incomingEdges->push_back(node_id); } + bool removeIncomingUnidirectionalEdgeIfExists(idType node_id) { + return removeIdFromList(*this->incomingEdges, node_id); + } + void increaseTotalIncomingEdgesNum() { this->totalIncomingLikns++; } + void decreaseTotalIncomingEdgesNum() { this->totalIncomingLikns--; } + void swapNodeIdInIncomingEdges(idType id_before, idType id_after) { + auto it = std::find(this->incomingEdges->begin(), this->incomingEdges->end(), id_before); + // This should always succeed + assert(it != this->incomingEdges->end()); + *it = id_after; + } +}; + +struct ElementGraphData { + size_t toplevel; + std::mutex neighborsGuard; + LevelData *others; + LevelData level0; + + ElementGraphData(size_t maxLevel, size_t high_level_size, + std::shared_ptr allocator) + : toplevel(maxLevel), others(nullptr), level0(allocator) { + if (toplevel > 0) { + others = (LevelData *)allocator->callocate(high_level_size * toplevel); + if (others == nullptr) { + throw std::runtime_error("VecSim index low memory error"); + } + for (size_t i = 0; i < maxLevel; i++) { + new ((char *)others + i * high_level_size) LevelData(allocator); + } + } + } + ~ElementGraphData() = delete; // Should be destroyed using `destroyGraphData` +}; + //////////////////////////////////// HNSW index implementation //////////////////////////////////// template @@ -282,6 +369,7 @@ class HNSWIndex : public VecSimIndexAbstract, void markDeletedInternal(idType internalId); bool isMarkedDeleted(idType internalId) const; bool isInProcess(idType internalId) const; + void markInProcess(idType internalId); void unmarkInProcess(idType internalId); AddVectorCtx storeNewElement(labelType label, const void *vector_data); void removeAndSwapDeletedElement(idType internalId); @@ -451,6 +539,13 @@ bool HNSWIndex::isMarkedDeleted(idType internalId) const { return isMarkedAs(internalId); } +template +void HNSWIndex::markInProcess(idType internalId) { + // Atomically set the IN_PROCESS mark flag (note that other parallel threads may set the flags + // at the same time (for marking the element with IN_PROCCESS flag). + markAs(internalId); +} + template bool HNSWIndex::isInProcess(idType internalId) const { return isMarkedAs(internalId); @@ -506,7 +601,6 @@ void HNSWIndex::unlockNodeLinks(idType node_id) const { /** * helper functions */ - template void HNSWIndex::emplaceToHeap( vecsim_stl::abstract_priority_queue &heap, DistType dist, idType id) const { From ca5bcfb88a8a96d3a1870fff8eed3bebedef9234 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Mon, 1 Jul 2024 16:16:46 +0300 Subject: [PATCH 02/24] Move graph data structures to a new file, add new serialization version and adjust loading accordingly. --- src/VecSim/algorithms/hnsw/hnsw.h | 87 ------------------------------- 1 file changed, 87 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 5d77dd2c4..992d32613 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -78,93 +78,6 @@ struct ElementMetaData { }; #pragma pack() // restore default packing -// Helper method that swaps the last element in the ids list with the given one (equivalent to -// removing the given element id from the list). -bool removeIdFromList(vecsim_stl::vector &element_ids_list, idType element_id); - -struct LevelData { - // A list of ids that are pointing to the node where each edge is *unidirectional* - vecsim_stl::vector *incomingEdges; - // Total size of incoming links to the node (both uni and bi directinal). - linkListSize totalIncomingLikns; - linkListSize numLinks; - // Flexible array member - https://en.wikipedia.org/wiki/Flexible_array_member - // Using this trick, we can have the links list as part of the LevelData struct, and avoid - // the need to dereference a pointer to get to the links list. - // We have to calculate the size of the struct manually, as `sizeof(LevelData)` will not include - // this member. We do so in the constructor of the index, under the name `levelDataSize` (and - // `elementGraphDataSize`). Notice that this member must be the last member of the struct and - // all nesting structs. - idType links[]; - - explicit LevelData(std::shared_ptr allocator) - : incomingEdges(new (allocator) vecsim_stl::vector(allocator)), - totalIncomingLikns(0), numLinks(0) {} - - linkListSize getNumLinks() const { return this->numLinks; } - idType getLinkAtPos(size_t pos) const { - assert(pos < numLinks); - return this->links[pos]; - } - const vecsim_stl::vector &getIncomingEdges() const { return *incomingEdges; } - std::vector copyLinks() { - std::vector links_copy; - links_copy.assign(links, links + numLinks); - return links_copy; - } - // Sets the outgoing links of the current element. - // Assumes that the object has the capacity to hold all the links. - void setLinks(vecsim_stl::vector &links) { - numLinks = links.size(); - memcpy(this->links, links.data(), numLinks * sizeof(idType)); - } - template - void setLinks(candidatesList &links) { - numLinks = 0; - for (auto &link : links) { - this->links[numLinks++] = link.second; - } - } - void popLink() { this->numLinks--; } - void setNumLinks(linkListSize num) { this->numLinks = num; } - void setLinkAtPos(size_t pos, idType node_id) { this->links[pos] = node_id; } - void appendLink(idType node_id) { this->links[this->numLinks++] = node_id; } - void newIncomingUnidirectionalEdge(idType node_id) { this->incomingEdges->push_back(node_id); } - bool removeIncomingUnidirectionalEdgeIfExists(idType node_id) { - return removeIdFromList(*this->incomingEdges, node_id); - } - void increaseTotalIncomingEdgesNum() { this->totalIncomingLikns++; } - void decreaseTotalIncomingEdgesNum() { this->totalIncomingLikns--; } - void swapNodeIdInIncomingEdges(idType id_before, idType id_after) { - auto it = std::find(this->incomingEdges->begin(), this->incomingEdges->end(), id_before); - // This should always succeed - assert(it != this->incomingEdges->end()); - *it = id_after; - } -}; - -struct ElementGraphData { - size_t toplevel; - std::mutex neighborsGuard; - LevelData *others; - LevelData level0; - - ElementGraphData(size_t maxLevel, size_t high_level_size, - std::shared_ptr allocator) - : toplevel(maxLevel), others(nullptr), level0(allocator) { - if (toplevel > 0) { - others = (LevelData *)allocator->callocate(high_level_size * toplevel); - if (others == nullptr) { - throw std::runtime_error("VecSim index low memory error"); - } - for (size_t i = 0; i < maxLevel; i++) { - new ((char *)others + i * high_level_size) LevelData(allocator); - } - } - } - ~ElementGraphData() = delete; // Should be destroyed using `destroyGraphData` -}; - //////////////////////////////////// HNSW index implementation //////////////////////////////////// template From 57ae7f9b628f21152219e081ea926e7918f86541 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Mon, 1 Jul 2024 22:47:57 +0300 Subject: [PATCH 03/24] tmp adding full benchmark file path for debug --- tests/benchmark/bm_vecsim_general.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/benchmark/bm_vecsim_general.h b/tests/benchmark/bm_vecsim_general.h index bc95e54f5..395becd21 100644 --- a/tests/benchmark/bm_vecsim_general.h +++ b/tests/benchmark/bm_vecsim_general.h @@ -79,6 +79,6 @@ class BM_VecSimGeneral : public benchmark::Fixture { // Adds the library's root path to @file_name static inline std::string AttachRootPath(std::string file_name) { - return std::string(getenv("ROOT")) + "/" + file_name; + return std::string("/home/alon/Code/VectorSimilarity") + "/" + file_name; } }; From 8f94f23119db4879b6c5b62c3d89603b73ab1de9 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Tue, 2 Jul 2024 12:43:14 +0300 Subject: [PATCH 04/24] Add the files for the test --- tests/unit/test_hnsw.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index 79c9fbdf0..a3ffb42fa 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -1991,7 +1991,7 @@ TYPED_TEST(HNSWTest, HNSWSerializationV3) { // Test for multi and single for (size_t i = 0; i < 2; ++i) { - auto file_name = std::string(getenv("ROOT")) + "/tests/unit/data/1k-d4-L2-M8-ef_c10_" + + auto file_name = std::string("/home/alon/Code/VectorSimialrity") + "/tests/unit/data/1k-d4-L2-M8-ef_c10_" + VecSimType_ToString(TypeParam::get_index_type()) + "_" + multiToString[i] + ".hnsw_v3"; From 71ffd4abe309f62204a4d9a249ffc0d470907bd1 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Tue, 2 Jul 2024 12:43:57 +0300 Subject: [PATCH 05/24] revert file name --- tests/benchmark/bm_vecsim_general.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/benchmark/bm_vecsim_general.h b/tests/benchmark/bm_vecsim_general.h index 395becd21..bc95e54f5 100644 --- a/tests/benchmark/bm_vecsim_general.h +++ b/tests/benchmark/bm_vecsim_general.h @@ -79,6 +79,6 @@ class BM_VecSimGeneral : public benchmark::Fixture { // Adds the library's root path to @file_name static inline std::string AttachRootPath(std::string file_name) { - return std::string("/home/alon/Code/VectorSimilarity") + "/" + file_name; + return std::string(getenv("ROOT")) + "/" + file_name; } }; From 393801b5379b8edb3c4392526d62fa06ff46603f Mon Sep 17 00:00:00 2001 From: alonre24 Date: Tue, 2 Jul 2024 12:54:28 +0300 Subject: [PATCH 06/24] revert file name + format --- tests/unit/test_hnsw.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index a3ffb42fa..79c9fbdf0 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -1991,7 +1991,7 @@ TYPED_TEST(HNSWTest, HNSWSerializationV3) { // Test for multi and single for (size_t i = 0; i < 2; ++i) { - auto file_name = std::string("/home/alon/Code/VectorSimialrity") + "/tests/unit/data/1k-d4-L2-M8-ef_c10_" + + auto file_name = std::string(getenv("ROOT")) + "/tests/unit/data/1k-d4-L2-M8-ef_c10_" + VecSimType_ToString(TypeParam::get_index_type()) + "_" + multiToString[i] + ".hnsw_v3"; From ef97988e5b96b3b75b3efdf486940fc3a0b38975 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Tue, 2 Jul 2024 17:36:12 +0300 Subject: [PATCH 07/24] Collect unreachable nodes. WIP - connect them and test (+ fix memory tests) --- src/VecSim/algorithms/hnsw/graph_data.h | 1 + src/VecSim/algorithms/hnsw/hnsw.h | 59 ++++++++++++++++++-- src/VecSim/algorithms/hnsw/hnsw_serializer.h | 3 +- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 42 ++++++++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/graph_data.h b/src/VecSim/algorithms/hnsw/graph_data.h index 396e21227..146244879 100644 --- a/src/VecSim/algorithms/hnsw/graph_data.h +++ b/src/VecSim/algorithms/hnsw/graph_data.h @@ -68,6 +68,7 @@ struct ElementLevelData { } void increaseTotalIncomingEdgesNum() { this->totalIncomingLinks++; } void decreaseTotalIncomingEdgesNum() { this->totalIncomingLinks--; } + bool inDegreeZero() const { return this->totalIncomingLinks == 0; } void swapNodeIdInIncomingEdges(idType id_before, idType id_after) { auto it = std::find(this->incomingUnidirectionalEdges->begin(), this->incomingUnidirectionalEdges->end(), id_before); diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 992d32613..de1a19809 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -112,6 +112,7 @@ class HNSWIndex : public VecSimIndexAbstract, size_t curElementCount; idType entrypointNode; size_t maxLevel; // this is the top level of the entry point's element + vecsim_stl::vector unreachableNodes; // Index data vecsim_stl::vector vectorBlocks; @@ -122,6 +123,7 @@ class HNSWIndex : public VecSimIndexAbstract, // This is mutable since the object changes upon search operations as well (which are const). mutable VisitedNodesHandlerPool visitedNodesHandlerPool; mutable std::shared_mutex indexDataGuard; + std::mutex unreachableNodesGuard; #ifdef BUILD_TESTS #include "VecSim/algorithms/hnsw/hnsw_base_tests_friends.h" @@ -170,7 +172,7 @@ class HNSWIndex : public VecSimIndexAbstract, void revisitNeighborConnections(size_t level, idType new_node_id, const std::pair &neighbor_data, ElementLevelData &new_node_level, - ElementLevelData &neighbor_level); + ElementLevelData &neighbor_level, bool &unreachable); idType mutuallyConnectNewElement(idType new_node_id, candidatesMaxHeap &top_candidates, size_t level); void mutuallyUpdateForRepairedNode(idType node_id, size_t level, @@ -262,6 +264,8 @@ class HNSWIndex : public VecSimIndexAbstract, void lockNodeLinks(ElementGraphData *node_data) const; void unlockNodeLinks(ElementGraphData *node_data) const; VisitedNodesHandler *getVisitedList() const; + vecsim_stl::vector fetchAndClearUnreachableNodes(); + void setUnreachableNode(graphNodeType node); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; VecSimIndexBasicInfo basicInfo() const override; @@ -413,6 +417,23 @@ ElementLevelData &HNSWIndex::getElementLevelData(idType inte return getGraphDataByInternalId(internal_id)->getElementLevelData(level, this->levelDataSize); } +template +vecsim_stl::vector HNSWIndex::fetchAndClearUnreachableNodes() { + std::unique_lock lock(unreachableNodesGuard); + auto unreachable_copy = unreachableNodes; + unreachableNodes.clear(); + return unreachable_copy; +} + +template +void HNSWIndex::setUnreachableNode(graphNodeType node) { + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", + node.first, node.second); + std::unique_lock lock(unreachableNodesGuard); + unreachableNodes.push_back(node); +} + + template ElementLevelData &HNSWIndex::getElementLevelData(ElementGraphData *graph_data, size_t level) const { @@ -863,6 +884,7 @@ void HNSWIndex::revisitNeighborConnections( // as is. neighbor_level.setLinkAtPos(neighbour_neighbours_idx++, neighbor_level.getLinkAtPos(i)); update_cur_node_required = false; + unreachable = false; continue; } // Now we know that we are looking at a node to be removed from the neighbor's neighbors. @@ -878,6 +900,7 @@ void HNSWIndex::revisitNeighborConnections( // connection is mutual - both new node and the selected neighbor in each other's list. neighbor_level.setLinkAtPos(neighbour_neighbours_idx++, new_node_id); new_node_level.increaseTotalIncomingEdgesNum(); + unreachable = false; } else { // unidirectional connection - put the new node in the neighbour's incoming edges. neighbor_level.newIncomingUnidirectionalEdge(new_node_id); @@ -912,6 +935,7 @@ idType HNSWIndex::mutuallyConnectNewElement( assert(new_node_level_data.getNumLinks() == 0 && "The newly inserted element should have blank link list"); + bool unreachable = true; for (auto &neighbor_data : top_candidates_list) { idType selected_neighbor = neighbor_data.second; // neighbor's id auto *neighbor_graph_data = getGraphDataByInternalId(selected_neighbor); @@ -956,6 +980,7 @@ idType HNSWIndex::mutuallyConnectNewElement( new_node_level_data.increaseTotalIncomingEdgesNum(); unlockNodeLinks(new_node_level); unlockNodeLinks(neighbor_graph_data); + unreachable = false; continue; } @@ -963,7 +988,10 @@ idType HNSWIndex::mutuallyConnectNewElement( // We collect all the existing neighbors and the new node as candidates, and mutually update // the neighbor's neighbors. We also release the acquired locks inside this call. revisitNeighborConnections(level, new_node_id, neighbor_data, new_node_level_data, - neighbor_level_data); + neighbor_level_data, unreachable); + } + if (unreachable) { + this->setUnreachableNode(std::make_pair(new_node_id, level)); } return next_closest_entry_point; } @@ -1027,6 +1055,9 @@ void HNSWIndex::repairConnectionsForDeletion( } // anyway update the incoming nodes counter. node_level_data.decreaseTotalIncomingEdgesNum(); + if (node_level_data.inDegreeZero()) { + this->setUnreachableNode(std::make_pair(node_id, level)); + } } } } else { @@ -1198,6 +1229,12 @@ void HNSWIndex::SwapLastIdWithDeletedId(idType element_inter this->idToMetaData[element_internal_id] = this->idToMetaData[curElementCount]; + // TODO - CHECK FOR EVERY LEVEL + // if (this->unreachableNodes.contains(curElementCount)) { + // this->unreachableNodes.erase(curElementCount); + // this->unreachableNodes.insert(element_internal_id); + // } + if (curElementCount == this->entrypointNode) { this->entrypointNode = element_internal_id; } @@ -1554,6 +1591,9 @@ void HNSWIndex::mutuallyRemoveNeighborAtPos(ElementLevelData node_level.newIncomingUnidirectionalEdge(removed_node); } removed_node_level.decreaseTotalIncomingEdgesNum(); + if (removed_node_level.inDegreeZero()) { + this->setUnreachableNode(std::make_pair(removed_node, level)); + } } template @@ -1584,8 +1624,15 @@ void HNSWIndex::insertElementToGraph(idType element_id, for (auto level = static_cast(max_common_level); level >= 0; level--) { candidatesMaxHeap top_candidates = - searchLayer(curr_element, vector_data, level, efConstruction); - curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); + searchLayer(curr_element, vector_data, level, efConstruction); + // If the entry point was marked deleted between iterations, we may recieve an empty + // candidates set. + if (!top_candidates.empty()) { + curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); + } else { + // Node has no neighbors - it is defintly unreachable + this->setUnreachableNode(std::make_pair(element_id, level)); + } } } @@ -1609,6 +1656,7 @@ HNSWIndex::HNSWIndex(const HNSWParams *params, size_t random_seed, size_t pool_initial_size) : VecSimIndexAbstract(abstractInitParams), VecSimIndexTombstone(), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), + unreachableNodes(this->allocator), vectorBlocks(this->allocator), graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(pool_initial_size, maxElements, this->allocator) { @@ -1679,6 +1727,9 @@ void HNSWIndex::removeAndSwap(idType internalId) { // this point (after all the repair jobs are done). neighbour.removeIncomingUnidirectionalEdgeIfExists(internalId); neighbour.decreaseTotalIncomingEdgesNum(); + if (neighbour.inDegreeZero()) { + this->setUnreachableNode(std::make_pair(cur_level.getLinkAtPos(i), level)); + } } } diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer.h b/src/VecSim/algorithms/hnsw/hnsw_serializer.h index 90129ad1f..c59e393b0 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer.h @@ -6,7 +6,8 @@ HNSWIndex::HNSWIndex(std::ifstream &input, const HNSWParams Serializer::EncodingVersion version) : VecSimIndexAbstract(abstractInitParams), Serializer(version), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - epsilon(params->epsilon), vectorBlocks(this->allocator), graphDataBlocks(this->allocator), + epsilon(params->epsilon), unreachableNodes(this->allocator), vectorBlocks(this->allocator), + graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(1, maxElements, this->allocator) { diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index ffb567abf..22dbfcacd 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -228,6 +228,8 @@ class TieredHNSWIndex : public VecSimTieredIndex { return res; } + size_t connectUnreachableNodes(); + #ifdef BUILD_TESTS void getDataByLabel(labelType label, std::vector> &vectors_output) const; #endif @@ -851,6 +853,46 @@ double TieredHNSWIndex::getDistanceFrom_Unsafe(labelType lab return std::fmin(flat_dist, hnsw_dist); } +template +size_t TieredHNSWIndex::connectUnreachableNodes() { + // this->mainIndexGuard.lock_shared(); + // auto hnsw_index = this->getHNSWIndex(); + // + // size_t num_nodes_handeled = 0; + // hnsw_index->lockIndexDataGuard(); + // auto unreachable_nodes_copy = hnsw_index->getUnreachableNodes(); + // hnsw_index->clearUnreachableNodes(); + // hnsw_index->unlockIndexDataGuard(); + // + // for (idType node_id : unreachable_nodes_copy) { + // auto ep = hnsw_index->safeGetEntryPointState(); + // if (node_id == ep.first) { + // continue; // entry point is always reachable + // } + // // Mark the element as in proccess so we won't connect new neighbors to it in another thread, + // // we do it before we release the lock to ensure atomicity. + // hnsw_index->markInProcess(node_id); + // // Remove the element from the graph at every layer. + // auto element_top_level = hnsw_index->getGraphDataByInternalId(node_id)->toplevel; + // // for (int level = element_top_level; level >= 0; --level) { + // // hnsw_index->removeNodeNeighbors(node_id, level); + // // } + // if (hnsw_index->unreachableElement(node_id)) { + // // Insert the element again to the graph. + // hnsw_index->insertElementToGraph(node_id, element_top_level, ep.first, + // ep.second, hnsw_index->getDataByInternalId(node_id)); + // num_nodes_handeled++; + // } + // hnsw_index->unmarkInProcess(node_id); + // } + // this->mainIndexGuard.unlock_shared(); + // // TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, + // // "Done reinserting %zu elements into HNSW index", unreachable_nodes_copy.size()); + // return unreachable_nodes_copy.size(); + return 1; +} + + //////////////////////////////////////////////////////////////////////////////////////////////////// // TieredHNSW_BatchIterator // //////////////////////////////////////////////////////////////////////////////////////////////////// From 913d37ba3d85f43943eb522d26ed4a53eee61cd3 Mon Sep 17 00:00:00 2001 From: alonre24 Date: Wed, 10 Jul 2024 10:29:34 +0300 Subject: [PATCH 08/24] reinsert elements to graph (wip - deadlock since existing outgoing edges reinserted) --- src/VecSim/algorithms/hnsw/hnsw.h | 59 +++++++++++++++++++++++- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 46 ++---------------- 2 files changed, 62 insertions(+), 43 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index de1a19809..fbc582685 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -266,6 +266,7 @@ class HNSWIndex : public VecSimIndexAbstract, VisitedNodesHandler *getVisitedList() const; vecsim_stl::vector fetchAndClearUnreachableNodes(); void setUnreachableNode(graphNodeType node); + void connectUnreachableNodes(); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; VecSimIndexBasicInfo basicInfo() const override; @@ -308,6 +309,8 @@ class HNSWIndex : public VecSimIndexAbstract, void insertElementToGraph(idType element_id, size_t element_max_level, idType entry_point, size_t global_max_level, const void *vector_data); + void reinsertElementToGraphAtLevel(idType element_id, size_t level_to_insert); + #ifdef BUILD_TESTS /** * @brief Used for testing - store vector(s) data associated with a given label. This function @@ -427,12 +430,24 @@ vecsim_stl::vector HNSWIndex::fetchAndClearUn template void HNSWIndex::setUnreachableNode(graphNodeType node) { + if (isMarkedDeleted(node.first)) return; this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", node.first, node.second); std::unique_lock lock(unreachableNodesGuard); unreachableNodes.push_back(node); } +template +void HNSWIndex::connectUnreachableNodes() { + auto nodes_to_connect = fetchAndClearUnreachableNodes(); + if (nodes_to_connect.empty()) return; + for (auto node : nodes_to_connect) { + reinsertElementToGraphAtLevel(node.first, node.second); + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, + "Reinserted node id %zu in level %zu to the graph", node.first, node.second); + } + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted %zu nodes to the graph", nodes_to_connect.size()); +} template ElementLevelData &HNSWIndex::getElementLevelData(ElementGraphData *graph_data, @@ -946,6 +961,7 @@ idType HNSWIndex::mutuallyConnectNewElement( lockNodeLinks(neighbor_graph_data); lockNodeLinks(new_node_level); } + LevelData &new_node_level_data = getLevelData(new_node_level, level); // validations... assert(new_node_level_data.getNumLinks() <= max_M_cur && "Neighbors number exceeds limit"); @@ -1628,6 +1644,9 @@ void HNSWIndex::insertElementToGraph(idType element_id, // If the entry point was marked deleted between iterations, we may recieve an empty // candidates set. if (!top_candidates.empty()) { + LevelData &new_node_level_data = getLevelData(element_id, level); + assert(new_node_level_data.getNumLinks() == 0 && + "The newly inserted element should have blank link list"); curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { // Node has no neighbors - it is defintly unreachable @@ -1636,6 +1655,41 @@ void HNSWIndex::insertElementToGraph(idType element_id, } } +template +void HNSWIndex::reinsertElementToGraphAtLevel(idType element_id, + size_t level_to_insert) { + + this->markInProcess(element_id); + auto [entry_point, max_level] = this->safeGetEntryPointState(); + if (element_id == entry_point || entry_point == INVALID_ID) { + return; // entry point is always reachable + } + DistType cur_dist = std::numeric_limits::max(); + const void *vector_data = this->getDataByInternalId(element_id); + idType curr_element = entry_point; + cur_dist = this->distFunc(vector_data, getDataByInternalId(curr_element), this->dim); + for (auto level = max_level; level > level_to_insert; --level) { + // this is done for the levels which are above the level to insert + // to which we are going to insert the new element. We do + // a greedy search in the graph starting from the entry point + // at each level, and move on with the closest element we can find. + // When there is no improvement to do, we take a step down. + greedySearchLevel(vector_data, level, curr_element, cur_dist); + } + + candidatesMaxHeap top_candidates = + searchLayer(curr_element, vector_data, level_to_insert, efConstruction); + // If the entry point was marked deleted between iterations, we may recieve an empty + // candidates set. + if (!top_candidates.empty()) { + curr_element = mutuallyConnectNewElement(element_id, top_candidates, level_to_insert); + } else { + // Node has no neighbors - it is defintly unreachable + this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); + } + this->unmarkInProcess(element_id); +} + /** * Ctor / Dtor */ @@ -1817,6 +1871,7 @@ void HNSWIndex::removeVectorInPlace(const idType element_int // Finally, remove the element from the index and make a swap with the last internal id to // avoid fragmentation and reclaim memory when needed. removeAndSwap(element_internal_id); + this->connectUnreachableNodes(); } // Store the new element in the global data structures and keep the new state. In multithreaded @@ -1917,8 +1972,10 @@ void HNSWIndex::appendVector(const void *vector_data, const } unmarkInProcess(new_element_id); if (auxiliaryCtx == nullptr && state.currMaxLevel < state.elementMaxLevel) { - // No external auxiliaryCtx, so it's this function responsibility to release the lock. + // No external auxiliaryCtx, so it's this function responsibility to release the lock, and + // connect unreachable nodes that were created due to this operation. this->unlockIndexDataGuard(); + this->connectUnreachableNodes(); } } diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 22dbfcacd..fb91a5426 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -228,8 +228,6 @@ class TieredHNSWIndex : public VecSimTieredIndex { return res; } - size_t connectUnreachableNodes(); - #ifdef BUILD_TESTS void getDataByLabel(labelType label, std::vector> &vectors_output) const; #endif @@ -474,6 +472,8 @@ void TieredHNSWIndex::insertVectorToHNSW( if (state.elementMaxLevel > state.currMaxLevel) { hnsw_index->unlockIndexDataGuard(); } + // Reinsert nodes that became unreachable due to this operation. + hnsw_index->connectUnreachableNodes(); this->mainIndexGuard.unlock_shared(); } } @@ -591,7 +591,8 @@ void TieredHNSWIndex::executeRepairJob(HNSWRepairJob *job) { this->idToRepairJobsGuard.unlock(); hnsw_index->repairNodeConnections(job->node_id, job->level); - + // Reinsert nodes that became unreachable due to this operation. + hnsw_index->connectUnreachableNodes(); this->mainIndexGuard.unlock_shared(); } @@ -853,45 +854,6 @@ double TieredHNSWIndex::getDistanceFrom_Unsafe(labelType lab return std::fmin(flat_dist, hnsw_dist); } -template -size_t TieredHNSWIndex::connectUnreachableNodes() { - // this->mainIndexGuard.lock_shared(); - // auto hnsw_index = this->getHNSWIndex(); - // - // size_t num_nodes_handeled = 0; - // hnsw_index->lockIndexDataGuard(); - // auto unreachable_nodes_copy = hnsw_index->getUnreachableNodes(); - // hnsw_index->clearUnreachableNodes(); - // hnsw_index->unlockIndexDataGuard(); - // - // for (idType node_id : unreachable_nodes_copy) { - // auto ep = hnsw_index->safeGetEntryPointState(); - // if (node_id == ep.first) { - // continue; // entry point is always reachable - // } - // // Mark the element as in proccess so we won't connect new neighbors to it in another thread, - // // we do it before we release the lock to ensure atomicity. - // hnsw_index->markInProcess(node_id); - // // Remove the element from the graph at every layer. - // auto element_top_level = hnsw_index->getGraphDataByInternalId(node_id)->toplevel; - // // for (int level = element_top_level; level >= 0; --level) { - // // hnsw_index->removeNodeNeighbors(node_id, level); - // // } - // if (hnsw_index->unreachableElement(node_id)) { - // // Insert the element again to the graph. - // hnsw_index->insertElementToGraph(node_id, element_top_level, ep.first, - // ep.second, hnsw_index->getDataByInternalId(node_id)); - // num_nodes_handeled++; - // } - // hnsw_index->unmarkInProcess(node_id); - // } - // this->mainIndexGuard.unlock_shared(); - // // TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, - // // "Done reinserting %zu elements into HNSW index", unreachable_nodes_copy.size()); - // return unreachable_nodes_copy.size(); - return 1; -} - //////////////////////////////////////////////////////////////////////////////////////////////////// // TieredHNSW_BatchIterator // From dbc6a41956661078d5492172c3eb017f177c0711 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 15 Jul 2024 11:36:57 +0300 Subject: [PATCH 09/24] remove neighbors before reinsert, fixes --- src/VecSim/algorithms/hnsw/hnsw.h | 43 ++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index fbc582685..f884a277f 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -265,7 +265,7 @@ class HNSWIndex : public VecSimIndexAbstract, void unlockNodeLinks(ElementGraphData *node_data) const; VisitedNodesHandler *getVisitedList() const; vecsim_stl::vector fetchAndClearUnreachableNodes(); - void setUnreachableNode(graphNodeType node); + void setUnreachableNode(const graphNodeType &node); void connectUnreachableNodes(); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; @@ -429,7 +429,7 @@ vecsim_stl::vector HNSWIndex::fetchAndClearUn } template -void HNSWIndex::setUnreachableNode(graphNodeType node) { +void HNSWIndex::setUnreachableNode(const graphNodeType &node) { if (isMarkedDeleted(node.first)) return; this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", node.first, node.second); @@ -442,7 +442,9 @@ void HNSWIndex::connectUnreachableNodes() { auto nodes_to_connect = fetchAndClearUnreachableNodes(); if (nodes_to_connect.empty()) return; for (auto node : nodes_to_connect) { + this->markInProcess(node.first); reinsertElementToGraphAtLevel(node.first, node.second); + this->unmarkInProcess(node.first); this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted node id %zu in level %zu to the graph", node.first, node.second); } @@ -1644,8 +1646,7 @@ void HNSWIndex::insertElementToGraph(idType element_id, // If the entry point was marked deleted between iterations, we may recieve an empty // candidates set. if (!top_candidates.empty()) { - LevelData &new_node_level_data = getLevelData(element_id, level); - assert(new_node_level_data.getNumLinks() == 0 && + assert(getLevelData(element_id, level).getNumLinks() == 0 && "The newly inserted element should have blank link list"); curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { @@ -1659,10 +1660,9 @@ template void HNSWIndex::reinsertElementToGraphAtLevel(idType element_id, size_t level_to_insert) { - this->markInProcess(element_id); auto [entry_point, max_level] = this->safeGetEntryPointState(); - if (element_id == entry_point || entry_point == INVALID_ID) { - return; // entry point is always reachable + if (element_id == entry_point || isMarkedDeleted(element_id)) { + return; // entry point is always reachable, no need to connect deleted element } DistType cur_dist = std::numeric_limits::max(); const void *vector_data = this->getDataByInternalId(element_id); @@ -1677,6 +1677,24 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element greedySearchLevel(vector_data, level, curr_element, cur_dist); } + lockNodeLinks(element_id); + auto &node_data = getLevelData(element_id, level_to_insert); + bool unreachable = node_data.inDegreeZero(); + unlockNodeLinks(element_id); + if (!unreachable) { + return; // node is no longer unreachable, we can skip reinserting it. + } + // All links (if any) are unidirectional, let's remove them before we collect the new neighbors. + for (size_t i = 0; i < node_data.getNumLinks(); i++) { + idType neighbor = node_data.getLinkAtPos(i); + auto &neighbor_data = getLevelData(neighbor, level_to_insert); + lockNodeLinks(neighbor); + neighbor_data.removeIncomingUnidirectionalEdgeIfExists(element_id); // always true + neighbor_data.decreaseTotalIncomingEdgesNum(); + unlockNodeLinks(neighbor); + } + node_data.setNumLinks(0); + candidatesMaxHeap top_candidates = searchLayer(curr_element, vector_data, level_to_insert, efConstruction); // If the entry point was marked deleted between iterations, we may recieve an empty @@ -1687,7 +1705,6 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element // Node has no neighbors - it is defintly unreachable this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); } - this->unmarkInProcess(element_id); } /** @@ -1971,10 +1988,12 @@ void HNSWIndex::appendVector(const void *vector_data, const vector_data); } unmarkInProcess(new_element_id); - if (auxiliaryCtx == nullptr && state.currMaxLevel < state.elementMaxLevel) { - // No external auxiliaryCtx, so it's this function responsibility to release the lock, and - // connect unreachable nodes that were created due to this operation. - this->unlockIndexDataGuard(); + if (auxiliaryCtx == nullptr) { + // No external auxiliaryCtx, so it's this function responsibility to release the lock if + // needed connect unreachable nodes that were created due to this operation. + if (state.currMaxLevel < state.elementMaxLevel) { + this->unlockIndexDataGuard(); + } this->connectUnreachableNodes(); } } From 838e07bc3bc489aa9327f6130208f99fae53c4fa Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Tue, 16 Jul 2024 11:40:22 +0300 Subject: [PATCH 10/24] small fixes (WIP) --- src/VecSim/algorithms/hnsw/hnsw.h | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index f884a277f..a48282978 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -442,9 +442,7 @@ void HNSWIndex::connectUnreachableNodes() { auto nodes_to_connect = fetchAndClearUnreachableNodes(); if (nodes_to_connect.empty()) return; for (auto node : nodes_to_connect) { - this->markInProcess(node.first); reinsertElementToGraphAtLevel(node.first, node.second); - this->unmarkInProcess(node.first); this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted node id %zu in level %zu to the graph", node.first, node.second); } @@ -945,7 +943,7 @@ idType HNSWIndex::mutuallyConnectNewElement( // Use the heuristic to filter the top candidates, and get the next closest entry point. idType next_closest_entry_point = getNeighborsByHeuristic2(top_candidates_list, M); assert(top_candidates_list.size() <= M && - "Should be not be more than M candidates returned by the heuristic"); + "Should not be more than M candidates returned by the heuristic"); auto *new_node_level = getGraphDataByInternalId(new_node_id); ElementLevelData &new_node_level_data = getElementLevelData(new_node_level, level); @@ -1485,7 +1483,6 @@ void HNSWIndex::mutuallyUpdateForRepairedNode( template void HNSWIndex::repairNodeConnections(idType node_id, size_t level) { - vecsim_stl::vector neighbors_candidate_ids(this->allocator); // Use bitmaps for fast accesses: // node_orig_neighbours_set is used to differentiate between the neighbors that will *not* be @@ -1661,9 +1658,10 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element size_t level_to_insert) { auto [entry_point, max_level] = this->safeGetEntryPointState(); - if (element_id == entry_point || isMarkedDeleted(element_id)) { + if (element_id == entry_point || entry_point == INVALID_ID || isMarkedDeleted(element_id)) { return; // entry point is always reachable, no need to connect deleted element } + this->markInProcess(element_id); DistType cur_dist = std::numeric_limits::max(); const void *vector_data = this->getDataByInternalId(element_id); idType curr_element = entry_point; @@ -1682,29 +1680,22 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element bool unreachable = node_data.inDegreeZero(); unlockNodeLinks(element_id); if (!unreachable) { + this->unmarkInProcess(element_id); return; // node is no longer unreachable, we can skip reinserting it. } - // All links (if any) are unidirectional, let's remove them before we collect the new neighbors. - for (size_t i = 0; i < node_data.getNumLinks(); i++) { - idType neighbor = node_data.getLinkAtPos(i); - auto &neighbor_data = getLevelData(neighbor, level_to_insert); - lockNodeLinks(neighbor); - neighbor_data.removeIncomingUnidirectionalEdgeIfExists(element_id); // always true - neighbor_data.decreaseTotalIncomingEdgesNum(); - unlockNodeLinks(neighbor); - } - node_data.setNumLinks(0); candidatesMaxHeap top_candidates = searchLayer(curr_element, vector_data, level_to_insert, efConstruction); // If the entry point was marked deleted between iterations, we may recieve an empty // candidates set. if (!top_candidates.empty()) { + // TODO: USE A DIFFERENT METHOD THAT TAKES INTO ACCOUNT EXISTING NEIGHBORS AT ANY POINT IN TIME curr_element = mutuallyConnectNewElement(element_id, top_candidates, level_to_insert); } else { // Node has no neighbors - it is defintly unreachable this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); } + this->unmarkInProcess(element_id); } /** From d273e163f50dd97dc9c9fa3e412fc946e031d1ff Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Thu, 18 Jul 2024 00:44:58 +0300 Subject: [PATCH 11/24] Move to set + fix many bugs + decrease n vectors in long tests WIP: fix failing tests, add dedicated tests. rebase! --- src/VecSim/algorithms/hnsw/graph_data.h | 1 + src/VecSim/algorithms/hnsw/hnsw.h | 231 ++++++++++++++++--- src/VecSim/algorithms/hnsw/hnsw_serializer.h | 3 +- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 1 - src/VecSim/utils/vecsim_stl.h | 12 +- tests/unit/test_hnsw.cpp | 13 +- tests/unit/test_hnsw_multi.cpp | 13 +- tests/unit/test_hnsw_tiered.cpp | 2 +- 8 files changed, 220 insertions(+), 56 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/graph_data.h b/src/VecSim/algorithms/hnsw/graph_data.h index 146244879..5b7c13dd4 100644 --- a/src/VecSim/algorithms/hnsw/graph_data.h +++ b/src/VecSim/algorithms/hnsw/graph_data.h @@ -43,6 +43,7 @@ struct ElementLevelData { links_copy.assign(links, links + numLinks); return links_copy; } + size_t inDegree() const { return totalIncomingLinks; } // Sets the outgoing links of the current element. // Assumes that the object has the capacity to hold all the links. void setLinks(vecsim_stl::vector &links) { diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index a48282978..216b0c39a 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -49,12 +49,20 @@ template using candidatesLabelsMaxHeap = vecsim_stl::abstract_priority_queue; using graphNodeType = pair; // represented as: (element_id, level) +class hashForPair { +public: + size_t operator()(const graphNodeType &x) const { + return std::hash()(x.first) ^ std::hash()(x.second); + } +}; + ////////////////////////////////////// Auxiliary HNSW structs ////////////////////////////////////// // Vectors flags (for marking a specific vector) typedef enum { DELETE_MARK = 0x1, // element is logically deleted, but still exists in the graph IN_PROCESS = 0x2, // element is being inserted into the graph + IN_REPAIR = 0x4, } Flags; // The state of the index and the newly inserted vector to be passed into addVector API in case that @@ -112,7 +120,7 @@ class HNSWIndex : public VecSimIndexAbstract, size_t curElementCount; idType entrypointNode; size_t maxLevel; // this is the top level of the entry point's element - vecsim_stl::vector unreachableNodes; + vecsim_stl::unordered_set unreachableNodes; // Index data vecsim_stl::vector vectorBlocks; @@ -175,6 +183,8 @@ class HNSWIndex : public VecSimIndexAbstract, ElementLevelData &neighbor_level, bool &unreachable); idType mutuallyConnectNewElement(idType new_node_id, candidatesMaxHeap &top_candidates, size_t level); + size_t mutuallyReconnectElement(idType node_id, candidatesMaxHeap &top_candidates, + size_t level); void mutuallyUpdateForRepairedNode(idType node_id, size_t level, vecsim_stl::vector &neighbors_to_remove, vecsim_stl::vector &nodes_to_update, @@ -215,6 +225,9 @@ class HNSWIndex : public VecSimIndexAbstract, template void removeAndSwap(idType internalId); + vecsim_stl::unordered_set fetchAndClearUnreachableNodes(); + void setUnreachableNode(const graphNodeType &node); + size_t getVectorRelativeIndex(idType id) const { return id % this->blockSize; } // Flagging API @@ -264,8 +277,6 @@ class HNSWIndex : public VecSimIndexAbstract, void lockNodeLinks(ElementGraphData *node_data) const; void unlockNodeLinks(ElementGraphData *node_data) const; VisitedNodesHandler *getVisitedList() const; - vecsim_stl::vector fetchAndClearUnreachableNodes(); - void setUnreachableNode(const graphNodeType &node); void connectUnreachableNodes(); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; @@ -421,7 +432,8 @@ ElementLevelData &HNSWIndex::getElementLevelData(idType inte } template -vecsim_stl::vector HNSWIndex::fetchAndClearUnreachableNodes() { +vecsim_stl::unordered_set +HNSWIndex::fetchAndClearUnreachableNodes() { std::unique_lock lock(unreachableNodesGuard); auto unreachable_copy = unreachableNodes; unreachableNodes.clear(); @@ -430,23 +442,26 @@ vecsim_stl::vector HNSWIndex::fetchAndClearUn template void HNSWIndex::setUnreachableNode(const graphNodeType &node) { - if (isMarkedDeleted(node.first)) return; + if (isMarkedDeleted(node.first)) + return; this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", - node.first, node.second); + node.first, node.second); std::unique_lock lock(unreachableNodesGuard); - unreachableNodes.push_back(node); + unreachableNodes.insert(node); } template void HNSWIndex::connectUnreachableNodes() { auto nodes_to_connect = fetchAndClearUnreachableNodes(); - if (nodes_to_connect.empty()) return; + if (nodes_to_connect.empty()) + return; for (auto node : nodes_to_connect) { reinsertElementToGraphAtLevel(node.first, node.second); this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, - "Reinserted node id %zu in level %zu to the graph", node.first, node.second); + "Reinserted node id %zu in level %zu to the graph", node.first, node.second); } - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted %zu nodes to the graph", nodes_to_connect.size()); + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted %zu nodes to the graph", + nodes_to_connect.size()); } template @@ -475,6 +490,8 @@ void HNSWIndex::markDeletedInternal(idType internalId) { if (internalId == entrypointNode) { // Internally, we hold and release the entrypoint neighbors lock. replaceEntryPoint(); + this->log(VecSimCommonStrings::LOG_DEBUG_STRING, + "New entry point due to deletion is %zu", entrypointNode); } // Atomically set the deletion mark flag (note that other parallel threads may set the flags // at the same time (for changing the IN_PROCESS flag). @@ -928,6 +945,135 @@ void HNSWIndex::revisitNeighborConnections( } } +template +size_t HNSWIndex::mutuallyReconnectElement( + idType node_id, candidatesMaxHeap &top_candidates, size_t level) { + + size_t max_M_cur = level ? M : M0; + candidatesList selected_neighbors_cands(this->allocator); + vecsim_stl::vector nodes_to_update(this->allocator); + + auto *node_graph_data = getGraphDataByInternalId(node_id); + lockNodeLinks(node_graph_data); + auto &node_level_data = getLevelData(node_graph_data, level); + for (size_t i = 0; i < node_level_data.getNumLinks(); i++) { + nodes_to_update.push_back(node_level_data.getLinkAtPos(i)); + } + unlockNodeLinks(node_graph_data); + + selected_neighbors_cands.insert(selected_neighbors_cands.end(), top_candidates.begin(), + top_candidates.end()); + getNeighborsByHeuristic2(selected_neighbors_cands, M); + assert(selected_neighbors_cands.size() <= M && + "Should not be more than M candidates returned by the heuristic"); + + vecsim_stl::vector selected_neighbors(selected_neighbors_cands.size(), this->allocator); + std::transform(selected_neighbors_cands.begin(), selected_neighbors_cands.end(), + selected_neighbors.begin(), [](pair el) { return el.second; }); + + // Sort the selected neighbors set for fast lookup. + std::sort(selected_neighbors.begin(), selected_neighbors.end()); + + // Remove duplications in advanced here - if a selected neighbors already exists in this set. + std::vector ids_to_remove; + for (idType node : nodes_to_update) { + if (std::binary_search(selected_neighbors.begin(), selected_neighbors.end(), node)) { + ids_to_remove.push_back(node); + } + } + for (idType id_to_remove : ids_to_remove) { + removeIdFromList(nodes_to_update, id_to_remove); + } + nodes_to_update.insert(nodes_to_update.end(), selected_neighbors.begin(), + selected_neighbors.end()); + nodes_to_update.push_back(node_id); + + // Acquire the required locks for the updates, after sorting the nodes to update + // (to avoid deadlocks) + std::sort(nodes_to_update.begin(), nodes_to_update.end()); + size_t nodes_to_update_count = nodes_to_update.size(); + for (size_t i = 0; i < nodes_to_update_count; i++) { + lockNodeLinks(nodes_to_update[i]); + } + + // Try to make mutuall connection for the current node neighbors that were found. + node_level_data = getLevelData(node_graph_data, level); + for (size_t i = 0; i < node_level_data.getNumLinks(); i++) { + idType neighbor = node_level_data.getLinkAtPos(i); + if (!std::binary_search(nodes_to_update.begin(), nodes_to_update.end(), neighbor)) { + // This neighbor was added in the meantime while we release the lock - we cannot + // perofrm mutuall update. + continue; + } + if (std::find(selected_neighbors.begin(), selected_neighbors.end(), neighbor) != + selected_neighbors.end()) { + // Neighbor already exists, no need to connect it later on. + removeIdFromList(selected_neighbors, neighbor); + } + // Connect the neighbor back if it has the capacity, and... + auto &neighbor_data = getLevelData(neighbor, level); + if (neighbor_data.getNumLinks() < max_M_cur && !isMarkedDeleted(node_id) && + !isMarkedDeleted(neighbor)) { + // The edge was unidirectional (no exisiting outgoing edge from the neighbor to the + // node) + if (neighbor_data.removeIncomingUnidirectionalEdgeIfExists(node_id)) { + neighbor_data.appendLink(node_id); + node_level_data.increaseTotalIncomingEdgesNum(); + } + } + } + + // Go over the chosen new neighbors that are not connected yet and perform mutuall updates + // if possible. + for (idType chosen_id : selected_neighbors) { + // If this specific new neighbor is deleted, we don't add this connection and continue. + // Also, don't add a new node whose being indexed in parallel, as it may choose this node + // as its neighbor and create a double connection (then this node will have a duplicate + // neighbor). + if (isMarkedDeleted(chosen_id) || isInProcess(chosen_id) || + isMarkedAs(chosen_id)) { + continue; + } + auto &chosen_node_level_data = getLevelData(chosen_id, level); + // Perform mutuall or exclusive unidriectional connectoins accordin to degree limitations. + if (node_level_data.getNumLinks() < max_M_cur) { + node_level_data.appendLink(chosen_id); + chosen_node_level_data.increaseTotalIncomingEdgesNum(); + if (chosen_node_level_data.getNumLinks() < max_M_cur) { + // Unless the chosen new neighbor was already pointing the node id, connect them + // now. + if (!node_level_data.removeIncomingUnidirectionalEdgeIfExists(chosen_id)) { + chosen_node_level_data.appendLink(node_id); + node_level_data.increaseTotalIncomingEdgesNum(); + } // else - the neighbors is already pointing to the node and the undirectional + // edge has been removed. + } else if (!node_level_data.removeIncomingUnidirectionalEdgeIfExists(chosen_id)) { + // The neighbor cannot add the node to its neighbors (and it was not there before) - + // so we update the unidirectional incoming edges. + chosen_node_level_data.newIncomingUnidirectionalEdge(node_id); + } + } else if (chosen_node_level_data.getNumLinks() < max_M_cur) { + // We can only connect the chosen neighbor to the node as a unidirectional link, if it + // was not already exist. + auto &incoming_uni_edges = node_level_data.getIncomingEdges(); + if (std::find(incoming_uni_edges.begin(), incoming_uni_edges.end(), chosen_id) == + incoming_uni_edges.end()) { + chosen_node_level_data.appendLink(node_id); + node_level_data.increaseTotalIncomingEdgesNum(); + node_level_data.newIncomingUnidirectionalEdge(chosen_id); + } // nothing to do otherwise - edge already exists. + } + } + size_t ret = node_level_data.inDegree(); + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, + "Node %d now holds %zu links and %zu incoming links", node_id, + node_level_data.getNumLinks(), ret); + for (size_t i = 0; i < nodes_to_update_count; i++) { + unlockNodeLinks(nodes_to_update[i]); + } + return ret; +} + template idType HNSWIndex::mutuallyConnectNewElement( idType new_node_id, candidatesMaxHeap &top_candidates, size_t level) { @@ -955,13 +1101,13 @@ idType HNSWIndex::mutuallyConnectNewElement( idType selected_neighbor = neighbor_data.second; // neighbor's id auto *neighbor_graph_data = getGraphDataByInternalId(selected_neighbor); if (new_node_id < selected_neighbor) { - lockNodeLinks(new_node_level); + lockNodeLinks(new_node_graph_data); lockNodeLinks(neighbor_graph_data); } else { lockNodeLinks(neighbor_graph_data); - lockNodeLinks(new_node_level); + lockNodeLinks(new_node_graph_data); } - LevelData &new_node_level_data = getLevelData(new_node_level, level); + LevelData &new_node_level_data = getLevelData(new_node_graph_data, level); // validations... assert(new_node_level_data.getNumLinks() <= max_M_cur && "Neighbors number exceeds limit"); @@ -973,14 +1119,14 @@ idType HNSWIndex::mutuallyConnectNewElement( // The new node cannot add more neighbors this->log(VecSimCommonStrings::LOG_DEBUG_STRING, "Couldn't add all chosen neighbors upon inserting a new node"); - unlockNodeLinks(new_node_level); + unlockNodeLinks(new_node_graph_data); unlockNodeLinks(neighbor_graph_data); break; } // If one of the two nodes has already deleted - skip the operation. if (isMarkedDeleted(new_node_id) || isMarkedDeleted(selected_neighbor)) { - unlockNodeLinks(new_node_level); + unlockNodeLinks(new_node_graph_data); unlockNodeLinks(neighbor_graph_data); continue; } @@ -994,7 +1140,7 @@ idType HNSWIndex::mutuallyConnectNewElement( neighbor_level_data.increaseTotalIncomingEdgesNum(); neighbor_level_data.appendLink(new_node_id); new_node_level_data.increaseTotalIncomingEdgesNum(); - unlockNodeLinks(new_node_level); + unlockNodeLinks(new_node_graph_data); unlockNodeLinks(neighbor_graph_data); unreachable = false; continue; @@ -1245,11 +1391,13 @@ void HNSWIndex::SwapLastIdWithDeletedId(idType element_inter this->idToMetaData[element_internal_id] = this->idToMetaData[curElementCount]; - // TODO - CHECK FOR EVERY LEVEL - // if (this->unreachableNodes.contains(curElementCount)) { - // this->unreachableNodes.erase(curElementCount); - // this->unreachableNodes.insert(element_internal_id); - // } + // Update the unreachable nodes conrainer for every level in which an unreachable node exists. + for (size_t l = 0; l <= last_element->toplevel; l++) { + if (this->unreachableNodes.contains({curElementCount, l})) { + this->unreachableNodes.erase({curElementCount, l}); + this->unreachableNodes.insert(std::make_pair(element_internal_id, l)); + } + } if (curElementCount == this->entrypointNode) { this->entrypointNode = element_internal_id; @@ -1483,6 +1631,8 @@ void HNSWIndex::mutuallyUpdateForRepairedNode( template void HNSWIndex::repairNodeConnections(idType node_id, size_t level) { + this->markAs(node_id); + this->log(VecSimCommonStrings::LOG_DEBUG_STRING, "running repair for node %zu", node_id); vecsim_stl::vector neighbors_candidate_ids(this->allocator); // Use bitmaps for fast accesses: // node_orig_neighbours_set is used to differentiate between the neighbors that will *not* be @@ -1514,6 +1664,7 @@ void HNSWIndex::repairNodeConnections(idType node_id, size_t // If there are not deleted neighbors at that point the repair job has already been made by // another parallel job, and there is no need to repair the node anymore. if (deleted_neighbors.empty()) { + this->unmarkAs(node_id); return; } @@ -1585,6 +1736,7 @@ void HNSWIndex::repairNodeConnections(idType node_id, size_t // locks. mutuallyUpdateForRepairedNode(node_id, level, neighbors_to_remove, nodes_to_update, chosen_neighbors, max_M_cur); + this->unmarkAs(node_id); } template @@ -1644,7 +1796,7 @@ void HNSWIndex::insertElementToGraph(idType element_id, // candidates set. if (!top_candidates.empty()) { assert(getLevelData(element_id, level).getNumLinks() == 0 && - "The newly inserted element should have blank link list"); + "The newly inserted element should have blank link list"); curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { // Node has no neighbors - it is defintly unreachable @@ -1655,11 +1807,15 @@ void HNSWIndex::insertElementToGraph(idType element_id, template void HNSWIndex::reinsertElementToGraphAtLevel(idType element_id, - size_t level_to_insert) { + size_t level_to_insert) { auto [entry_point, max_level] = this->safeGetEntryPointState(); if (element_id == entry_point || entry_point == INVALID_ID || isMarkedDeleted(element_id)) { - return; // entry point is always reachable, no need to connect deleted element + return; // entry point is always reachable, no need to connect deleted element + } + if (this->isInProcess(element_id)) { + return; // If it being inserted - no need to reconnect. If it is being reconnected by other + // thread, also no need to reconnect. } this->markInProcess(element_id); DistType cur_dist = std::numeric_limits::max(); @@ -1681,19 +1837,20 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element unlockNodeLinks(element_id); if (!unreachable) { this->unmarkInProcess(element_id); - return; // node is no longer unreachable, we can skip reinserting it. + return; // node is no longer unreachable, we can skip reinserting it. } candidatesMaxHeap top_candidates = searchLayer(curr_element, vector_data, level_to_insert, efConstruction); - // If the entry point was marked deleted between iterations, we may recieve an empty - // candidates set. - if (!top_candidates.empty()) { - // TODO: USE A DIFFERENT METHOD THAT TAKES INTO ACCOUNT EXISTING NEIGHBORS AT ANY POINT IN TIME - curr_element = mutuallyConnectNewElement(element_id, top_candidates, level_to_insert); - } else { - // Node has no neighbors - it is defintly unreachable + if (top_candidates.empty()) { + // No candidates found (entry point been deleted in the meantime). this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); + } else { + size_t ret = mutuallyReconnectElement(element_id, top_candidates, level_to_insert); + if (ret == 0) { + // Node is still unreachable with 0 inDegree. + this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); + } } this->unmarkInProcess(element_id); } @@ -1718,9 +1875,8 @@ HNSWIndex::HNSWIndex(const HNSWParams *params, size_t random_seed, size_t pool_initial_size) : VecSimIndexAbstract(abstractInitParams), VecSimIndexTombstone(), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - unreachableNodes(this->allocator), - vectorBlocks(this->allocator), graphDataBlocks(this->allocator), - idToMetaData(maxElements, this->allocator), + unreachableNodes(this->allocator), vectorBlocks(this->allocator), + graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(pool_initial_size, maxElements, this->allocator) { M = params->M ? params->M : HNSW_DEFAULT_M; @@ -1795,6 +1951,11 @@ void HNSWIndex::removeAndSwap(idType internalId) { } } + // Remove it from the unreachable nodes container for every level in which it exists. + for (size_t l = 0; l <= element->toplevel; l++) { + this->unreachableNodes.erase({internalId, l}); + } + // Free the element's resources element->destroy(this->levelDataSize, this->allocator); diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer.h b/src/VecSim/algorithms/hnsw/hnsw_serializer.h index c59e393b0..b601dc06b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer.h @@ -7,8 +7,7 @@ HNSWIndex::HNSWIndex(std::ifstream &input, const HNSWParams : VecSimIndexAbstract(abstractInitParams), Serializer(version), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), epsilon(params->epsilon), unreachableNodes(this->allocator), vectorBlocks(this->allocator), - graphDataBlocks(this->allocator), - idToMetaData(maxElements, this->allocator), + graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(1, maxElements, this->allocator) { this->restoreIndexFields(input); diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index fb91a5426..f3a3703fb 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -854,7 +854,6 @@ double TieredHNSWIndex::getDistanceFrom_Unsafe(labelType lab return std::fmin(flat_dist, hnsw_dist); } - //////////////////////////////////////////////////////////////////////////////////////////////////// // TieredHNSW_BatchIterator // //////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/src/VecSim/utils/vecsim_stl.h b/src/VecSim/utils/vecsim_stl.h index 0b24c2258..739801504 100644 --- a/src/VecSim/utils/vecsim_stl.h +++ b/src/VecSim/utils/vecsim_stl.h @@ -92,18 +92,16 @@ class set : public VecsimBaseObject, public std::set, VecsimSTLA : VecsimBaseObject(alloc), std::set, VecsimSTLAllocator>(alloc) {} }; -template -class unordered_set - : public VecsimBaseObject, - public std::unordered_set, std::equal_to, VecsimSTLAllocator> { +template > +class unordered_set : public VecsimBaseObject, + public std::unordered_set, VecsimSTLAllocator> { public: explicit unordered_set(const std::shared_ptr &alloc) : VecsimBaseObject(alloc), - std::unordered_set, std::equal_to, VecsimSTLAllocator>(alloc) {} + std::unordered_set, VecsimSTLAllocator>(alloc) {} explicit unordered_set(size_t n_bucket, const std::shared_ptr &alloc) : VecsimBaseObject(alloc), - std::unordered_set, std::equal_to, VecsimSTLAllocator>(n_bucket, - alloc) {} + std::unordered_set, VecsimSTLAllocator>(n_bucket, alloc) {} }; } // namespace vecsim_stl diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index 79c9fbdf0..c2acc02a0 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -916,7 +916,7 @@ TYPED_TEST(HNSWTest, hnsw_bad_params) { } TYPED_TEST(HNSWTest, hnsw_delete_entry_point) { - size_t n = 10000; + size_t n = 1000; size_t dim = 4; size_t M = 2; @@ -931,11 +931,14 @@ TYPED_TEST(HNSWTest, hnsw_delete_entry_point) { ASSERT_TRUE(index != NULL); - int64_t vec[dim]; - for (size_t i = 0; i < dim; i++) - vec[i] = i; - for (size_t j = 0; j < n; j++) + for (size_t j = 0; j < n; j++) { + TEST_DATA_T vec[dim]; + for (size_t i = 0; i < dim; i++) { + vec[i] = std::rand() / (TEST_DATA_T)RAND_MAX; + ; + } VecSimIndex_AddVector(index, vec, j); + } VecSimIndexInfo info = VecSimIndex_Info(index); diff --git a/tests/unit/test_hnsw_multi.cpp b/tests/unit/test_hnsw_multi.cpp index 3f17c0422..8f9cd97ec 100644 --- a/tests/unit/test_hnsw_multi.cpp +++ b/tests/unit/test_hnsw_multi.cpp @@ -1223,7 +1223,7 @@ TYPED_TEST(HNSWMultiTest, test_query_runtime_params_user_build_args) { } TYPED_TEST(HNSWMultiTest, hnsw_delete_entry_point) { - size_t n = 10000; + size_t n = 1000; size_t per_label = 5; size_t dim = 4; size_t M = 2; @@ -1239,11 +1239,14 @@ TYPED_TEST(HNSWMultiTest, hnsw_delete_entry_point) { ASSERT_TRUE(index != NULL); - TEST_DATA_T vec[dim]; - for (size_t i = 0; i < dim; i++) - vec[i] = i; - for (size_t j = 0; j < n; j++) + for (size_t j = 0; j < n; j++) { + TEST_DATA_T vec[dim]; + for (size_t i = 0; i < dim; i++) { + vec[i] = std::rand() / (TEST_DATA_T)RAND_MAX; + ; + } VecSimIndex_AddVector(index, vec, j / per_label); + } VecSimIndexInfo info = VecSimIndex_Info(index); diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index 83c2e4995..ace1b2e5e 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -1005,7 +1005,7 @@ TYPED_TEST(HNSWTieredIndexTestBasic, deleteFromHNSWMultiLevels) { TYPED_TEST(HNSWTieredIndexTest, deleteFromHNSWWithRepairJobExec) { // Create TieredHNSW index instance with a mock queue. - size_t n = 1000; + size_t n = 200; size_t dim = 4; bool isMulti = TypeParam::isMulti(); From 153186b2b46d1d7542825fc99cb4f6e4a48c4cf3 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Thu, 18 Jul 2024 17:39:33 +0300 Subject: [PATCH 12/24] fix tests and rebase finish --- src/VecSim/algorithms/hnsw/hnsw.h | 24 +++++++++++------------- tests/unit/test_allocator.cpp | 14 +++++++++++--- tests/unit/test_common.cpp | 10 ++++++---- tests/unit/test_utils.cpp | 9 +++++++++ tests/unit/test_utils.h | 2 ++ 5 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 216b0c39a..e1f7dc865 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -856,7 +856,7 @@ void HNSWIndex::getNeighborsByHeuristic2_internal( template void HNSWIndex::revisitNeighborConnections( size_t level, idType new_node_id, const std::pair &neighbor_data, - ElementLevelData &new_node_level, ElementLevelData &neighbor_level) { + ElementLevelData &new_node_level, ElementLevelData &neighbor_level, bool &unreachable) { // Note - expect that node_lock and neighbor_lock are locked at that point. // Collect the existing neighbors and the new node as the neighbor's neighbors candidates. @@ -955,7 +955,7 @@ size_t HNSWIndex::mutuallyReconnectElement( auto *node_graph_data = getGraphDataByInternalId(node_id); lockNodeLinks(node_graph_data); - auto &node_level_data = getLevelData(node_graph_data, level); + auto &node_level_data = getElementLevelData(node_graph_data, level); for (size_t i = 0; i < node_level_data.getNumLinks(); i++) { nodes_to_update.push_back(node_level_data.getLinkAtPos(i)); } @@ -982,7 +982,7 @@ size_t HNSWIndex::mutuallyReconnectElement( } } for (idType id_to_remove : ids_to_remove) { - removeIdFromList(nodes_to_update, id_to_remove); + nodes_to_update.remove(id_to_remove); } nodes_to_update.insert(nodes_to_update.end(), selected_neighbors.begin(), selected_neighbors.end()); @@ -997,7 +997,7 @@ size_t HNSWIndex::mutuallyReconnectElement( } // Try to make mutuall connection for the current node neighbors that were found. - node_level_data = getLevelData(node_graph_data, level); + node_level_data = getElementLevelData(node_graph_data, level); for (size_t i = 0; i < node_level_data.getNumLinks(); i++) { idType neighbor = node_level_data.getLinkAtPos(i); if (!std::binary_search(nodes_to_update.begin(), nodes_to_update.end(), neighbor)) { @@ -1008,10 +1008,10 @@ size_t HNSWIndex::mutuallyReconnectElement( if (std::find(selected_neighbors.begin(), selected_neighbors.end(), neighbor) != selected_neighbors.end()) { // Neighbor already exists, no need to connect it later on. - removeIdFromList(selected_neighbors, neighbor); + selected_neighbors.remove(neighbor); } // Connect the neighbor back if it has the capacity, and... - auto &neighbor_data = getLevelData(neighbor, level); + auto &neighbor_data = getElementLevelData(neighbor, level); if (neighbor_data.getNumLinks() < max_M_cur && !isMarkedDeleted(node_id) && !isMarkedDeleted(neighbor)) { // The edge was unidirectional (no exisiting outgoing edge from the neighbor to the @@ -1034,7 +1034,7 @@ size_t HNSWIndex::mutuallyReconnectElement( isMarkedAs(chosen_id)) { continue; } - auto &chosen_node_level_data = getLevelData(chosen_id, level); + auto &chosen_node_level_data = getElementLevelData(chosen_id, level); // Perform mutuall or exclusive unidriectional connectoins accordin to degree limitations. if (node_level_data.getNumLinks() < max_M_cur) { node_level_data.appendLink(chosen_id); @@ -1091,8 +1091,8 @@ idType HNSWIndex::mutuallyConnectNewElement( assert(top_candidates_list.size() <= M && "Should not be more than M candidates returned by the heuristic"); - auto *new_node_level = getGraphDataByInternalId(new_node_id); - ElementLevelData &new_node_level_data = getElementLevelData(new_node_level, level); + auto *new_node_graph_data = getGraphDataByInternalId(new_node_id); + ElementLevelData &new_node_level_data = getElementLevelData(new_node_graph_data, level); assert(new_node_level_data.getNumLinks() == 0 && "The newly inserted element should have blank link list"); @@ -1107,7 +1107,7 @@ idType HNSWIndex::mutuallyConnectNewElement( lockNodeLinks(neighbor_graph_data); lockNodeLinks(new_node_graph_data); } - LevelData &new_node_level_data = getLevelData(new_node_graph_data, level); + new_node_level_data = getElementLevelData(new_node_graph_data, level); // validations... assert(new_node_level_data.getNumLinks() <= max_M_cur && "Neighbors number exceeds limit"); @@ -1795,8 +1795,6 @@ void HNSWIndex::insertElementToGraph(idType element_id, // If the entry point was marked deleted between iterations, we may recieve an empty // candidates set. if (!top_candidates.empty()) { - assert(getLevelData(element_id, level).getNumLinks() == 0 && - "The newly inserted element should have blank link list"); curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { // Node has no neighbors - it is defintly unreachable @@ -1832,7 +1830,7 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element } lockNodeLinks(element_id); - auto &node_data = getLevelData(element_id, level_to_insert); + auto &node_data = getElementLevelData(element_id, level_to_insert); bool unreachable = node_data.inDegreeZero(); unlockNodeLinks(element_id); if (!unreachable) { diff --git a/tests/unit/test_allocator.cpp b/tests/unit/test_allocator.cpp index c761e3a87..7874ad490 100644 --- a/tests/unit/test_allocator.cpp +++ b/tests/unit/test_allocator.cpp @@ -18,6 +18,8 @@ const size_t vecsimAllocationOverhead = VecSimAllocator::getAllocationOverheadSi const size_t hashTableNodeSize = getLabelsLookupNodeSize(); +const size_t unreachableNodeHashTableNodeSize = getUnreachableNodeSize(); + class AllocatorTest : public ::testing::Test {}; struct SimpleObject : public VecsimBaseObject { public: @@ -323,8 +325,9 @@ TYPED_TEST(IndexAllocatorTest, testIncomingEdgesSet) { /* Compute the expected allocation delta: * 1. empty incoming edges set in every level (+ allocator's header). - * 2. A node in the labels_lookup has table (+ allocator's header). If rehashing occurred, we - * account also for the diff in the buckets size (each bucket has sizeof(size_t) overhead). + * 2. A node in the labels_lookup hash table (+ allocator's header). If rehashing occurred, we + * account also for the diff in the buckets size (each bucket has sizeof(size_t) overhead). Same + * applies for the unreachable nodes unoredered set. * 3. Account for allocating link lists for levels higher than 0, if exists. * 4. Finally, expect an allocation of the data buffer in the incoming edges vector of vec1 due * to the insertion, and the fact that vec1 will re-select its neighbours. @@ -333,7 +336,10 @@ TYPED_TEST(IndexAllocatorTest, testIncomingEdgesSet) { (vec_max_level + 1) * (sizeof(vecsim_stl::vector) + vecsimAllocationOverhead) + hashTableNodeSize; size_t buckets_diff = hnswIndex->labelLookup.bucket_count() - buckets_num_before; - expected_allocation_delta += buckets_diff * sizeof(size_t); + size_t unreachable_nodes_overhead = + hnswIndex->unreachableNodes.bucket_count() * sizeof(graphNodeType) + + vecsimAllocationOverhead; + expected_allocation_delta += buckets_diff * sizeof(size_t) + unreachable_nodes_overhead; if (vec_max_level > 0) { expected_allocation_delta += hnswIndex->levelDataSize * vec_max_level + vecsimAllocationOverhead; @@ -448,6 +454,8 @@ TYPED_TEST(IndexAllocatorTest, test_hnsw_reclaim_memory) { // All data structures' memory returns to as it was, with the exceptional of the labels_lookup // (STL unordered_map with hash table implementation), that leaves some empty buckets. size_t hash_table_memory = hnswIndex->labelLookup.bucket_count() * sizeof(size_t); + // This applies for the unordered ser unreachable nodes as well. + hash_table_memory += hnswIndex->unreachableNodes.bucket_count() * sizeof(size_t); // Data block vectors do not shrink on resize so extra memory is expected. size_t block_vectors_memory = sizeof(DataBlock) * (hnswIndex->graphDataBlocks.capacity() + hnswIndex->vectorBlocks.capacity()) + diff --git a/tests/unit/test_common.cpp b/tests/unit/test_common.cpp index 4546da7f4..f791a8ca2 100644 --- a/tests/unit/test_common.cpp +++ b/tests/unit/test_common.cpp @@ -539,15 +539,17 @@ TEST(CommonAPITest, testlogTieredIndex) { GenerateAndAddVector(tiered_index, 4, 1); mock_thread_pool.thread_iteration(); tiered_index->deleteVector(1); - ASSERT_EQ(log.logBuffer.size(), 4); + ASSERT_EQ(log.logBuffer.size(), 5); ASSERT_EQ(log.logBuffer[0], "verbose: " + log.prefix + "Updating HNSW index capacity from 0 to 1024"); - ASSERT_EQ(log.logBuffer[1], + ASSERT_EQ(log.logBuffer[1], "debug: " + log.prefix + "New entry point due to deletion is " + + std::to_string(INVALID_ID)); + ASSERT_EQ(log.logBuffer[2], "verbose: " + log.prefix + "Tiered HNSW index GC: there are 1 ready swap jobs. Start executing 1 swap jobs"); - ASSERT_EQ(log.logBuffer[2], - "verbose: " + log.prefix + "Updating HNSW index capacity from 1024 to 0"); ASSERT_EQ(log.logBuffer[3], + "verbose: " + log.prefix + "Updating HNSW index capacity from 1024 to 0"); + ASSERT_EQ(log.logBuffer[4], "verbose: " + log.prefix + "Tiered HNSW index GC: done executing 1 swap jobs"); } diff --git a/tests/unit/test_utils.cpp b/tests/unit/test_utils.cpp index 7b99eba22..073fcfc8d 100644 --- a/tests/unit/test_utils.cpp +++ b/tests/unit/test_utils.cpp @@ -376,3 +376,12 @@ size_t getLabelsLookupNodeSize() { size_t memory_after = allocator->getAllocationSize(); return memory_after - memory_before; } + +size_t getUnreachableNodeSize() { + std::shared_ptr allocator = VecSimAllocator::newVecsimAllocator(); + auto dummy_lookup = vecsim_stl::unordered_set(1, allocator); + size_t memory_before = allocator->getAllocationSize(); + dummy_lookup.insert({1, 1}); // Insert a dummy {key, value} element pair. + size_t memory_after = allocator->getAllocationSize(); + return memory_after - memory_before; +} diff --git a/tests/unit/test_utils.h b/tests/unit/test_utils.h index 5b2b7e56d..deb9e1b8e 100644 --- a/tests/unit/test_utils.h +++ b/tests/unit/test_utils.h @@ -145,6 +145,8 @@ void runRangeQueryTest(VecSimIndex *index, const void *query, double radius, size_t getLabelsLookupNodeSize(); +size_t getUnreachableNodeSize(); + inline double GetInfVal(VecSimType type) { if (type == VecSimType_FLOAT64) { return exp(500); From b1e9b8d7d16daae553682a8c30aa4e4330f11cf5 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Fri, 19 Jul 2024 17:59:35 +0300 Subject: [PATCH 13/24] Add tests + small fixes and cleanups --- src/VecSim/algorithms/hnsw/graph_data.h | 6 ++- src/VecSim/algorithms/hnsw/hnsw.h | 59 +++++++++---------------- tests/unit/test_hnsw.cpp | 31 ++++++++++++- tests/unit/test_hnsw_multi.cpp | 1 - tests/unit/test_hnsw_tiered.cpp | 4 -- 5 files changed, 54 insertions(+), 47 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/graph_data.h b/src/VecSim/algorithms/hnsw/graph_data.h index 5b7c13dd4..3d3bc5753 100644 --- a/src/VecSim/algorithms/hnsw/graph_data.h +++ b/src/VecSim/algorithms/hnsw/graph_data.h @@ -30,6 +30,7 @@ struct ElementLevelData { : incomingUnidirectionalEdges(new (allocator) vecsim_stl::vector(allocator)), totalIncomingLinks(0), numLinks(0) {} + /*************************** Getters ********************************/ linkListSize getNumLinks() const { return this->numLinks; } idType getLinkAtPos(size_t pos) const { assert(pos < numLinks); @@ -38,12 +39,14 @@ struct ElementLevelData { const vecsim_stl::vector &getIncomingEdges() const { return *incomingUnidirectionalEdges; } + size_t inDegree() const { return totalIncomingLinks; } std::vector copyLinks() { std::vector links_copy; links_copy.assign(links, links + numLinks); return links_copy; } - size_t inDegree() const { return totalIncomingLinks; } + + /************************************ Setters ****************************/ // Sets the outgoing links of the current element. // Assumes that the object has the capacity to hold all the links. void setLinks(vecsim_stl::vector &links) { @@ -69,7 +72,6 @@ struct ElementLevelData { } void increaseTotalIncomingEdgesNum() { this->totalIncomingLinks++; } void decreaseTotalIncomingEdgesNum() { this->totalIncomingLinks--; } - bool inDegreeZero() const { return this->totalIncomingLinks == 0; } void swapNodeIdInIncomingEdges(idType id_before, idType id_after) { auto it = std::find(this->incomingUnidirectionalEdges->begin(), this->incomingUnidirectionalEdges->end(), id_before); diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index e1f7dc865..c2b0ecb10 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -61,8 +61,8 @@ class hashForPair { // Vectors flags (for marking a specific vector) typedef enum { DELETE_MARK = 0x1, // element is logically deleted, but still exists in the graph - IN_PROCESS = 0x2, // element is being inserted into the graph - IN_REPAIR = 0x4, + IN_PROCESS = 0x2, // element is being inserted/reinserted into the graph + IN_REPAIR = 0x4, // element is being repaired due to deletion of its neighbor(s) } Flags; // The state of the index and the newly inserted vector to be passed into addVector API in case that @@ -154,7 +154,6 @@ class HNSWIndex : public VecSimIndexAbstract, tag_t *elements_tags, tag_t visited_tag, std::unique_ptr &top_candidates, candidatesMaxHeap &candidate_set, DistType lowerBound, DistType radius) const; - template candidatesMaxHeap searchLayer(idType ep_id, const void *data_point, size_t layer, size_t ef) const; template @@ -298,8 +297,6 @@ class HNSWIndex : public VecSimIndexAbstract, void markDeletedInternal(idType internalId); bool isMarkedDeleted(idType internalId) const; bool isInProcess(idType internalId) const; - void markInProcess(idType internalId); - void unmarkInProcess(idType internalId); AddVectorCtx storeNewElement(labelType label, const void *vector_data); void removeAndSwapDeletedElement(idType internalId); void repairNodeConnections(idType node_id, size_t level); @@ -505,25 +502,11 @@ bool HNSWIndex::isMarkedDeleted(idType internalId) const { return isMarkedAs(internalId); } -template -void HNSWIndex::markInProcess(idType internalId) { - // Atomically set the IN_PROCESS mark flag (note that other parallel threads may set the flags - // at the same time (for marking the element with IN_PROCCESS flag). - markAs(internalId); -} - template bool HNSWIndex::isInProcess(idType internalId) const { return isMarkedAs(internalId); } -template -void HNSWIndex::unmarkInProcess(idType internalId) { - // Atomically unset the IN_PROCESS mark flag (note that other parallel threads may set the flags - // at the same time (for marking the element with IN_PROCCESS flag). - unmarkAs(internalId); -} - template void HNSWIndex::lockIndexDataGuard() const { indexDataGuard.lock(); @@ -736,7 +719,6 @@ void HNSWIndex::processCandidate_RangeSearch( } template -template candidatesMaxHeap HNSWIndex::searchLayer(idType ep_id, const void *data_point, size_t layer, size_t ef) const { @@ -748,7 +730,7 @@ HNSWIndex::searchLayer(idType ep_id, const void *data_point, candidatesMaxHeap candidate_set(this->allocator); DistType lowerBound; - if (!has_marked_deleted || !isMarkedDeleted(ep_id)) { + if (!isMarkedDeleted(ep_id)) { DistType dist = this->distFunc(data_point, getDataByInternalId(ep_id), this->dim); lowerBound = dist; top_candidates.emplace(dist, ep_id); @@ -768,9 +750,9 @@ HNSWIndex::searchLayer(idType ep_id, const void *data_point, } candidate_set.pop(); - processCandidate(curr_el_pair.second, data_point, layer, ef, - visited_nodes_handler->getElementsTags(), visited_tag, - top_candidates, candidate_set, lowerBound); + processCandidate(curr_el_pair.second, data_point, layer, ef, + visited_nodes_handler->getElementsTags(), visited_tag, + top_candidates, candidate_set, lowerBound); } returnVisitedList(visited_nodes_handler); @@ -1217,7 +1199,7 @@ void HNSWIndex::repairConnectionsForDeletion( } // anyway update the incoming nodes counter. node_level_data.decreaseTotalIncomingEdgesNum(); - if (node_level_data.inDegreeZero()) { + if (node_level_data.inDegree() == 0) { this->setUnreachableNode(std::make_pair(node_id, level)); } } @@ -1632,7 +1614,6 @@ void HNSWIndex::mutuallyUpdateForRepairedNode( template void HNSWIndex::repairNodeConnections(idType node_id, size_t level) { this->markAs(node_id); - this->log(VecSimCommonStrings::LOG_DEBUG_STRING, "running repair for node %zu", node_id); vecsim_stl::vector neighbors_candidate_ids(this->allocator); // Use bitmaps for fast accesses: // node_orig_neighbours_set is used to differentiate between the neighbors that will *not* be @@ -1758,7 +1739,7 @@ void HNSWIndex::mutuallyRemoveNeighborAtPos(ElementLevelData node_level.newIncomingUnidirectionalEdge(removed_node); } removed_node_level.decreaseTotalIncomingEdgesNum(); - if (removed_node_level.inDegreeZero()) { + if (removed_node_level.inDegree() == 0) { this->setUnreachableNode(std::make_pair(removed_node, level)); } } @@ -1791,7 +1772,7 @@ void HNSWIndex::insertElementToGraph(idType element_id, for (auto level = static_cast(max_common_level); level >= 0; level--) { candidatesMaxHeap top_candidates = - searchLayer(curr_element, vector_data, level, efConstruction); + searchLayer(curr_element, vector_data, level, efConstruction); // If the entry point was marked deleted between iterations, we may recieve an empty // candidates set. if (!top_candidates.empty()) { @@ -1812,10 +1793,10 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element return; // entry point is always reachable, no need to connect deleted element } if (this->isInProcess(element_id)) { - return; // If it being inserted - no need to reconnect. If it is being reconnected by other - // thread, also no need to reconnect. + return; // If it being inserted - no need to reconnect, and if it is being reconnected by + // another thread, there is no need for someelse to this job as well. } - this->markInProcess(element_id); + this->markAs(element_id); DistType cur_dist = std::numeric_limits::max(); const void *vector_data = this->getDataByInternalId(element_id); idType curr_element = entry_point; @@ -1831,15 +1812,15 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element lockNodeLinks(element_id); auto &node_data = getElementLevelData(element_id, level_to_insert); - bool unreachable = node_data.inDegreeZero(); + bool unreachable = node_data.inDegree() == 0; unlockNodeLinks(element_id); if (!unreachable) { - this->unmarkInProcess(element_id); - return; // node is no longer unreachable, we can skip reinserting it. + this->unmarkAs(element_id); + return; // node is no longer unreachable, we can skip on reinserting it. } candidatesMaxHeap top_candidates = - searchLayer(curr_element, vector_data, level_to_insert, efConstruction); + searchLayer(curr_element, vector_data, level_to_insert, efConstruction); if (top_candidates.empty()) { // No candidates found (entry point been deleted in the meantime). this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); @@ -1850,7 +1831,7 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); } } - this->unmarkInProcess(element_id); + this->unmarkAs(element_id); } /** @@ -1943,7 +1924,7 @@ void HNSWIndex::removeAndSwap(idType internalId) { // this point (after all the repair jobs are done). neighbour.removeIncomingUnidirectionalEdgeIfExists(internalId); neighbour.decreaseTotalIncomingEdgesNum(); - if (neighbour.inDegreeZero()) { + if (neighbour.inDegree() == 0) { this->setUnreachableNode(std::make_pair(cur_level.getLinkAtPos(i), level)); } } @@ -2137,10 +2118,10 @@ void HNSWIndex::appendVector(const void *vector_data, const insertElementToGraph(new_element_id, element_max_level, prev_entry_point, prev_max_level, vector_data); } - unmarkInProcess(new_element_id); + unmarkAs(new_element_id); if (auxiliaryCtx == nullptr) { // No external auxiliaryCtx, so it's this function responsibility to release the lock if - // needed connect unreachable nodes that were created due to this operation. + // needed and connect unreachable nodes that were created due to this operation. if (state.currMaxLevel < state.elementMaxLevel) { this->unlockIndexDataGuard(); } diff --git a/tests/unit/test_hnsw.cpp b/tests/unit/test_hnsw.cpp index c2acc02a0..bffd1c2f9 100644 --- a/tests/unit/test_hnsw.cpp +++ b/tests/unit/test_hnsw.cpp @@ -935,7 +935,6 @@ TYPED_TEST(HNSWTest, hnsw_delete_entry_point) { TEST_DATA_T vec[dim]; for (size_t i = 0; i < dim; i++) { vec[i] = std::rand() / (TEST_DATA_T)RAND_MAX; - ; } VecSimIndex_AddVector(index, vec, j); } @@ -2270,3 +2269,33 @@ TYPED_TEST(HNSWTest, FitMemoryTest) { VecSimIndex_Free(index); } + +TYPED_TEST(HNSWTest, NewNodeIsReachable) { + size_t dim = 4; + size_t n = 100; + HNSWParams params = {.dim = dim, .metric = VecSimMetric_L2, .efConstruction = 10}; + VecSimIndex *index = this->CreateNewIndex(params); + auto hnsw_index = this->CastToHNSW_Single(index); + + // Add 99 vectors + for (size_t i = 0; i < n; i++) { + GenerateAndAddVector(index, dim, i, i); + } + // Mark all vectors as deleted, except from first. + for (size_t i = 1; i < n; i++) { + hnsw_index->markDelete(i); + } + // Insert New node. The scan should not go through deleted candidates (otherwise, the new vector + // would have been unreachable. + EXPECT_EQ(hnsw_index->getEntryPointLabel(), 0); + GenerateAndAddVector(index, dim, n, n); + + TEST_DATA_T query[] = {(TEST_DATA_T)n, (TEST_DATA_T)n, (TEST_DATA_T)n, (TEST_DATA_T)n}; + auto verify_n_reachable = [&](size_t id, double score, size_t index) { + ASSERT_EQ(id, n); + ASSERT_EQ(score, 0); + }; + runTopKSearchTest(index, query, 1, verify_n_reachable); + + VecSimIndex_Free(index); +} diff --git a/tests/unit/test_hnsw_multi.cpp b/tests/unit/test_hnsw_multi.cpp index 8f9cd97ec..69c37801d 100644 --- a/tests/unit/test_hnsw_multi.cpp +++ b/tests/unit/test_hnsw_multi.cpp @@ -1243,7 +1243,6 @@ TYPED_TEST(HNSWMultiTest, hnsw_delete_entry_point) { TEST_DATA_T vec[dim]; for (size_t i = 0; i < dim; i++) { vec[i] = std::rand() / (TEST_DATA_T)RAND_MAX; - ; } VecSimIndex_AddVector(index, vec, j / per_label); } diff --git a/tests/unit/test_hnsw_tiered.cpp b/tests/unit/test_hnsw_tiered.cpp index ace1b2e5e..8cc42c9c3 100644 --- a/tests/unit/test_hnsw_tiered.cpp +++ b/tests/unit/test_hnsw_tiered.cpp @@ -2953,10 +2953,6 @@ TYPED_TEST(HNSWTieredIndexTest, switchWriteModes) { // (the label that we just inserted), and the first result should be this vector. auto ver_res = [&](size_t label, double score, size_t index) { if (index == 0) { - if (label != i % n_labels + n_labels && !TypeParam::isMulti()) { - // TODO: remove after we have a mechanism for connecting new elements - return; // this is flaky - ignore for now - } EXPECT_EQ(label, i % n_labels + n_labels); EXPECT_DOUBLE_EQ(score, 0); } From bc39e2f699b7cb3a61ce1f7f9b638d20b1886131 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 11:58:21 +0300 Subject: [PATCH 14/24] Use alpha (poc) --- src/VecSim/algorithms/hnsw/hnsw.h | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index c2b0ecb10..6450c7374 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -165,13 +165,13 @@ class HNSWIndex : public VecSimIndexAbstract, searchRangeBottomLayer_WithTimeout(idType ep_id, const void *data_point, double epsilon, DistType radius, void *timeoutCtx, VecSimQueryReply_Code *rc) const; - idType getNeighborsByHeuristic2(candidatesList &top_candidates, size_t M) const; + idType getNeighborsByHeuristic2(candidatesList &top_candidates, size_t M, float alpha = 1.0f) const; void getNeighborsByHeuristic2(candidatesList &top_candidates, size_t M, vecsim_stl::vector ¬_chosen_candidates) const; template void getNeighborsByHeuristic2_internal( candidatesList &top_candidates, size_t M, - vecsim_stl::vector *removed_candidates = nullptr) const; + vecsim_stl::vector *removed_candidates = nullptr, float alpha = 1.0f) const; // Helper function for re-selecting node's neighbors which was selected as a neighbor for // a newly inserted node. Also, responsible for mutually connect the new node and the neighbor // (unidirectional or bidirectional connection). @@ -762,13 +762,13 @@ HNSWIndex::searchLayer(idType ep_id, const void *data_point, template idType HNSWIndex::getNeighborsByHeuristic2(candidatesList &top_candidates, - const size_t M) const { + const size_t M, float alpha) const { if (top_candidates.size() < M) { return std::min_element(top_candidates.begin(), top_candidates.end(), [](const auto &a, const auto &b) { return a.first < b.first; }) ->second; } - getNeighborsByHeuristic2_internal(top_candidates, M, nullptr); + getNeighborsByHeuristic2_internal(top_candidates, M, nullptr, alpha); return top_candidates.front().second; } @@ -783,7 +783,7 @@ template template void HNSWIndex::getNeighborsByHeuristic2_internal( candidatesList &top_candidates, const size_t M, - vecsim_stl::vector *removed_candidates) const { + vecsim_stl::vector *removed_candidates, float alpha) const { if (top_candidates.size() < M) { return; } @@ -812,7 +812,7 @@ void HNSWIndex::getNeighborsByHeuristic2_internal( for (size_t i = 0; i < return_list.size(); i++) { DistType candidate_to_selected_dist = this->distFunc(cached_vectors[i], curr_vector, this->dim); - if (candidate_to_selected_dist < candidate_to_query_dist) { + if (candidate_to_selected_dist < candidate_to_query_dist * alpha) { if constexpr (record_removed) { removed_candidates->push_back(current_pair->second); } @@ -945,7 +945,8 @@ size_t HNSWIndex::mutuallyReconnectElement( selected_neighbors_cands.insert(selected_neighbors_cands.end(), top_candidates.begin(), top_candidates.end()); - getNeighborsByHeuristic2(selected_neighbors_cands, M); + float alpha = 1.5f; // we relax the heuristcs to get more neighbors + getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); assert(selected_neighbors_cands.size() <= M && "Should not be more than M candidates returned by the heuristic"); From 47247c9b221753b7ce7991d48cf33a8cbf251631 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 16:49:21 +0300 Subject: [PATCH 15/24] fix alpha --- src/VecSim/algorithms/hnsw/hnsw.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 6450c7374..4906087d0 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -812,7 +812,7 @@ void HNSWIndex::getNeighborsByHeuristic2_internal( for (size_t i = 0; i < return_list.size(); i++) { DistType candidate_to_selected_dist = this->distFunc(cached_vectors[i], curr_vector, this->dim); - if (candidate_to_selected_dist < candidate_to_query_dist * alpha) { + if (alpha * candidate_to_selected_dist < candidate_to_query_dist) { if constexpr (record_removed) { removed_candidates->push_back(current_pair->second); } From f5272dd1119203541abfafc16bc23f13df83b733 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 18:43:08 +0300 Subject: [PATCH 16/24] connect unreachable nodes permanent --- src/VecSim/algorithms/hnsw/hnsw.h | 44 ++++++++++++++------ src/VecSim/algorithms/hnsw/hnsw_serializer.h | 3 +- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 4 ++ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 4906087d0..6a06cbcff 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -121,6 +121,7 @@ class HNSWIndex : public VecSimIndexAbstract, idType entrypointNode; size_t maxLevel; // this is the top level of the entry point's element vecsim_stl::unordered_set unreachableNodes; + vecsim_stl::unordered_set unreachableNodesPermanent; // Index data vecsim_stl::vector vectorBlocks; @@ -224,8 +225,9 @@ class HNSWIndex : public VecSimIndexAbstract, template void removeAndSwap(idType internalId); - vecsim_stl::unordered_set fetchAndClearUnreachableNodes(); + vecsim_stl::unordered_set fetchAndClearUnreachableNodes(bool permanent); void setUnreachableNode(const graphNodeType &node); + void setUnreachablePermNode(const graphNodeType &node); size_t getVectorRelativeIndex(idType id) const { return id % this->blockSize; } @@ -276,7 +278,7 @@ class HNSWIndex : public VecSimIndexAbstract, void lockNodeLinks(ElementGraphData *node_data) const; void unlockNodeLinks(ElementGraphData *node_data) const; VisitedNodesHandler *getVisitedList() const; - void connectUnreachableNodes(); + void connectUnreachableNodes(bool permanent = false); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; VecSimIndexBasicInfo basicInfo() const override; @@ -430,9 +432,9 @@ ElementLevelData &HNSWIndex::getElementLevelData(idType inte template vecsim_stl::unordered_set -HNSWIndex::fetchAndClearUnreachableNodes() { +HNSWIndex::fetchAndClearUnreachableNodes(bool permanent) { std::unique_lock lock(unreachableNodesGuard); - auto unreachable_copy = unreachableNodes; + auto unreachable_copy = permanent ? unreachableNodesPermanent : unreachableNodes; unreachableNodes.clear(); return unreachable_copy; } @@ -448,8 +450,19 @@ void HNSWIndex::setUnreachableNode(const graphNodeType &node } template -void HNSWIndex::connectUnreachableNodes() { - auto nodes_to_connect = fetchAndClearUnreachableNodes(); +void HNSWIndex::setUnreachablePermNode(const graphNodeType &node) { + if (isMarkedDeleted(node.first)) + return; + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu still unreachable in " + "level %zu after trying to reinsert - setting as permanent unreachable", + node.first, node.second); + std::unique_lock lock(unreachableNodesGuard); + unreachableNodesPermanent.insert(node); +} + +template +void HNSWIndex::connectUnreachableNodes(bool permanent) { + auto nodes_to_connect = fetchAndClearUnreachableNodes(permanent); if (nodes_to_connect.empty()) return; for (auto node : nodes_to_connect) { @@ -945,8 +958,9 @@ size_t HNSWIndex::mutuallyReconnectElement( selected_neighbors_cands.insert(selected_neighbors_cands.end(), top_candidates.begin(), top_candidates.end()); - float alpha = 1.5f; // we relax the heuristcs to get more neighbors - getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); + // float alpha = 1.5f; // we relax the heuristcs to get more neighbors + // getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); + getNeighborsByHeuristic2(selected_neighbors_cands, M); assert(selected_neighbors_cands.size() <= M && "Should not be more than M candidates returned by the heuristic"); @@ -1380,6 +1394,10 @@ void HNSWIndex::SwapLastIdWithDeletedId(idType element_inter this->unreachableNodes.erase({curElementCount, l}); this->unreachableNodes.insert(std::make_pair(element_internal_id, l)); } + if (this->unreachableNodesPermanent.contains({curElementCount, l})) { + this->unreachableNodesPermanent.erase({curElementCount, l}); + this->unreachableNodesPermanent.insert(std::make_pair(element_internal_id, l)); + } } if (curElementCount == this->entrypointNode) { @@ -1779,8 +1797,8 @@ void HNSWIndex::insertElementToGraph(idType element_id, if (!top_candidates.empty()) { curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { - // Node has no neighbors - it is defintly unreachable - this->setUnreachableNode(std::make_pair(element_id, level)); + // Node still has no neighbors - it is defintly unreachable + this->setUnreachablePermNode(std::make_pair(element_id, level)); } } } @@ -1829,7 +1847,7 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element size_t ret = mutuallyReconnectElement(element_id, top_candidates, level_to_insert); if (ret == 0) { // Node is still unreachable with 0 inDegree. - this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); + this->setUnreachablePermNode(std::make_pair(element_id, level_to_insert)); } } this->unmarkAs(element_id); @@ -1855,7 +1873,8 @@ HNSWIndex::HNSWIndex(const HNSWParams *params, size_t random_seed, size_t pool_initial_size) : VecSimIndexAbstract(abstractInitParams), VecSimIndexTombstone(), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - unreachableNodes(this->allocator), vectorBlocks(this->allocator), + unreachableNodes(this->allocator), unreachableNodesPermanent(this->allocator), +vectorBlocks(this->allocator), graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(pool_initial_size, maxElements, this->allocator) { @@ -1934,6 +1953,7 @@ void HNSWIndex::removeAndSwap(idType internalId) { // Remove it from the unreachable nodes container for every level in which it exists. for (size_t l = 0; l <= element->toplevel; l++) { this->unreachableNodes.erase({internalId, l}); + this->unreachableNodesPermanent.erase({internalId, l}); } // Free the element's resources diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer.h b/src/VecSim/algorithms/hnsw/hnsw_serializer.h index b601dc06b..c4d15ebb3 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer.h @@ -6,7 +6,8 @@ HNSWIndex::HNSWIndex(std::ifstream &input, const HNSWParams Serializer::EncodingVersion version) : VecSimIndexAbstract(abstractInitParams), Serializer(version), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - epsilon(params->epsilon), unreachableNodes(this->allocator), vectorBlocks(this->allocator), + epsilon(params->epsilon), unreachableNodes(this->allocator), unreachableNodesPermanent(this->allocator), + vectorBlocks(this->allocator), graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(1, maxElements, this->allocator) { diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index f3a3703fb..135a99983 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -208,6 +208,10 @@ class TieredHNSWIndex : public VecSimTieredIndex { TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, "running asynchronous GC for tiered HNSW index"); this->executeReadySwapJobs(this->pendingSwapJobsThreshold); + // Try to reinsert permanent unreachable nodes + TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, + "Try to reinsert permanent unreachable nodes"); + this->getHNSWIndex()->connectUnreachableNodes(true); } void acquireSharedLocks() override { this->flatIndexGuard.lock_shared(); From 1db59e056a17f2d2309690bb903c6b44c219ec02 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 22:18:47 +0300 Subject: [PATCH 17/24] connect unreachable nodes permanent while resize index --- src/VecSim/algorithms/hnsw/hnsw.h | 10 ++++++---- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 4 ---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 6a06cbcff..ffbc2ee7b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -465,13 +465,11 @@ void HNSWIndex::connectUnreachableNodes(bool permanent) { auto nodes_to_connect = fetchAndClearUnreachableNodes(permanent); if (nodes_to_connect.empty()) return; + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Try to reinsert %zu nodes to the graph", + nodes_to_connect.size()); for (auto node : nodes_to_connect) { reinsertElementToGraphAtLevel(node.first, node.second); - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, - "Reinserted node id %zu in level %zu to the graph", node.first, node.second); } - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Reinserted %zu nodes to the graph", - nodes_to_connect.size()); } template @@ -1514,6 +1512,10 @@ void HNSWIndex::resizeIndexCommon(size_t new_max_elements) { idToMetaData.shrink_to_fit(); maxElements = new_max_elements; + // Try to reinsert permanent unreachable nodes + this->log(VecSimCommonStrings::LOG_NOTICE_STRING, + "Goiong over permanent unreachable nodes:"); + this->connectUnreachableNodes(true); } template diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index 135a99983..f3a3703fb 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -208,10 +208,6 @@ class TieredHNSWIndex : public VecSimTieredIndex { TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, "running asynchronous GC for tiered HNSW index"); this->executeReadySwapJobs(this->pendingSwapJobsThreshold); - // Try to reinsert permanent unreachable nodes - TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, - "Try to reinsert permanent unreachable nodes"); - this->getHNSWIndex()->connectUnreachableNodes(true); } void acquireSharedLocks() override { this->flatIndexGuard.lock_shared(); From 005a958a9f2f360a25a9c0bb7241a0224cbf4486 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 22:39:56 +0300 Subject: [PATCH 18/24] connect unreachable nodes permanent IN TIERED ONLY before adding / removing blocks --- src/VecSim/algorithms/hnsw/hnsw.h | 4 ---- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 12 +++++++++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index ffbc2ee7b..4bd056659 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -1512,10 +1512,6 @@ void HNSWIndex::resizeIndexCommon(size_t new_max_elements) { idToMetaData.shrink_to_fit(); maxElements = new_max_elements; - // Try to reinsert permanent unreachable nodes - this->log(VecSimCommonStrings::LOG_NOTICE_STRING, - "Goiong over permanent unreachable nodes:"); - this->connectUnreachableNodes(true); } template diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index f3a3703fb..fad3ed7ba 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -208,6 +208,12 @@ class TieredHNSWIndex : public VecSimTieredIndex { TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, "running asynchronous GC for tiered HNSW index"); this->executeReadySwapJobs(this->pendingSwapJobsThreshold); + // Try to reinsert permanent unreachable nodes + TIERED_LOG(VecSimCommonStrings::LOG_NOTICE_STRING, + "Goiong over permanent unreachable nodes:"); + this->mainIndexGuard.lock_shared(); + this->getHNSWIndex()->connectUnreachableNodes(true); + this->mainIndexGuard.unlock_shared(); } void acquireSharedLocks() override { this->flatIndexGuard.lock_shared(); @@ -423,9 +429,13 @@ void TieredHNSWIndex::insertVectorToHNSW( hnsw_index->lockIndexDataGuard(); // Check if resizing is needed for HNSW index (requires write lock). if (hnsw_index->indexCapacity() == hnsw_index->indexSize()) { + hnsw_index->unlockIndexDataGuard(); + // Try to reinsert permanent unreachable nodes + TIERED_LOG(VecSimCommonStrings::LOG_NOTICE_STRING, + "Goiong over permanent unreachable nodes:"); + hnsw_index->connectUnreachableNodes(true); // Release the inner HNSW data lock before we re-acquire the global HNSW lock. this->mainIndexGuard.unlock_shared(); - hnsw_index->unlockIndexDataGuard(); this->mainIndexGuard.lock(); hnsw_index->lockIndexDataGuard(); From 46b75640bcfda3c33589cef5e30f92f1ff8de72f Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 22:46:07 +0300 Subject: [PATCH 19/24] bring alpha back --- src/VecSim/algorithms/hnsw/hnsw.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 4bd056659..9190f6840 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -956,9 +956,8 @@ size_t HNSWIndex::mutuallyReconnectElement( selected_neighbors_cands.insert(selected_neighbors_cands.end(), top_candidates.begin(), top_candidates.end()); - // float alpha = 1.5f; // we relax the heuristcs to get more neighbors - // getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); - getNeighborsByHeuristic2(selected_neighbors_cands, M); + float alpha = 1.5f; // we relax the heuristcs to get more neighbors + getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); assert(selected_neighbors_cands.size() <= M && "Should not be more than M candidates returned by the heuristic"); From 13069dc564cea3e230b7a32f1d72c2bb93fc2bc1 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Mon, 22 Jul 2024 22:58:28 +0300 Subject: [PATCH 20/24] fix - clear the right collection --- src/VecSim/algorithms/hnsw/hnsw.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 9190f6840..5812ae366 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -435,7 +435,7 @@ vecsim_stl::unordered_set HNSWIndex::fetchAndClearUnreachableNodes(bool permanent) { std::unique_lock lock(unreachableNodesGuard); auto unreachable_copy = permanent ? unreachableNodesPermanent : unreachableNodes; - unreachableNodes.clear(); + permanent ? unreachableNodesPermanent.clear() : unreachableNodes.clear(); return unreachable_copy; } From ed8baa6e2f55eccb0fdad599ebb0100ddc37bc4c Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Tue, 23 Jul 2024 00:17:42 +0300 Subject: [PATCH 21/24] use different alpha for first reinsertion and later --- src/VecSim/algorithms/hnsw/hnsw.h | 78 ++++++++++---------- src/VecSim/algorithms/hnsw/hnsw_serializer.h | 2 +- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 6 +- 3 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 5812ae366..e061a5bbe 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -121,7 +121,7 @@ class HNSWIndex : public VecSimIndexAbstract, idType entrypointNode; size_t maxLevel; // this is the top level of the entry point's element vecsim_stl::unordered_set unreachableNodes; - vecsim_stl::unordered_set unreachableNodesPermanent; + vecsim_stl::unordered_set hardUnreachableNodes; // Index data vecsim_stl::vector vectorBlocks; @@ -183,6 +183,7 @@ class HNSWIndex : public VecSimIndexAbstract, ElementLevelData &neighbor_level, bool &unreachable); idType mutuallyConnectNewElement(idType new_node_id, candidatesMaxHeap &top_candidates, size_t level); + template size_t mutuallyReconnectElement(idType node_id, candidatesMaxHeap &top_candidates, size_t level); void mutuallyUpdateForRepairedNode(idType node_id, size_t level, @@ -225,9 +226,10 @@ class HNSWIndex : public VecSimIndexAbstract, template void removeAndSwap(idType internalId); - vecsim_stl::unordered_set fetchAndClearUnreachableNodes(bool permanent); + template + vecsim_stl::unordered_set fetchAndClearUnreachableNodes(); + template void setUnreachableNode(const graphNodeType &node); - void setUnreachablePermNode(const graphNodeType &node); size_t getVectorRelativeIndex(idType id) const { return id % this->blockSize; } @@ -246,6 +248,8 @@ class HNSWIndex : public VecSimIndexAbstract, } void mutuallyRemoveNeighborAtPos(ElementLevelData &node_level, size_t level, idType node_id, size_t pos); + template + void reinsertElementToGraphAtLevel(idType element_id, size_t level_to_insert); public: HNSWIndex(const HNSWParams *params, const AbstractIndexInitParams &abstractInitParams, @@ -278,7 +282,8 @@ class HNSWIndex : public VecSimIndexAbstract, void lockNodeLinks(ElementGraphData *node_data) const; void unlockNodeLinks(ElementGraphData *node_data) const; VisitedNodesHandler *getVisitedList() const; - void connectUnreachableNodes(bool permanent = false); + template + void connectUnreachableNodes(); void returnVisitedList(VisitedNodesHandler *visited_nodes_handler) const; VecSimIndexInfo info() const override; VecSimIndexBasicInfo basicInfo() const override; @@ -319,8 +324,6 @@ class HNSWIndex : public VecSimIndexAbstract, void insertElementToGraph(idType element_id, size_t element_max_level, idType entry_point, size_t global_max_level, const void *vector_data); - void reinsertElementToGraphAtLevel(idType element_id, size_t level_to_insert); - #ifdef BUILD_TESTS /** * @brief Used for testing - store vector(s) data associated with a given label. This function @@ -431,44 +434,43 @@ ElementLevelData &HNSWIndex::getElementLevelData(idType inte } template +template vecsim_stl::unordered_set -HNSWIndex::fetchAndClearUnreachableNodes(bool permanent) { +HNSWIndex::fetchAndClearUnreachableNodes() { std::unique_lock lock(unreachableNodesGuard); - auto unreachable_copy = permanent ? unreachableNodesPermanent : unreachableNodes; - permanent ? unreachableNodesPermanent.clear() : unreachableNodes.clear(); + auto unreachable_copy = force ? hardUnreachableNodes : unreachableNodes; + force ? hardUnreachableNodes.clear() : unreachableNodes.clear(); return unreachable_copy; } template +template void HNSWIndex::setUnreachableNode(const graphNodeType &node) { if (isMarkedDeleted(node.first)) return; - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", - node.first, node.second); std::unique_lock lock(unreachableNodesGuard); - unreachableNodes.insert(node); -} - -template -void HNSWIndex::setUnreachablePermNode(const graphNodeType &node) { - if (isMarkedDeleted(node.first)) - return; - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu still unreachable in " - "level %zu after trying to reinsert - setting as permanent unreachable", - node.first, node.second); - std::unique_lock lock(unreachableNodesGuard); - unreachableNodesPermanent.insert(node); + if (force) { + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu still unreachable in " + "level %zu after trying to reinsert - setting as permanent unreachable", + node.first, node.second); + hardUnreachableNodes.insert(node); + } else { + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Element %zu is unreachable in level %zu", + node.first, node.second); + unreachableNodes.insert(node); + } } template -void HNSWIndex::connectUnreachableNodes(bool permanent) { - auto nodes_to_connect = fetchAndClearUnreachableNodes(permanent); +template +void HNSWIndex::connectUnreachableNodes() { + auto nodes_to_connect = fetchAndClearUnreachableNodes(); if (nodes_to_connect.empty()) return; - this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Try to reinsert %zu nodes to the graph", - nodes_to_connect.size()); + this->log(VecSimCommonStrings::LOG_VERBOSE_STRING, "Try to reinsert %zu nodes to the graph %s", + nodes_to_connect.size(), force ? "- hard unreachable nodes" : ""); for (auto node : nodes_to_connect) { - reinsertElementToGraphAtLevel(node.first, node.second); + reinsertElementToGraphAtLevel(node.first, node.second); } } @@ -939,6 +941,7 @@ void HNSWIndex::revisitNeighborConnections( } template +template size_t HNSWIndex::mutuallyReconnectElement( idType node_id, candidatesMaxHeap &top_candidates, size_t level) { @@ -956,7 +959,7 @@ size_t HNSWIndex::mutuallyReconnectElement( selected_neighbors_cands.insert(selected_neighbors_cands.end(), top_candidates.begin(), top_candidates.end()); - float alpha = 1.5f; // we relax the heuristcs to get more neighbors + float alpha = force ? 1.5f : 1.2f; // we relax the heuristcs to get more neighbors getNeighborsByHeuristic2(selected_neighbors_cands, M, alpha); assert(selected_neighbors_cands.size() <= M && "Should not be more than M candidates returned by the heuristic"); @@ -1391,9 +1394,9 @@ void HNSWIndex::SwapLastIdWithDeletedId(idType element_inter this->unreachableNodes.erase({curElementCount, l}); this->unreachableNodes.insert(std::make_pair(element_internal_id, l)); } - if (this->unreachableNodesPermanent.contains({curElementCount, l})) { - this->unreachableNodesPermanent.erase({curElementCount, l}); - this->unreachableNodesPermanent.insert(std::make_pair(element_internal_id, l)); + if (this->hardUnreachableNodes.contains({curElementCount, l})) { + this->hardUnreachableNodes.erase({curElementCount, l}); + this->hardUnreachableNodes.insert(std::make_pair(element_internal_id, l)); } } @@ -1795,12 +1798,13 @@ void HNSWIndex::insertElementToGraph(idType element_id, curr_element = mutuallyConnectNewElement(element_id, top_candidates, level); } else { // Node still has no neighbors - it is defintly unreachable - this->setUnreachablePermNode(std::make_pair(element_id, level)); + this->setUnreachableNode(std::make_pair(element_id, level)); } } } template +template void HNSWIndex::reinsertElementToGraphAtLevel(idType element_id, size_t level_to_insert) { @@ -1841,10 +1845,10 @@ void HNSWIndex::reinsertElementToGraphAtLevel(idType element // No candidates found (entry point been deleted in the meantime). this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); } else { - size_t ret = mutuallyReconnectElement(element_id, top_candidates, level_to_insert); + size_t ret = mutuallyReconnectElement(element_id, top_candidates, level_to_insert); if (ret == 0) { // Node is still unreachable with 0 inDegree. - this->setUnreachablePermNode(std::make_pair(element_id, level_to_insert)); + this->setUnreachableNode(std::make_pair(element_id, level_to_insert)); } } this->unmarkAs(element_id); @@ -1870,7 +1874,7 @@ HNSWIndex::HNSWIndex(const HNSWParams *params, size_t random_seed, size_t pool_initial_size) : VecSimIndexAbstract(abstractInitParams), VecSimIndexTombstone(), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - unreachableNodes(this->allocator), unreachableNodesPermanent(this->allocator), + unreachableNodes(this->allocator), hardUnreachableNodes(this->allocator), vectorBlocks(this->allocator), graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(pool_initial_size, maxElements, this->allocator) { @@ -1950,7 +1954,7 @@ void HNSWIndex::removeAndSwap(idType internalId) { // Remove it from the unreachable nodes container for every level in which it exists. for (size_t l = 0; l <= element->toplevel; l++) { this->unreachableNodes.erase({internalId, l}); - this->unreachableNodesPermanent.erase({internalId, l}); + this->hardUnreachableNodes.erase({internalId, l}); } // Free the element's resources diff --git a/src/VecSim/algorithms/hnsw/hnsw_serializer.h b/src/VecSim/algorithms/hnsw/hnsw_serializer.h index c4d15ebb3..64eb9eb8b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_serializer.h +++ b/src/VecSim/algorithms/hnsw/hnsw_serializer.h @@ -6,7 +6,7 @@ HNSWIndex::HNSWIndex(std::ifstream &input, const HNSWParams Serializer::EncodingVersion version) : VecSimIndexAbstract(abstractInitParams), Serializer(version), maxElements(RoundUpInitialCapacity(params->initialCapacity, this->blockSize)), - epsilon(params->epsilon), unreachableNodes(this->allocator), unreachableNodesPermanent(this->allocator), + epsilon(params->epsilon), unreachableNodes(this->allocator), hardUnreachableNodes(this->allocator), vectorBlocks(this->allocator), graphDataBlocks(this->allocator), idToMetaData(maxElements, this->allocator), visitedNodesHandlerPool(1, maxElements, this->allocator) { diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index fad3ed7ba..b9076dde5 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -209,10 +209,8 @@ class TieredHNSWIndex : public VecSimTieredIndex { "running asynchronous GC for tiered HNSW index"); this->executeReadySwapJobs(this->pendingSwapJobsThreshold); // Try to reinsert permanent unreachable nodes - TIERED_LOG(VecSimCommonStrings::LOG_NOTICE_STRING, - "Goiong over permanent unreachable nodes:"); this->mainIndexGuard.lock_shared(); - this->getHNSWIndex()->connectUnreachableNodes(true); + this->getHNSWIndex()->template connectUnreachableNodes(); this->mainIndexGuard.unlock_shared(); } void acquireSharedLocks() override { @@ -433,7 +431,7 @@ void TieredHNSWIndex::insertVectorToHNSW( // Try to reinsert permanent unreachable nodes TIERED_LOG(VecSimCommonStrings::LOG_NOTICE_STRING, "Goiong over permanent unreachable nodes:"); - hnsw_index->connectUnreachableNodes(true); + hnsw_index->template connectUnreachableNodes(); // Release the inner HNSW data lock before we re-acquire the global HNSW lock. this->mainIndexGuard.unlock_shared(); this->mainIndexGuard.lock(); From 15b724e3146efb9cb5632b5538fb935a2fdc110c Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Tue, 23 Jul 2024 10:07:54 +0300 Subject: [PATCH 22/24] use verbose log --- src/VecSim/algorithms/hnsw/hnsw_tiered.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw_tiered.h b/src/VecSim/algorithms/hnsw/hnsw_tiered.h index b9076dde5..b44788e61 100644 --- a/src/VecSim/algorithms/hnsw/hnsw_tiered.h +++ b/src/VecSim/algorithms/hnsw/hnsw_tiered.h @@ -429,8 +429,8 @@ void TieredHNSWIndex::insertVectorToHNSW( if (hnsw_index->indexCapacity() == hnsw_index->indexSize()) { hnsw_index->unlockIndexDataGuard(); // Try to reinsert permanent unreachable nodes - TIERED_LOG(VecSimCommonStrings::LOG_NOTICE_STRING, - "Goiong over permanent unreachable nodes:"); + TIERED_LOG(VecSimCommonStrings::LOG_VERBOSE_STRING, + "Going over permanent unreachable nodes:"); hnsw_index->template connectUnreachableNodes(); // Release the inner HNSW data lock before we re-acquire the global HNSW lock. this->mainIndexGuard.unlock_shared(); From 631f771e5b967ea5b7cac280065642f8a5f9b2a4 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Sat, 27 Jul 2024 11:25:26 +0300 Subject: [PATCH 23/24] lock both locks in info to have a coherent picture + fix bug - do not add neighbors to deleted node, and remove the redundant in_repair --- src/VecSim/algorithms/hnsw/hnsw.h | 10 ++++------ src/VecSim/vec_sim_tiered_index.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index e061a5bbe..07cb34141 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -62,7 +62,6 @@ class hashForPair { typedef enum { DELETE_MARK = 0x1, // element is logically deleted, but still exists in the graph IN_PROCESS = 0x2, // element is being inserted/reinserted into the graph - IN_REPAIR = 0x4, // element is being repaired due to deletion of its neighbor(s) } Flags; // The state of the index and the newly inserted vector to be passed into addVector API in case that @@ -1023,12 +1022,14 @@ size_t HNSWIndex::mutuallyReconnectElement( // Go over the chosen new neighbors that are not connected yet and perform mutuall updates // if possible. for (idType chosen_id : selected_neighbors) { + if (isMarkedDeleted(node_id)) { + break; + } // If this specific new neighbor is deleted, we don't add this connection and continue. // Also, don't add a new node whose being indexed in parallel, as it may choose this node // as its neighbor and create a double connection (then this node will have a duplicate // neighbor). - if (isMarkedDeleted(chosen_id) || isInProcess(chosen_id) || - isMarkedAs(chosen_id)) { + if (isMarkedDeleted(chosen_id) || isInProcess(chosen_id)) { continue; } auto &chosen_node_level_data = getElementLevelData(chosen_id, level); @@ -1632,7 +1633,6 @@ void HNSWIndex::mutuallyUpdateForRepairedNode( template void HNSWIndex::repairNodeConnections(idType node_id, size_t level) { - this->markAs(node_id); vecsim_stl::vector neighbors_candidate_ids(this->allocator); // Use bitmaps for fast accesses: // node_orig_neighbours_set is used to differentiate between the neighbors that will *not* be @@ -1664,7 +1664,6 @@ void HNSWIndex::repairNodeConnections(idType node_id, size_t // If there are not deleted neighbors at that point the repair job has already been made by // another parallel job, and there is no need to repair the node anymore. if (deleted_neighbors.empty()) { - this->unmarkAs(node_id); return; } @@ -1736,7 +1735,6 @@ void HNSWIndex::repairNodeConnections(idType node_id, size_t // locks. mutuallyUpdateForRepairedNode(node_id, level, neighbors_to_remove, nodes_to_update, chosen_neighbors, max_M_cur); - this->unmarkAs(node_id); } template diff --git a/src/VecSim/vec_sim_tiered_index.h b/src/VecSim/vec_sim_tiered_index.h index 66e6230b3..01ffd3b43 100644 --- a/src/VecSim/vec_sim_tiered_index.h +++ b/src/VecSim/vec_sim_tiered_index.h @@ -271,10 +271,10 @@ template VecSimIndexInfo VecSimTieredIndex::info() const { VecSimIndexInfo info; this->flatIndexGuard.lock_shared(); + this->mainIndexGuard.lock(); VecSimIndexInfo frontendInfo = this->frontendIndex->info(); this->flatIndexGuard.unlock_shared(); - this->mainIndexGuard.lock(); VecSimIndexInfo backendInfo = this->backendIndex->info(); this->mainIndexGuard.unlock(); From a07a876f057da946cf006c782a21d6c93980e0d4 Mon Sep 17 00:00:00 2001 From: alon-reshef Date: Sat, 27 Jul 2024 11:27:09 +0300 Subject: [PATCH 24/24] . --- src/VecSim/algorithms/hnsw/hnsw.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 07cb34141..f48727181 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -1023,6 +1023,7 @@ size_t HNSWIndex::mutuallyReconnectElement( // if possible. for (idType chosen_id : selected_neighbors) { if (isMarkedDeleted(node_id)) { + // don't add neighbors from/to a deleted node break; } // If this specific new neighbor is deleted, we don't add this connection and continue.