From ece0aaabc332e2262e7e68c10a3564b22c58e07b Mon Sep 17 00:00:00 2001 From: tzaeschke Date: Tue, 13 Dec 2022 12:43:43 +0100 Subject: [PATCH 1/4] squash 15 --- include/phtree/common/flat_sparse_map.h | 16 ++-- include/phtree/v16/node.h | 7 +- include/phtree/v16/phtree_v16.h | 105 ++---------------------- 3 files changed, 22 insertions(+), 106 deletions(-) diff --git a/include/phtree/common/flat_sparse_map.h b/include/phtree/common/flat_sparse_map.h index 70cf2fba..549792b3 100644 --- a/include/phtree/common/flat_sparse_map.h +++ b/include/phtree/common/flat_sparse_map.h @@ -96,13 +96,20 @@ class sparse_map { } template - auto emplace(Args&&... args) { - return try_emplace_base(std::forward(args)...); + auto emplace(size_t key, Args&&... args) { + auto iter = lower_bound(key); + return try_emplace_base(iter, key, std::forward(args)...); } template auto try_emplace(size_t key, Args&&... args) { - return try_emplace_base(key, std::forward(args)...); + auto iter = lower_bound(key); + return try_emplace_base(iter, key, std::forward(args)...); + } + + template + auto try_emplace(iterator iter, size_t key, Args&&... args) { + return try_emplace_base(iter, key, std::forward(args)...); } template @@ -137,8 +144,7 @@ class sparse_map { } template - auto try_emplace_base(KeyT key, Args&&... args) { - auto it = lower_bound(key); + auto try_emplace_base(const iterator& it, KeyT key, Args&&... args) { if (it != end() && it->first == key) { return std::make_pair(it, false); } else { diff --git a/include/phtree/v16/node.h b/include/phtree/v16/node.h index 7d2c5fc1..57824226 100644 --- a/include/phtree/v16/node.h +++ b/include/phtree/v16/node.h @@ -47,11 +47,9 @@ using EntryMap = typename std::conditional_t< DIM <= 8, sparse_map, Entry>, b_plus_tree_map>>; -//template -//using EntryMap = std::map, Entry>; template -using EntryIterator = typename std::remove_const().begin())>::type; +using EntryIterator = typename std::remove_const_t().begin())>; template using EntryIteratorC = decltype(EntryMap().cbegin()); @@ -167,8 +165,7 @@ class Node { return const_cast(*this).Find(key, postfix_len); } - // TODO rename to lower_bound() - auto FindIter(const KeyT& key, bit_width_t postfix_len, bool& found) { + auto LowerBound(const KeyT& key, bit_width_t postfix_len, bool& found) { hc_pos_t hc_pos = CalcPosInArray(key, postfix_len); auto iter = entries_.lower_bound(hc_pos); found = diff --git a/include/phtree/v16/phtree_v16.h b/include/phtree/v16/phtree_v16.h index b06776e3..53d019ca 100644 --- a/include/phtree/v16/phtree_v16.h +++ b/include/phtree/v16/phtree_v16.h @@ -278,7 +278,7 @@ class PhTreeV16 { * whose second element is a bool that is true if the value was actually relocated. */ template - size_t relocate_if2(const KeyT& old_key, const KeyT& new_key, PRED pred) { + [[deprecated]] size_t relocate_if2(const KeyT& old_key, const KeyT& new_key, PRED pred) { auto pair = _find_two(old_key, new_key); auto& iter_old = pair.first; auto& iter_new = pair.second; @@ -315,8 +315,6 @@ class PhTreeV16 { return 1; } - // TODO is this a memory leak problem????? - /* * Relocate (move) an entry from one position to another, subject to a predicate. * @@ -327,77 +325,6 @@ class PhTreeV16 { * @return A pair, whose first element points to the possibly relocated value, and * whose second element is a bool that is true if the value was actually relocated. */ - // TODO: test work with old relocate_if(). It also work with std::map - // TODO test also FAILS with B-Plus_tree_map; but not with array_map! - // WITHOUT ITERATOR - template - auto relocate_ifX(const KeyT& old_key, const KeyT& new_key, PRED&& pred) { - bit_width_t n_diverging_bits = NumberOfDivergingBits(old_key, new_key); - - EntryT* current_entry = &root_; // An entry. - EntryT* old_node_entry = nullptr; // Node that contains entry to be removed - EntryT* old_node_entry_parent = nullptr; // Parent of the old_node_entry - EntryT* new_node_entry = nullptr; // Node that will contain new entry - // Find node for removal - while (current_entry && current_entry->IsNode()) { - old_node_entry_parent = old_node_entry; - old_node_entry = current_entry; - auto postfix_len = old_node_entry->GetNodePostfixLen(); - if (postfix_len + 1 >= n_diverging_bits) { - new_node_entry = old_node_entry; - } - // TODO stop earlier, we are going to have to redo this after insert.... - current_entry = current_entry->GetNode().Find(old_key, postfix_len); - } - EntryT* old_entry = current_entry; // Entry to be removed - - // Can we stop already? - if (old_entry == nullptr || !pred(old_entry->GetValue())) { - return 0; // old_key not found! - } - - // Are the keys equal? Or is the quadrant the same? -> same entry - if (n_diverging_bits == 0 || old_node_entry->GetNodePostfixLen() >= n_diverging_bits) { - old_entry->SetKey(new_key); - return 1; - } - - // Find node for insertion - auto new_entry = new_node_entry; - while (new_entry && new_entry->IsNode()) { - new_node_entry = new_entry; - new_entry = new_entry->GetNode().Find(new_key, new_entry->GetNodePostfixLen()); - } - if (new_entry != nullptr) { - return 0; // Entry exists! - } - bool is_inserted = false; - // TODO remove "if" - if (new_entry == nullptr) { // TODO use in-node pointer - new_entry = &new_node_entry->GetNode().Emplace( - is_inserted, - new_key, - new_node_entry->GetNodePostfixLen(), - std::move(old_entry->ExtractValue())); - } - - // Erase old value. See comments in try_emplace(iterator) for details. - if (old_node_entry_parent == new_node_entry) { - // In this case the old_node_entry may have been invalidated by the previous - // insertion. - old_node_entry = old_node_entry_parent; - } - - bool found = false; - while (old_node_entry) { - old_node_entry = old_node_entry->GetNode().Erase( - old_key, old_node_entry, old_node_entry != &root_, found); - } - assert(found); - return 1; - } - - // WITH ITERATOR template auto relocate_if(const KeyT& old_key, const KeyT& new_key, PRED&& pred) { bit_width_t n_diverging_bits = NumberOfDivergingBits(old_key, new_key); @@ -418,7 +345,7 @@ class PhTreeV16 { } // TODO stop earlier, we are going to have to redo this after insert.... bool is_found = false; - iter = current_entry->GetNode().FindIter(old_key, postfix_len, is_found); + iter = current_entry->GetNode().LowerBound(old_key, postfix_len, is_found); current_entry = is_found ? &iter->second : nullptr; } EntryT* old_entry = current_entry; // Entry to be removed @@ -439,15 +366,13 @@ class PhTreeV16 { bool is_found = false; while (new_entry && new_entry->IsNode()) { new_node_entry = new_entry; - iter = new_entry->GetNode().FindIter(new_key, new_entry->GetNodePostfixLen(), is_found); + iter = + new_entry->GetNode().LowerBound(new_key, new_entry->GetNodePostfixLen(), is_found); new_entry = is_found ? &iter->second : nullptr; } if (is_found) { return 0; // Entry exists } - if (new_entry != nullptr) { - return 0; // Entry exists! - } bool is_inserted = false; new_entry = &new_node_entry->GetNode().Emplace( iter, @@ -463,13 +388,13 @@ class PhTreeV16 { old_node_entry = old_node_entry_parent; } - bool found = false; - // TODO use in-node pointer + is_found = false; + // TODO use in-node iterator if possible while (old_node_entry) { old_node_entry = old_node_entry->GetNode().Erase( - old_key, old_node_entry, old_node_entry != &root_, found); + old_key, old_node_entry, old_node_entry != &root_, is_found); } - assert(found); + assert(is_found); return 1; } @@ -525,18 +450,6 @@ class PhTreeV16 { return std::make_pair(iter1, iter2); } - // TODO what is different/required for MM: - // - "old" is not always removed - // - "new" may exist, but may not result in collision - // TODO i.e. we need three conditions: - // - pred_move ( {return is_valid(old); } ) - // - pred_can_be_moved ( { return destination.emplace() == true; } ) - // - pred_remove_old ( { source.erase(); return source.empty(); } ) - - // TODO - // - relocate(key, key, value) relocates 0 or 1 entries...? - // - relocate_if(key, key) relocates potentially many keys - public: /* * Tries to locate two entries that are 'close' to each other. @@ -555,7 +468,7 @@ class PhTreeV16 { bit_width_t n_diverging_bits = NumberOfDivergingBits(old_key, new_key); if (!verify_exists && n_diverging_bits == 0) { - return 1; // TODO COUNT()? + return 1; // We omit calling because that would require looking up the entry... } EntryT* new_entry = &root_; // An entry. From fc19f8f781e1a7542bbc1a4434c5a1c706703578 Mon Sep 17 00:00:00 2001 From: tzaeschke Date: Tue, 13 Dec 2022 12:48:02 +0100 Subject: [PATCH 2/4] clean up --- include/phtree/common/flat_sparse_map.h | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/include/phtree/common/flat_sparse_map.h b/include/phtree/common/flat_sparse_map.h index 549792b3..fad7a47f 100644 --- a/include/phtree/common/flat_sparse_map.h +++ b/include/phtree/common/flat_sparse_map.h @@ -112,11 +112,6 @@ class sparse_map { return try_emplace_base(iter, key, std::forward(args)...); } - template - auto try_emplace(iterator iter, size_t key, Args&&... args) { - return try_emplace_base(iter, key, std::forward(args)...); - } - void erase(KeyT key) { auto it = lower_bound(key); if (it != end() && it->first == key) { @@ -157,21 +152,6 @@ class sparse_map { } } - // TODO merge with above - template - auto try_emplace_base(const iterator& it, KeyT key, Args&&... args) { - if (it != end() && it->first == key) { - return std::make_pair(it, false); - } else { - auto x = data_.emplace( - it, - std::piecewise_construct, - std::forward_as_tuple(key), - std::forward_as_tuple(std::forward(args)...)); - return std::make_pair(x, true); - } - } - std::vector data_; }; From b7dedfbaa9876861e656854d3e49e27f34432dfe Mon Sep 17 00:00:00 2001 From: tzaeschke Date: Tue, 13 Dec 2022 12:50:31 +0100 Subject: [PATCH 3/4] clean up --- benchmark/update_mm_d_benchmark.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/benchmark/update_mm_d_benchmark.cc b/benchmark/update_mm_d_benchmark.cc index 5b4ec38e..6c5cfa57 100644 --- a/benchmark/update_mm_d_benchmark.cc +++ b/benchmark/update_mm_d_benchmark.cc @@ -166,7 +166,7 @@ typename std::enable_if< UpdateEntry(TestMap& tree, std::vector>& updates) { size_t n = 0; for (auto& update : updates) { - n += tree.relocate(update.old_, update.new_, update.id_, false); + n += tree.relocate(update.old_, update.new_, update.id_); } return n; } @@ -177,10 +177,7 @@ typename std::enable_if::type size_t n = 0; for (auto& update : updates) { n += tree.relocate_if( - update.old_, - update.new_, - [&update](const payload_t& v) { return v == update.id_; }, - false); + update.old_, update.new_, [&update](const payload_t& v) { return v == update.id_; }); } return n; } From b4b4570d4cfed670db401e1c7474521e1102f4ae Mon Sep 17 00:00:00 2001 From: tzaeschke Date: Tue, 13 Dec 2022 12:53:57 +0100 Subject: [PATCH 4/4] clean up --- CHANGELOG.md | 7 +++++-- include/phtree/common/common.h | 35 --------------------------------- include/phtree/v16/entry.h | 5 ----- include/phtree/v16/phtree_v16.h | 21 -------------------- test/phtree_test.cc | 1 - 5 files changed, 5 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d40b3f2..218b4149 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Added B+tree multimap for internal (future) use. [#93](https://github.com/tzaeschke/phtree-cpp/issues/93) ### Changed -- Rewrote relocate(). This should be much cleaner now and slightly faster. [#98](https://github.com/tzaeschke/phtree-cpp/pull/98) +- Rewrote relocate(). This should be much cleaner now and slightly faster. + [#98](https://github.com/tzaeschke/phtree-cpp/pull/98), [#99](https://github.com/tzaeschke/phtree-cpp/pull/99) + - Cleaned up HandleCollision() and key comparison functions. [#97](https://github.com/tzaeschke/phtree-cpp/pull/97) - Improved performance by eliminating memory indirection for DIM > 3. This was enabled by referencing "Node" directly in "Entry" which was enabled by @@ -18,7 +20,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Improved performance of window queries by executing them partially as point queries. This works best for point datasets, and somewhat for box datasets with "include" queries. There is no benefit for "intersection" queries. [#88](https://github.com/tzaeschke/phtree-cpp/issues/88) -- Improved benchmarks for insert and query to use a more compact format. [#91](https://github.com/tzaeschke/phtree-cpp/pull/91) +- Improved benchmarks for insert and query to use a more compact format. + [#91](https://github.com/tzaeschke/phtree-cpp/pull/91) - Improved performance of window queries by optimizing calculation of min/max masks. Improved performance of queries and updates by changing bit-width of min/max masks and hc_pos_t. [#95](https://github.com/tzaeschke/phtree-cpp/pull/95) diff --git a/include/phtree/common/common.h b/include/phtree/common/common.h index 0158b169..152881b1 100644 --- a/include/phtree/common/common.h +++ b/include/phtree/common/common.h @@ -103,21 +103,6 @@ static bit_width_t NumberOfDivergingBits( return MAX_BIT_WIDTH - CountLeadingZeros(diff2); } -//template -//static bit_width_t NumberOfDivergingBits2( -// const PhPoint& v1, const PhPoint& v2) { -// // write all differences to diff, we just check diff afterwards -// SCALAR diff = 0; -// //bit_mask_t diff = 0; -// for (dimension_t i = 0; i < DIM; ++i) { -// diff |= v1[i] ^ v2[i]; -// } -// bit_mask_t diff2 = reinterpret_cast&>(diff); -// assert(CountLeadingZeros(diff2) <= MAX_BIT_WIDTH); -// return MAX_BIT_WIDTH - CountLeadingZeros(diff2); -//} - - template static bool KeyEquals( const PhPoint& key_a, const PhPoint& key_b, bit_width_t ignore_bits) { @@ -127,26 +112,6 @@ static bool KeyEquals( } return diff >> ignore_bits == 0; } -//template -//static bool KeyEquals0( -// const PhPoint& key_a, const PhPoint& key_b, SCALAR mask) { -// for (dimension_t i = 0; i < DIM; ++i) { -// if (((key_a[i] ^ key_b[i]) & mask) != 0) { -// return false; -// } -// } -// return true; -//} -// -//template -//static bool KeyEquals1( -// const PhPoint& key_a, const PhPoint& key_b, SCALAR mask) { -// SCALAR sum = 0; -// for (dimension_t i = 0; i < DIM; ++i) { -// sum |= (key_a[i] ^ key_b[i]); -// } -// return (sum & mask) == 0; -//} // ************************************************************************ // String helpers diff --git a/include/phtree/v16/entry.h b/include/phtree/v16/entry.h index 2d09fc93..8ab8c488 100644 --- a/include/phtree/v16/entry.h +++ b/include/phtree/v16/entry.h @@ -177,11 +177,6 @@ class Entry { kd_key_ = key; } - void SetValue(T&& value) noexcept { - assert(union_type_ == VALUE); - value_ = std::move(value); - } - void SetNode(NodeT&& node, bit_width_t postfix_len) noexcept { postfix_len_ = static_cast(postfix_len); DestroyUnion(); diff --git a/include/phtree/v16/phtree_v16.h b/include/phtree/v16/phtree_v16.h index 53d019ca..dc7e951f 100644 --- a/include/phtree/v16/phtree_v16.h +++ b/include/phtree/v16/phtree_v16.h @@ -595,7 +595,6 @@ class PhTreeV16 { return std::make_pair(iter1, iter2); } - public: /* * Iterates over all entries in the tree. The optional filter allows filtering entries and nodes * (=sub-trees) before returning / traversing them. By default, all entries are returned. Filter @@ -775,26 +774,6 @@ class PhTreeV16 { return {parent, entry_iter}; } - std::pair> find_starting_node( - const KeyT& key1, const KeyT& key2, bit_width_t& max_conflicting_bits) { - auto& prefix = key1; - max_conflicting_bits = NumberOfDivergingBits(key1, key2); - EntryT* parent = &root_; - if (max_conflicting_bits > root_.GetNodePostfixLen()) { - // Abort early if we have no shared prefix in the query - return {&root_, root_.GetNode().Entries().end()}; - } - EntryIterator entry_iter = - root_.GetNode().FindPrefix(prefix, max_conflicting_bits, root_.GetNodePostfixLen()); - while (entry_iter != parent->GetNode().Entries().end() && entry_iter->second.IsNode() && - entry_iter->second.GetNodePostfixLen() >= max_conflicting_bits) { - parent = &entry_iter->second; - entry_iter = parent->GetNode().FindPrefix( - prefix, max_conflicting_bits, parent->GetNodePostfixLen()); - } - return {parent, entry_iter}; - } - size_t num_entries_; // Contract: root_ contains a Node with 0 or more entries. The root node is the only Node // that is allowed to have less than two entries. diff --git a/test/phtree_test.cc b/test/phtree_test.cc index 58ffc055..46b2a58d 100644 --- a/test/phtree_test.cc +++ b/test/phtree_test.cc @@ -668,7 +668,6 @@ TEST(PhTreeTest, TestUpdateWithRelocateIf) { TestPoint pNew{pOld[0] + delta, pOld[1] + delta, pOld[2] + delta}; if ((delta > 0.0 && tree.find(pNew) != tree.end()) || (i % 2 != 0)) { // Skip this, there is already another entry - std::cout << "x = " << x << " i=" << i << std::endl; ASSERT_EQ(0, tree.relocate_if(pOld, pNew, pred)); } else { ASSERT_EQ(1, tree.relocate_if(pOld, pNew, pred));