From 58c5319d03ef084a061b11abbc418ba816fdd302 Mon Sep 17 00:00:00 2001 From: Roland Conybeare Date: Thu, 8 Jan 2026 12:27:48 -0500 Subject: [PATCH] xo-arena: DArenaHashMap try_expand bugfix + edge utest --- xo-arena/include/xo/arena/DArenaHashMap.hpp | 158 +++++++++++++++----- xo-arena/include/xo/arena/DArenaVector.hpp | 14 ++ xo-arena/utest/DArenaHashMap.test.cpp | 47 +++++- xo-arena/utest/random_hash_ops.hpp | 68 +++++++++ 4 files changed, 248 insertions(+), 39 deletions(-) diff --git a/xo-arena/include/xo/arena/DArenaHashMap.hpp b/xo-arena/include/xo/arena/DArenaHashMap.hpp index 0c234631..92d0ed87 100644 --- a/xo-arena/include/xo/arena/DArenaHashMap.hpp +++ b/xo-arena/include/xo/arena/DArenaHashMap.hpp @@ -174,12 +174,55 @@ namespace xo { n_slot_{group_exp2.second * c_group_size}, control_{DArenaVector::map(ArenaConfig{.size_ = n_slot_ + c_group_size})}, slots_{DArenaVector::map(ArenaConfig{.size_ = n_slot_ * sizeof(value_type)})} - {} + { + /* here: arenas have allocated address range, but no committed memory yet */ + + this->_init(); + } size_type empty() const noexcept { return size_ == 0; } size_type capacity() const noexcept { return n_group_ * c_group_size; } float load_factor() const noexcept { return size_ / static_cast(n_slot_); } + void resize_from_empty(const std::pair & group_exp2) + { + assert(size_ == 0); + + this->n_group_exponent_ = group_exp2.first; + this->n_group_ = group_exp2.second; + this->n_slot_ = group_exp2.second * c_group_size; + + this->_init(); + } + + void clear() { + /* remark: discontinuity in the sense that we lose n_group_ = 2 ^ n_group_epxonent_ + * + * juice may not be worth the squeeze here, + * since DArena doesn't yet (Jan 2026) unmap on clear + */ + + this->size_ = 0; + this->n_group_exponent_ = 0; + this->n_group_ = 0; + this->n_slot_ = 0; + this->control_.resize(0); + this->slots_.resize(0); + } + + public: + void _init() { + this->control_.resize(n_slot_ + c_group_size); + + /* all slots marked empty initially */ + std::fill(this->control_.begin(), + this->control_.end(), + c_empty_slot); + + this->slots_.resize(n_slot_); + } + group_type _load_group(size_type ix) { return group_type(&(control_[ix])); } @@ -296,6 +339,8 @@ namespace xo { size_type capacity() const noexcept { return store_.capacity(); } float load_factor() const noexcept { return store_.load_factor(); } + bool verify_ok(verify_policy p = verify_policy::throw_only()) const; + iterator begin() { iterator ix(&(store_.control_[0]), &(store_.control_[store_.capacity()]), @@ -334,7 +379,8 @@ namespace xo { **/ bool insert(const value_type & kv_pair); - bool verify_ok(verify_policy p = verify_policy::throw_only()) const; + /** reset to empty state **/ + void clear(); private: /** insert @p kv_pair, @@ -389,15 +435,6 @@ namespace xo { store_{lub_exp2(lub_group_mult(hint_max_capacity))}, debug_flag_{debug_flag} { - /* invariant: arenas have allocated address range, but no committed memory yet */ - this->store_.control_.resize(store_.n_slot_ + c_group_size); - - /* all slots marked empty initially */ - std::fill(this->store_.control_.begin(), - this->store_.control_.end(), - c_empty_slot); - - this->store_.slots_.resize(store_.n_slot_); } template @@ -426,6 +463,8 @@ namespace xo { -> std::pair { + scope log(XO_DEBUG(false)); + size_type h = hash_value; // h1: hi bits: probe sequence size_type h1 = h >> 7; @@ -434,6 +473,10 @@ namespace xo { size_type N = p_store->capacity(); + if (N == 0) [[unlikely]] { + return std::make_pair(nullptr, false); + } + // same as: // ix = h1 % N // since N is power of 2 @@ -528,36 +571,58 @@ namespace xo { bool DArenaHashMap::_try_grow() { - size_type n_group_exponent_2x = store_.n_group_exponent_ + 1; - size_type n_group_2x = store_.n_group_ * 2; + scope log(XO_DEBUG(false)); - detail::HashMapStore store_2x(std::make_pair(n_group_exponent_2x, - n_group_2x)); - /* rehash everything in store_, - * into store_2x - */ + size_type n_group_exponent_2x = 0; + size_type n_group_2x = 0; - for (size_type i = 0, n = store_.capacity(); i < n; ++i) { - uint8_t ctrl = store_.control_[i]; - value_type & kv_pair = store_.slots_[i]; - - if ((ctrl != c_empty_slot) - && (ctrl != c_tombstone)) - { - size_type h = hash_(kv_pair.first); - auto chk = this->_try_insert_aux(h, kv_pair, &store_2x); - - if (!chk.second) { - // shenanigans - something isn't right. - // - may have run out of memory - assert(false); - - return false; - } - } + if (store_.n_group_ == 0) [[unlikely]] { + // special case: grow from hard empty state + n_group_exponent_2x = 0; + n_group_2x = 1; + } else { + n_group_exponent_2x = store_.n_group_exponent_ + 1; + n_group_2x = 2 * n_group_exponent_2x; } - this->store_ = std::move(store_2x); + // optimization when table is empty. in that case can resize + // arenas in place + + if (this->empty()) { + log && log("resize-from-empty branch"); + + this->store_.resize_from_empty(std::make_pair(n_group_exponent_2x, n_group_2x)); + } else { + log && log("duplicate-and-replace branch"); + + detail::HashMapStore store_2x(std::make_pair(n_group_exponent_2x, + n_group_2x)); + /* rehash everything in store_, + * into store_2x + */ + + for (size_type i = 0, n = store_.capacity(); i < n; ++i) { + uint8_t ctrl = store_.control_[i]; + value_type & kv_pair = store_.slots_[i]; + + if ((ctrl != c_empty_slot) + && (ctrl != c_tombstone)) + { + size_type h = hash_(kv_pair.first); + auto chk = this->_try_insert_aux(h, kv_pair, &store_2x); + + if (!chk.second) { + // shenanigans - something isn't right. + // - may have run out of memory + assert(false); + + return false; + } + } + } + + this->store_ = std::move(store_2x); + } return true; } @@ -572,10 +637,15 @@ namespace xo { Hash, Equal>::insert(const std::pair & kv_pair) { + scope log(XO_DEBUG(false)); + auto [slot_addr, ins_flag] = this->try_insert(kv_pair); - if (slot_addr) + if (slot_addr) { + log && log("fast", xtag("slot_addr", (void*)slot_addr), xtag("ins_flag", ins_flag)); + return ins_flag; + } assert((store_.size_ + 1) / static_cast(store_.n_slot_) >= c_max_load_factor); @@ -585,11 +655,23 @@ namespace xo { return ins_flag; } else { + assert(false); + // TODO: set last error. Presumeably reached max size return false; } } + template + void + DArenaHashMap::clear() + { + this->store_.clear(); + } + /** * Verify DArenaHashMap class invariants. * diff --git a/xo-arena/include/xo/arena/DArenaVector.hpp b/xo-arena/include/xo/arena/DArenaVector.hpp index d1d66343..ea1557f1 100644 --- a/xo-arena/include/xo/arena/DArenaVector.hpp +++ b/xo-arena/include/xo/arena/DArenaVector.hpp @@ -84,6 +84,8 @@ namespace xo { void swap(DArenaVector & other) noexcept; + DArenaVector & operator=(DArenaVector && x) noexcept; + private: T * _address_of(size_type i) { return ((T *)store_.lo_) + i; } const T * _address_of(size_type i) const { return ((const T *)store_.lo_) + i; } @@ -126,6 +128,18 @@ namespace xo { } } + template + DArenaVector & + DArenaVector::operator=(DArenaVector && other) noexcept + { + this->size_ = other.size_; + this->store_ = std::move(other.store_); + + other.size_ = 0; + + return *this; + } + template DArenaVector DArenaVector::map(const ArenaConfig & cfg) diff --git a/xo-arena/utest/DArenaHashMap.test.cpp b/xo-arena/utest/DArenaHashMap.test.cpp index 58d75ffb..0d51d197 100644 --- a/xo-arena/utest/DArenaHashMap.test.cpp +++ b/xo-arena/utest/DArenaHashMap.test.cpp @@ -132,6 +132,48 @@ namespace xo { REQUIRE(n == map.size()); } } + + { + map.clear(); + + REQUIRE(map.empty()); + REQUIRE(map.size() == 0); + REQUIRE(map.groups() == 0); + REQUIRE(map.capacity() == 0); + } + + /* slightly different starting point, 0 capacity! */ + { + auto x = map.try_insert(std::make_pair(1, 11)); + + /* try_insert should fail - no capacity */ + REQUIRE(!x.first); + REQUIRE(!x.second); + } + + { + /* insert will grow hash table */ + auto x = map.insert(std::make_pair(1, 11)); + + CHECK(x); + REQUIRE(!map.empty()); + REQUIRE(map.size() == 1); + REQUIRE(map.groups() == 1); + REQUIRE(map.capacity() == DArenaHashMapUtil::c_group_size); + REQUIRE(map.load_factor() == 1/16.0); + + /* verify iteration */ + { + size_t n = 0; + for (auto & ix : map) { + REQUIRE(ix.first == 1); + REQUIRE(ix.second == 11); + ++n; + } + REQUIRE(n == map.size()); + } + } + } TEST_CASE("DArenaHashMap-try-insert2", "[arena][DArenaHashMap]") @@ -153,7 +195,7 @@ namespace xo { * observes test failure */ - for (std::uint32_t n = 0; n <= 2; ) { + for (std::uint32_t n = 0; n <= 8; ) { HashMap hash_map; auto test_fn = [&rgen, &hash_map](bool dbg_flag, @@ -163,6 +205,9 @@ namespace xo { ok_flag &= HashMapUtil::random_inserts(n, dbg_flag, &rgen, &hash_map); + ok_flag &= HashMapUtil::check_forward_iterator(0.0 /*dvalue*/, + dbg_flag, hash_map); + return ok_flag; }; diff --git a/xo-arena/utest/random_hash_ops.hpp b/xo-arena/utest/random_hash_ops.hpp index 548f72c8..85ba8a96 100644 --- a/xo-arena/utest/random_hash_ops.hpp +++ b/xo-arena/utest/random_hash_ops.hpp @@ -7,6 +7,7 @@ #include #include #include +#include #include namespace utest { @@ -379,6 +380,73 @@ namespace utest { } /*random_lookups*/ #endif + /* Require: + * - hash has keys [0..n-1] where n=map size + * - tree value at key k is dvalue+10*k + */ + static bool + check_forward_iterator(uint32_t dvalue, + bool catch_flag, + HashMap & map) + { + using xo::scope; + using xo::xtag; + + /* -> flase if/when verification fails */ + bool ok_flag = true; + + std::size_t const n = map.size(); + + scope log(XO_DEBUG(catch_flag)); + + log && log("map with size n", xtag("n", n)); + + std::unordered_set keys; + + { + auto end_ix = map.end(); + + //log && log(xtag("end_ix", end_ix)); + + auto begin_ix = map.begin(); + auto ix = begin_ix; + + int last_key = -1; + + while (ix != end_ix) { + log && log("forward loop top" + //xtag("ix", ix) + ); + + /* verify: keys in map are in [0 .. n) */ + REQUIRE_ORFAIL(ok_flag, catch_flag, 0 <= ix->first); + REQUIRE_ORFAIL(ok_flag, catch_flag, ix->first < n); + + /* verify: keys in map are unique */ + REQUIRE_ORFAIL(ok_flag, catch_flag, !keys.contains(ix->first)); + keys.insert(ix->first); + + REQUIRE_ORFAIL(ok_flag, catch_flag, ix->second == dvalue + 10 * ix->first); + + last_key = ix->first; + ++ix; + + log && log("forward loop bottom", + xtag("last_key", last_key) + //xtag("next ix", ix) + ); + } + + /* should have visited exactly n locations */ + REQUIRE_ORFAIL(ok_flag, catch_flag, map.size() == keys.size()); + REQUIRE_ORFAIL(ok_flag, catch_flag, ix == end_ix); + + //log && log(xtag("ix", ix), xtag("begin_ix", begin_ix)); + } + + return ok_flag; + } + #ifdef NOT_YET /* Require: * - tree has keys [0..n-1], where n=treẹsize()