From 6b65c8f2308ca8162857ab359e6ac883b26e7f8e Mon Sep 17 00:00:00 2001 From: joaquintides Date: Tue, 26 Sep 2023 20:08:13 +0200 Subject: [PATCH] fixed assignment bug with POCXA, fancy, unequal allocators (#214) --- .../unordered/detail/foa/concurrent_table.hpp | 7 +- include/boost/unordered/detail/foa/core.hpp | 80 +++++++++------- include/boost/unordered/detail/foa/table.hpp | 8 +- test/cfoa/assign_tests.cpp | 93 ++++++++++--------- 4 files changed, 95 insertions(+), 93 deletions(-) diff --git a/include/boost/unordered/detail/foa/concurrent_table.hpp b/include/boost/unordered/detail/foa/concurrent_table.hpp index a351815a..11c7bb39 100644 --- a/include/boost/unordered/detail/foa/concurrent_table.hpp +++ b/include/boost/unordered/detail/foa/concurrent_table.hpp @@ -504,16 +504,13 @@ public: x.arrays.elements_});}, size_ctrl_type{x.size_ctrl.ml,x.size_ctrl.size}} { - ah.release(); - x.arrays=ah.get(); + x.arrays=ah.release(); x.size_ctrl.ml=x.initial_max_load(); x.size_ctrl.size=0; } concurrent_table(compatible_nonconcurrent_table&& x): - concurrent_table(std::move(x), arrays_holder< - typename compatible_nonconcurrent_table::arrays_type,Allocator - >{compatible_nonconcurrent_table::arrays_type::new_(x.al(),0),x.al()}) + concurrent_table(std::move(x),x.make_empty_arrays()) {} ~concurrent_table()=default; diff --git a/include/boost/unordered/detail/foa/core.hpp b/include/boost/unordered/detail/foa/core.hpp index 6e0f6123..c994311a 100644 --- a/include/boost/unordered/detail/foa/core.hpp +++ b/include/boost/unordered/detail/foa/core.hpp @@ -970,34 +970,26 @@ Ptr to_pointer(Ptr p) template struct arrays_holder { - arrays_holder(Arrays const& arrays, Allocator const& al) - :arrays_{arrays},al_{al} + arrays_holder(const Arrays& arrays,const Allocator& al): + arrays_{arrays},al_{al} {} - arrays_holder(arrays_holder const&)=delete; + /* not defined but VS in pre-C++17 mode needs to see it for RVO */ + arrays_holder(arrays_holder const&); arrays_holder& operator=(arrays_holder const&)=delete; - ~arrays_holder() - { - if (!released_){ - arrays_.delete_(al_,arrays_); - } - } + ~arrays_holder(){if(!released_)arrays_.delete_(al_,arrays_);} - Arrays const& get()const + const Arrays& release() { + released_=true; return arrays_; } - void release() - { - released_=true; - } - private: - Arrays arrays_; + Arrays arrays_; Allocator al_; - bool released_=false; + bool released_=false; }; template @@ -1367,6 +1359,7 @@ public: using size_type=std::size_t; using difference_type=std::ptrdiff_t; using locator=table_locator; + using arrays_holder_type=arrays_holder; table_core( std::size_t n=default_bucket_count,const Hash& h_=Hash(), @@ -1394,15 +1387,12 @@ public: table_core{x,alloc_traits::select_on_container_copy_construction(x.al())}{} template - table_core( - table_core&& x,arrays_holder&& ah, - ArraysFn arrays_fn): + table_core(table_core&& x,arrays_holder_type&& ah,ArraysFn arrays_fn): table_core( std::move(x.h()),std::move(x.pred()),std::move(x.al()), arrays_fn,x.size_ctrl) { - ah.release(); - x.arrays=ah.get(); + x.arrays=ah.release(); x.size_ctrl.ml=x.initial_max_load(); x.size_ctrl.size=0; } @@ -1414,9 +1404,7 @@ public: std::is_nothrow_move_constructible::value&& !uses_fancy_pointers): table_core{ - std::move(x),arrays_holder{ - x.new_arrays(0),x.al()}, - [&x]{return x.arrays;}} + std::move(x),x.make_empty_arrays(),[&x]{return x.arrays;}} {} table_core(const table_core& x,const Allocator& al_): @@ -1468,6 +1456,11 @@ public: } } + arrays_holder_type make_empty_arrays()const + { + return make_arrays(0); + } + table_core& operator=(const table_core& x) { BOOST_UNORDERED_STATIC_ASSERT_HASH_PRED(Hash, Pred) @@ -1482,9 +1475,6 @@ public: hasher tmp_h=x.h(); key_equal tmp_p=x.pred(); - /* already noexcept, clear() before we swap the Hash, Pred just in case - * the clear() impl relies on them at some point in the future. - */ clear(); /* Because we've asserted at compile-time that Hash and Pred are nothrow @@ -1496,7 +1486,12 @@ public: swap(pred(),tmp_p); if_constexpr([&,this]{ - if(al()!=x.al())reserve(0); + if(al()!=x.al()){ + auto ah=x.make_arrays(std::size_t(std::ceil(float(x.size())/mlf))); + delete_arrays(arrays); + arrays=ah.release(); + size_ctrl.ml=initial_max_load(); + } copy_assign_if(al(),x.al()); }); /* noshrink: favor memory reuse over tightness */ @@ -1535,16 +1530,24 @@ public: using std::swap; clear(); - swap(h(),x.h()); - swap(pred(),x.pred()); if(pocma||al()==x.al()){ - reserve(0); + auto ah=x.make_empty_arrays(); + swap(h(),x.h()); + swap(pred(),x.pred()); + delete_arrays(arrays); move_assign_if(al(),x.al()); - swap(arrays,x.arrays); - swap(size_ctrl,x.size_ctrl); + arrays=x.arrays; + size_ctrl.ml=std::size_t(x.size_ctrl.ml); + size_ctrl.size=std::size_t(x.size_ctrl.size); + x.arrays=ah.release(); + x.size_ctrl.ml=x.initial_max_load(); + x.size_ctrl.size=0; } else{ + swap(h(),x.h()); + swap(pred(),x.pred()); + /* noshrink: favor memory reuse over tightness */ noshrink_reserve(x.size()); clear_on_exit c{x}; @@ -1940,12 +1943,12 @@ private: { } - arrays_type new_arrays(std::size_t n) + arrays_type new_arrays(std::size_t n)const { return arrays_type::new_(al(),n); } - arrays_type new_arrays_for_growth() + arrays_type new_arrays_for_growth()const { /* Due to the anti-drift mechanism (see recover_slot), the new arrays may * be of the same size as the old arrays; in the limit, erasing one @@ -1966,6 +1969,11 @@ private: arrays_type::delete_(al(),arrays_); } + arrays_holder_type make_arrays(std::size_t n)const + { + return {new_arrays(n),al()}; + } + template void construct_element_from_try_emplace_args( element_type* p,std::false_type,Key&& x,Args&&... args) diff --git a/include/boost/unordered/detail/foa/table.hpp b/include/boost/unordered/detail/foa/table.hpp index f8308b1e..78cfa3b1 100644 --- a/include/boost/unordered/detail/foa/table.hpp +++ b/include/boost/unordered/detail/foa/table.hpp @@ -555,19 +555,15 @@ private: x.arrays.elements_};}, size_ctrl_type{x.size_ctrl.ml,x.size_ctrl.size}} { - ah.release(); - compatible_concurrent_table::arrays_type::delete_group_access(x.al(),x.arrays); - x.arrays=ah.get(); + x.arrays=ah.release(); x.size_ctrl.ml=x.initial_max_load(); x.size_ctrl.size=0; } template table(compatible_concurrent_table&& x,ExclusiveLockGuard): - table(std::move(x),arrays_holder< - typename compatible_concurrent_table::arrays_type,Allocator - >{compatible_concurrent_table::arrays_type::new_(x.al(),0),x.al()}) + table(std::move(x),x.make_empty_arrays()) {} struct erase_on_exit diff --git a/test/cfoa/assign_tests.cpp b/test/cfoa/assign_tests.cpp index 6d2253f2..276c47b2 100644 --- a/test/cfoa/assign_tests.cpp +++ b/test/cfoa/assign_tests.cpp @@ -104,78 +104,82 @@ std::initializer_list set_init_list{ auto test_map_and_init_list=std::make_pair(test_map,map_init_list); auto test_set_and_init_list=std::make_pair(test_set,set_init_list); -template struct pocca_allocator +template +struct poca_allocator: fancy_allocator { - using propagate_on_container_copy_assignment = std::true_type; + using super = fancy_allocator; + using pointer = typename super::pointer; + using propagate_on_container_copy_assignment = + std::integral_constant; + using propagate_on_container_move_assignment = + std::integral_constant; int x_ = -1; - using value_type = T; + template struct rebind + { + typedef poca_allocator other; + }; - pocca_allocator() = default; - pocca_allocator(pocca_allocator const&) = default; - pocca_allocator(pocca_allocator&&) = default; + poca_allocator() = default; + poca_allocator(poca_allocator const&) = default; + poca_allocator(poca_allocator &&) = default; - pocca_allocator(int const x) : x_{x} {} + poca_allocator(int const x) : x_{x} {} - pocca_allocator& operator=(pocca_allocator const& rhs) + poca_allocator& operator=(poca_allocator const& rhs) { if (this != &rhs) { + super::operator=(rhs); x_ = rhs.x_; } return *this; } - template pocca_allocator(pocca_allocator const& rhs) : x_{rhs.x_} + template poca_allocator( + poca_allocator const& rhs) : + super{rhs}, x_{rhs.x_} { } - T* allocate(std::size_t n) + pointer allocate(std::size_t n) { - return static_cast(::operator new(n * sizeof(T))); + auto p = super::allocate(n + 1); + reinterpret_cast(*p) = static_cast(x_); + return p + std::ptrdiff_t(1); } - void deallocate(T* p, std::size_t) { ::operator delete(p); } + void deallocate(pointer p, std::size_t n) + { + p = p + std::ptrdiff_t(-1); + BOOST_TEST_EQ(reinterpret_cast(*p), static_cast(x_)); + super::deallocate(p, n + 1); + } - bool operator==(pocca_allocator const& rhs) const { return x_ == rhs.x_; } - bool operator!=(pocca_allocator const& rhs) const { return x_ != rhs.x_; } + bool operator==(poca_allocator const& rhs) const { return x_ == rhs.x_; } + bool operator!=(poca_allocator const& rhs) const { return x_ != rhs.x_; } }; -template struct pocma_allocator +template +struct pocca_allocator: poca_allocator { - using propagate_on_container_move_assignment = std::true_type; + pocca_allocator() = default; + pocca_allocator(pocca_allocator const&) = default; + pocca_allocator(pocca_allocator &&) = default; + using poca_allocator::poca_allocator; - int x_ = -1; - - using value_type = T; + pocca_allocator& operator=(pocca_allocator const&) = default; +}; +template +struct pocma_allocator: poca_allocator +{ pocma_allocator() = default; pocma_allocator(pocma_allocator const&) = default; - pocma_allocator(pocma_allocator&&) = default; + pocma_allocator(pocma_allocator &&) = default; + using poca_allocator::poca_allocator; - pocma_allocator(int const x) : x_{x} {} - - pocma_allocator& operator=(pocma_allocator const& rhs) - { - if (this != &rhs) { - x_ = rhs.x_; - } - return *this; - } - - template pocma_allocator(pocma_allocator const& rhs) : x_{rhs.x_} - { - } - - T* allocate(std::size_t n) - { - return static_cast(::operator new(n * sizeof(T))); - } - - void deallocate(T* p, std::size_t) { ::operator delete(p); } - - bool operator==(pocma_allocator const& rhs) const { return x_ == rhs.x_; } - bool operator!=(pocma_allocator const& rhs) const { return x_ != rhs.x_; } + pocma_allocator& operator=(pocma_allocator const&) = default; }; namespace { @@ -433,9 +437,6 @@ namespace { std::is_nothrow_move_assignable< replace_allocator >::value); - BOOST_STATIC_ASSERT( - std::is_nothrow_move_assignable::value); - BOOST_STATIC_ASSERT( !std::is_nothrow_move_assignable< replace_allocator >::value);