From d47deb39e9e57c5e2220c2ef376b83ea18dcab40 Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Thu, 27 Sep 2018 17:09:35 -0700 Subject: [PATCH] slot_map: The free list should be LIFO, not FIFO. This is mentioned at the end of Allan's paper http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0661r0.pdf > Alternatively, the free list could store both a front and back index > on the container, providing the ability to cycle through free slots > more uniformly, reducing the chance of a generation counter overflow > occurring. This is a quite valuable benefit with low overhead, > so explicitly requiring it seems worthwhile. --- SG14/slot_map.h | 43 ++++++++++++++++++++++++++++++------- SG14_test/slot_map_test.cpp | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/SG14/slot_map.h b/SG14/slot_map.h index f68a03a4..3c27ce9e 100644 --- a/SG14/slot_map.h +++ b/SG14/slot_map.h @@ -217,14 +217,19 @@ class slot_map // Functions for accessing and modifying the size of the slots container. // These are beneficial as allocating more slots than values will cause the // generation counter increases to be more evenly distributed across the slots. - // TODO [ajo]: The above comment is false, at least for this implementation. // constexpr void reserve_slots(size_type n) { slot_map_detail::reserve_if_possible(slots_, n); - while (slots_.size() < n) { - auto idx = next_available_slot_index_; - next_available_slot_index_ = static_cast(slots_.size()); - slots_.emplace_back(key_type{idx, key_generation_type{}}); + key_index_type original_num_slots = static_cast(slots_.size()); + if (original_num_slots < n) { + slots_.emplace_back(key_type{next_available_slot_index_, key_generation_type{}}); + key_index_type last_new_slot = original_num_slots; + --n; + while (last_new_slot != n) { + slots_.emplace_back(key_type{last_new_slot, key_generation_type{}}); + ++last_new_slot; + } + next_available_slot_index_ = last_new_slot; } } constexpr size_type slot_count() const { return slots_.size(); } @@ -243,9 +248,15 @@ class slot_map if (next_available_slot_index_ == slots_.size()) { auto idx = next_available_slot_index_; ++idx; slots_.emplace_back(key_type{idx, key_generation_type{}}); // make a new slot + last_available_slot_index_ = idx; } auto slot_iter = std::next(slots_.begin(), next_available_slot_index_); - next_available_slot_index_ = this->get_index(*slot_iter); + if (next_available_slot_index_ == last_available_slot_index_) { + next_available_slot_index_ = static_cast(slots_.size()); + last_available_slot_index_ = next_available_slot_index_; + } else { + next_available_slot_index_ = this->get_index(*slot_iter); + } this->set_index(*slot_iter, value_pos); key_type result = *slot_iter; this->set_index(result, std::distance(slots_.begin(), slot_iter)); @@ -292,6 +303,7 @@ class slot_map values_.clear(); reverse_map_.clear(); next_available_slot_index_ = key_index_type{}; + last_available_slot_index_ = key_index_type{}; } // swap is not mentioned in P0661r1 but it should be. @@ -301,6 +313,7 @@ class slot_map swap(values_, rhs.values_); swap(reverse_map_, rhs.reverse_map_); swap(next_available_slot_index_, rhs.next_available_slot_index_); + swap(last_available_slot_index_, rhs.last_available_slot_index_); } protected: @@ -332,9 +345,15 @@ class slot_map values_.pop_back(); reverse_map_.pop_back(); // Expire this key. - this->set_index(*slot_iter, next_available_slot_index_); + if (next_available_slot_index_ == slots_.size()) { + next_available_slot_index_ = static_cast(slot_index); + last_available_slot_index_ = static_cast(slot_index); + } else { + auto last_slot_iter = std::next(slots_.begin(), last_available_slot_index_); + this->set_index(*last_slot_iter, slot_index); + last_available_slot_index_ = static_cast(slot_index); + } this->increment_generation(*slot_iter); - next_available_slot_index_ = static_cast(slot_index); return std::next(values_.begin(), value_index); } @@ -342,6 +361,14 @@ class slot_map Container reverse_map_; // exactly size() entries Container values_; // exactly size() entries key_index_type next_available_slot_index_{}; + key_index_type last_available_slot_index_{}; + + // Class invariant: + // Either next_available_slot_index_ == last_available_slot_index_ == slots_.size(), + // or else 0 <= next_available_slot_index_ < slots_.size() and the "key" of that slot + // entry points to the subsequent available slot, and so on, until reaching + // last_available_slot_index_ (which might equal next_available_slot_index_ if there + // is only one available slot at the moment). }; template class Container> diff --git a/SG14_test/slot_map_test.cpp b/SG14_test/slot_map_test.cpp index d39c3159..9006ea86 100644 --- a/SG14_test/slot_map_test.cpp +++ b/SG14_test/slot_map_test.cpp @@ -473,6 +473,42 @@ static void GenerationsDontSkipTest() #endif } +template +static void IndexesAreUsedEvenlyTest() +{ + using T = typename SM::mapped_type; + SM sm; + auto k1 = sm.emplace(Monad::from_value(1)); + auto k2 = sm.emplace(Monad::from_value(2)); + int original_cap = sm.slot_count(); + for (int i=2; i < original_cap; ++i) { + sm.emplace(Monad::from_value(i)); + } + assert(sm.size() == sm.slot_count()); + + sm.erase(k1); + sm.erase(k2); + assert(sm.size() == sm.slot_count() - 2); + + // There are now two slots available. + // So consecutive insertions should prefer to + // use different slots, rather than sticking to + // just one slot and bumping its generation count. + k1 = sm.emplace(Monad::from_value(1)); + sm.erase(k1); + k2 = sm.emplace(Monad::from_value(2)); + sm.erase(k2); + +#if __cplusplus < 201703L + using std::get; + assert(get<0>(k2) != get<0>(k1)); +#else + auto [idx1, gen1] = k1; + auto [idx2, gen2] = k2; + assert(idx2 != idx1); +#endif +} + void sg14_test::slot_map_test() { TypedefTests(); @@ -488,6 +524,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(true); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); // Test slot_map with a custom key type (C++14 destructuring). using slot_map_2 = stdext::slot_map; @@ -500,6 +537,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(true); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); #if __cplusplus >= 201703L // Test slot_map with a custom key type (C++17 destructuring). @@ -513,6 +551,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(true); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); #endif // __cplusplus >= 201703L // Test slot_map with a custom (but standard and random-access) container type. @@ -526,6 +565,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(false); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); // Test slot_map with a custom (non-standard, random-access) container type. using slot_map_5 = stdext::slot_map, TestContainer::Vector>; @@ -538,6 +578,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(false); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); // Test slot_map with a custom (standard, bidirectional-access) container type. using slot_map_6 = stdext::slot_map, std::list>; @@ -550,6 +591,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(false); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); // Test slot_map with a move-only value_type. // Sadly, standard containers do not propagate move-only-ness, so we must use our custom Vector instead. @@ -567,6 +609,7 @@ void sg14_test::slot_map_test() ReserveTest(); VerifyCapacityExists(false); GenerationsDontSkipTest(); + IndexesAreUsedEvenlyTest(); } #if defined(__cpp_concepts)