From 0d311e734ac2dfa15e4b8ae1fe915e7d9c73aa79 Mon Sep 17 00:00:00 2001 From: Mehdi Hosseini <116847813+shosseinimotlagh@users.noreply.github.com> Date: Thu, 7 Dec 2023 10:43:35 -0800 Subject: [PATCH] FIX remove (#239) --- conanfile.py | 2 +- .../btree/detail/btree_remove_impl.ipp | 20 +++++++++++++------ .../homestore/btree/detail/simple_node.hpp | 2 -- .../homestore/btree/detail/varlen_node.hpp | 6 ++++-- src/lib/common/resource_mgr.cpp | 2 +- src/tests/btree_helpers/btree_test_helper.hpp | 2 -- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/conanfile.py b/conanfile.py index 43ed205db..c50de1b4c 100644 --- a/conanfile.py +++ b/conanfile.py @@ -5,7 +5,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "4.9.1" + version = "4.9.2" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" diff --git a/src/include/homestore/btree/detail/btree_remove_impl.ipp b/src/include/homestore/btree/detail/btree_remove_impl.ipp index df0a22773..f138b0248 100644 --- a/src/include/homestore/btree/detail/btree_remove_impl.ipp +++ b/src/include/homestore/btree/detail/btree_remove_impl.ipp @@ -335,14 +335,23 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const auto& old_ith_node = old_nodes[src_cursor.ith_node]; auto const nentries = new_node->copy_by_size(m_bt_cfg, *old_ith_node, src_cursor.nth_entry, available_size); + total_size -= new_node->occupied_size(); if (old_ith_node->total_entries() == (src_cursor.nth_entry + nentries)) { // Copied entire node ++src_cursor.ith_node; src_cursor.nth_entry = 0; available_size = balanced_size - new_node->occupied_size(); } else { - src_cursor.nth_entry += nentries; - available_size = 0; + // If it is the last node supposed to be, check if the remaining entries can be copied and not creating a + // new nodes. This will make the last new node a little skewed from balanced size due to large key/values but + // avoid making extra new node. + if (new_nodes.size() == num_nodes - 1 && total_size < new_node->available_size()) { + available_size = new_node->available_size(); + src_cursor.nth_entry += nentries; + } else { + src_cursor.nth_entry += nentries; + available_size = 0; + } } } @@ -362,6 +371,7 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const } if (!K::is_fixed_size()) { +#if 0 // Lets see if we have enough room in parent node to accommodate changes. This is needed only if the key is not // fixed length. For fixed length node merge will always result in less or equal size auto excess_releasing_nodes = @@ -375,14 +385,13 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const goto out; } -#if 0 +#endif // we first calculate the least amount of space being released after removing excess children. the key size // cannot be taken account; so we know for sure that value (i.e., linkinfo) and also its record will be freed. // If the end_idx is the parent's edge, the space is not released eventually. auto excess_releasing_nodes = old_nodes.size() - new_nodes.size() - (parent_node->total_entries() == end_idx) ? 1 : 0; - auto minimum_releasing_excess_size = - excess_releasing_nodes * (BtreeLinkInfo::get_fixed_size() + parent_node->get_record_size()); + auto minimum_releasing_excess_size = excess_releasing_nodes * (BtreeLinkInfo::get_fixed_size()); // aside from releasing size due to excess node, K::get_max_size is needed for each updating element // at worst case (linkinfo and record remain the same for old and new nodes). The number of updating elements @@ -396,7 +405,6 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const ret = btree_status_t::merge_not_required; goto out; } -#endif } // Now it is time to commit things and at this point no going back, since in-place write nodes are modified diff --git a/src/include/homestore/btree/detail/simple_node.hpp b/src/include/homestore/btree/detail/simple_node.hpp index b8149374c..b9f29d7b9 100644 --- a/src/include/homestore/btree/detail/simple_node.hpp +++ b/src/include/homestore/btree/detail/simple_node.hpp @@ -229,11 +229,9 @@ class SimpleNode : public VariantNode< K, V > { std::string to_string_keys(bool print_friendly = false) const override { #if 0 std::string delimiter = print_friendly ? "\n" : "\t"; - auto str = fmt::format("{}{}.{} nEntries={} {} ", auto str = fmt::format("{}{}.{} nEntries={} {} ", print_friendly ? "------------------------------------------------------------\n" : "", this->node_id(), this->link_version(), this->total_entries(), (this->is_leaf() ? "LEAF" : "INTERIOR")); - this->node_id(), this->link_version(), this->total_entries(), (this->is_leaf() ? "LEAF" : "INTERIOR")); if (!this->is_leaf() && (this->has_valid_edge())) { fmt::format_to(std::back_inserter(str), "edge_id={}.{}", this->edge_info().m_bnodeid, this->edge_info().m_link_version); diff --git a/src/include/homestore/btree/detail/varlen_node.hpp b/src/include/homestore/btree/detail/varlen_node.hpp index de155f9c5..289dd8b7d 100644 --- a/src/include/homestore/btree/detail/varlen_node.hpp +++ b/src/include/homestore/btree/detail/varlen_node.hpp @@ -71,6 +71,10 @@ class VariableNode : public VariantNode< K, V > { virtual ~VariableNode() = default; + uint32_t occupied_size() const override { + return (get_var_node_header_const()->m_init_available_space - sizeof(var_node_header) - available_size()); + } + /* Insert the key and value in provided index * Assumption: Node lock is already taken */ btree_status_t insert(uint32_t ind, const BtreeKey& key, const BtreeValue& val) override { @@ -517,11 +521,9 @@ class VariableNode : public VariantNode< K, V > { std::string to_string_keys(bool print_friendly = false) const override { #if 0 std::string delimiter = print_friendly ? "\n" : "\t"; - auto str = fmt::format("{}{}.{} nEntries={} {} ", auto str = fmt::format("{}{}.{} nEntries={} {} ", print_friendly ? "------------------------------------------------------------\n" : "", this->node_id(), this->link_version(), this->total_entries(), (this->is_leaf() ? "LEAF" : "INTERIOR")); - this->node_id(), this->link_version(), this->total_entries(), (this->is_leaf() ? "LEAF" : "INTERIOR")); if (!this->is_leaf() && (this->has_valid_edge())) { fmt::format_to(std::back_inserter(str), "edge_id={}.{}", this->edge_info().m_bnodeid, this->edge_info().m_link_version); diff --git a/src/lib/common/resource_mgr.cpp b/src/lib/common/resource_mgr.cpp index 92160c364..4e87ad5be 100644 --- a/src/lib/common/resource_mgr.cpp +++ b/src/lib/common/resource_mgr.cpp @@ -36,7 +36,7 @@ void ResourceMgr::dec_dirty_buf_size(const uint32_t size) { HS_REL_ASSERT_GT(size, 0); const int64_t dirty_buf_cnt = m_hs_dirty_buf_cnt.fetch_sub(size, std::memory_order_relaxed); COUNTER_DECREMENT(m_metrics, dirty_buf_cnt, size); - HS_REL_ASSERT_GE(dirty_buf_cnt, size); + HS_REL_ASSERT_GE(dirty_buf_cnt, 0); } void ResourceMgr::register_dirty_buf_exceed_cb(exceed_limit_cb_t cb) { m_dirty_buf_exceed_cb = std::move(cb); } diff --git a/src/tests/btree_helpers/btree_test_helper.hpp b/src/tests/btree_helpers/btree_test_helper.hpp index 606aceba0..a5fba4132 100644 --- a/src/tests/btree_helpers/btree_test_helper.hpp +++ b/src/tests/btree_helpers/btree_test_helper.hpp @@ -294,9 +294,7 @@ struct BtreeTestHelper : public testing::Test { void multi_op_execute(const std::vector< std::pair< std::string, int > >& op_list) { preload(SISL_OPTIONS["preload_size"].as< uint32_t >()); - print_keys(); run_in_parallel(op_list); - print_keys(); } void print(const std::string& file = "") const { m_bt->print_tree(file); }