diff --git a/conanfile.py b/conanfile.py index 1e7364867..70abb5980 100644 --- a/conanfile.py +++ b/conanfile.py @@ -5,7 +5,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "4.5.5" + version = "4.5.6" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" diff --git a/src/include/homestore/btree/btree.ipp b/src/include/homestore/btree/btree.ipp index a3bd1ee02..5ebe3e235 100644 --- a/src/include/homestore/btree/btree.ipp +++ b/src/include/homestore/btree/btree.ipp @@ -207,7 +207,7 @@ retry: m_btree_lock.unlock_shared(); ret = check_collapse_root(req); - if (ret != btree_status_t::success) { + if (ret != btree_status_t::success && ret != btree_status_t::merge_not_required) { LOGERROR("check collapse read failed btree name {}", m_bt_cfg.name()); goto out; } diff --git a/src/include/homestore/btree/detail/btree_mutate_impl.ipp b/src/include/homestore/btree/detail/btree_mutate_impl.ipp index 3eee5b75e..21d6752f3 100644 --- a/src/include/homestore/btree/detail/btree_mutate_impl.ipp +++ b/src/include/homestore/btree/detail/btree_mutate_impl.ipp @@ -523,7 +523,7 @@ btree_status_t Btree< K, V >::split_node(const BtreeNodePtr& parent_node, const } BT_NODE_LOG(TRACE, parent_node, "Available space for split entry={}", parent_node->available_size(m_bt_cfg)); -// child_node1->inc_link_version(); + // child_node1->inc_link_version(); // Update the existing parent node entry to point to second child ptr. parent_node->update(parent_ind, child_node2->link_info()); @@ -552,6 +552,7 @@ btree_status_t Btree< K, V >::split_node(const BtreeNodePtr& parent_node, const template < typename K, typename V > template < typename ReqT > bool Btree< K, V >::is_split_needed(const BtreeNodePtr& node, const BtreeConfig& cfg, ReqT& req) const { + if (node->total_entries() <= 1) { return false; } if (bt_thread_vars()->force_split_node && (bt_thread_vars()->force_split_node == node)) { bt_thread_vars()->force_split_node = nullptr; return true; diff --git a/src/include/homestore/btree/detail/btree_remove_impl.ipp b/src/include/homestore/btree/detail/btree_remove_impl.ipp index 60b983562..fb5471e92 100644 --- a/src/include/homestore/btree/detail/btree_remove_impl.ipp +++ b/src/include/homestore/btree/detail/btree_remove_impl.ipp @@ -288,6 +288,7 @@ bool Btree< K, V >::remove_extents_in_leaf(const BtreeNodePtr& node, BtreeRangeR template < typename K, typename V > template < typename ReqT > btree_status_t Btree< K, V >::check_collapse_root(ReqT& req) { + if (!m_bt_cfg.m_merge_turned_on) { return btree_status_t::merge_not_required; } BtreeNodePtr child; BtreeNodePtr root; btree_status_t ret = btree_status_t::success; @@ -355,7 +356,7 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const for (auto indx = start_idx + 1; indx <= end_idx; ++indx) { if (indx == parent_node->total_entries()) { BT_NODE_LOG_ASSERT(parent_node->has_valid_edge(), parent_node, - "Assertion failure, expected valid edge for parent_node: {}"); + "Assertion failure, expected valid edge for parent_node"); } BtreeLinkInfo child_info; @@ -463,28 +464,25 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const if (!K::is_fixed_size()) { // 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 lesser or equal size - int64_t post_merge_size{0}; - auto& old_node = old_nodes[leftmost_src.ith_nodes.back()]; - if (old_node->total_entries()) { - post_merge_size += old_node->get_nth_obj_size( - std::min(leftmost_src.last_node_upto, old_node->total_entries() - 1)); // New leftmost entry - } - post_merge_size -= parent_node->get_nth_obj_size(start_idx); // Previous left entry - - for (auto& node : new_nodes) { - if (node->total_entries()) { post_merge_size += node->get_nth_obj_size(node->total_entries() - 1); } - } - - for (auto& node : old_nodes) { - if (node->total_entries()) { post_merge_size -= node->get_nth_obj_size(node->total_entries() - 1); } - } - - if (post_merge_size > parent_node->available_size(m_bt_cfg)) { + // fixed length. For fixed length node merge will always result in less or equal size + + // 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()); + + // aside from releasing size due to excess node, K::get_estimate_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 + // are the size of the new nodes (the last key of the last new node is not getting updated; hence excluded) plus + // the leftmost node. + if (parent_node->available_size(m_bt_cfg) + minimum_releasing_excess_size < + (1 + new_nodes.size() ? new_nodes.size() - 1 : 0) * K::get_estimate_max_size()) { BT_NODE_LOG(DEBUG, parent_node, - "Merge is needed, however after merge it will add {} bytes which is more than " - "available_size={}, so not proceeding with merge", - post_merge_size, parent_node->available_size(m_bt_cfg)); + "Merge is needed, however after merge, the parent MAY not have enough space to accommodate the " + "new keys, so not proceeding with merge"); ret = btree_status_t::merge_not_required; goto out; } @@ -590,15 +588,18 @@ btree_status_t Btree< K, V >::merge_nodes(const BtreeNodePtr& parent_node, const out: // Do free/unlock based on success/failure in reverse order if (ret == btree_status_t::success) { - for (auto& node : old_nodes) { - free_node(node, locktype_t::WRITE, context); + for (auto it = old_nodes.rbegin(); it != old_nodes.rend(); ++it) { + BT_NODE_LOG(DEBUG, (*it).get(), "Freeing this node as part of successful merge"); + free_node(*it, locktype_t::WRITE, context); } } else { - for (auto& node : old_nodes) { - unlock_node(node, locktype_t::WRITE); + for (auto it = old_nodes.rbegin(); it != old_nodes.rend(); ++it) { + BT_NODE_LOG(DEBUG, (*it).get(), "Unlocking this node as part of unsuccessful merge"); + unlock_node(*it, locktype_t::WRITE); } - for (auto& node : new_nodes) { - free_node(node, locktype_t::NONE, context); + for (auto it = new_nodes.rbegin(); it != new_nodes.rend(); ++it) { + BT_NODE_LOG(DEBUG, (*it).get(), "Freeing this new node as part of unsuccessful merge"); + free_node(*it, locktype_t::NONE, context); } } return ret; diff --git a/src/include/homestore/btree/detail/simple_node.hpp b/src/include/homestore/btree/detail/simple_node.hpp index 8dbbc62a6..479a8245d 100644 --- a/src/include/homestore/btree/detail/simple_node.hpp +++ b/src/include/homestore/btree/detail/simple_node.hpp @@ -147,8 +147,7 @@ class SimpleNode : public BtreeNode { } uint32_t num_entries_by_size(uint32_t start_idx, uint32_t size) const override { - uint32_t possible_entries = (size == 0) ? 0 : (size - 1) / get_nth_obj_size(0) + 1; - return std::min(possible_entries, this->total_entries() - start_idx); + return std::min(size / get_nth_obj_size(0), this->total_entries() - start_idx); } uint32_t copy_by_size(const BtreeConfig& cfg, const BtreeNode& o, uint32_t start_idx, uint32_t size) override { diff --git a/src/include/homestore/btree/detail/varlen_node.hpp b/src/include/homestore/btree/detail/varlen_node.hpp index 9c89a89c0..ee253f577 100644 --- a/src/include/homestore/btree/detail/varlen_node.hpp +++ b/src/include/homestore/btree/detail/varlen_node.hpp @@ -166,7 +166,7 @@ class VariableNode : public BtreeNode { get_nth_value(ind_s - 1, &last_1_val, false); this->set_edge_value(last_1_val); - for (uint32_t i = ind_s; i < total_entries; i++) { + for (uint32_t i = ind_s - 1; i < total_entries; i++) { get_var_node_header()->m_available_space += get_nth_key_len(i) + get_nth_value_len(i) + recSize; } this->sub_entries(total_entries - ind_s + 1); @@ -270,15 +270,15 @@ class VariableNode : public BtreeNode { vb.bytes = kb.bytes + kb.size; vb.size = get_nth_value_len(ind); - auto sz = other.insert(0, kb, vb); // Keep on inserting on the first index, thus moving everything to right - if (!sz) break; - - --ind; - ++nmoved; if ((kb.size + vb.size + this->get_record_size()) > size_to_move) { // We reached threshold of how much we could move break; } + + auto sz = other.insert(0, kb, vb); // Keep on inserting on the first index, thus moving everything to right + + --ind; + ++nmoved; size_to_move -= sz; } remove(ind + 1, this->total_entries() - 1); @@ -329,6 +329,7 @@ class VariableNode : public BtreeNode { if (sz == 0) { break; } ++n; ++idx; + copy_size -= sz; } this->set_gen(this_gen + 1); diff --git a/src/tests/btree_test_kvs.hpp b/src/tests/btree_test_kvs.hpp index 21c283bc6..3822f8ec4 100644 --- a/src/tests/btree_test_kvs.hpp +++ b/src/tests/btree_test_kvs.hpp @@ -21,8 +21,8 @@ #include #include -static constexpr uint32_t g_max_keysize{120}; -static constexpr uint32_t g_max_valsize{120}; +static constexpr uint32_t g_max_keysize{100}; // for node size = 512 : free space : 442 => 100+100+6(record size) = 46% +static constexpr uint32_t g_max_valsize{100}; static std::random_device g_rd{}; static std::default_random_engine g_re{g_rd()}; static std::normal_distribution<> g_randkeysize_generator{32, 24}; @@ -49,10 +49,10 @@ static std::string gen_random_string(size_t len, uint32_t preamble = std::numeri static thread_local std::random_device rd{}; static thread_local std::default_random_engine re{rd()}; std::uniform_int_distribution< size_t > rand_char{0, alphanum.size() - 1}; - for (size_t i{0}; i < len; ++i) { + if (len < str.size()) { len = str.size(); } + for (size_t i{0}; i < len - str.size(); ++i) { str += alphanum[rand_char(re)]; } - str += '\0'; return str; } diff --git a/src/tests/test_mem_btree.cpp b/src/tests/test_mem_btree.cpp index 979cdc498..495cb8fd3 100644 --- a/src/tests/test_mem_btree.cpp +++ b/src/tests/test_mem_btree.cpp @@ -556,9 +556,7 @@ class BtreeConcurrentTest : public testing::Test { }); preload(SISL_OPTIONS["preload_size"].as< uint32_t >()); - print_keys(); runInParallel(op_list); - print_keys(); } private: