From e82ea5de96bc1beb186449cff4998ebe7cfa5b2b Mon Sep 17 00:00:00 2001 From: Laurent Rineau Date: Wed, 29 Jan 2020 17:13:11 +0100 Subject: [PATCH] Add move-semantic to Compact_container That required a refactoring the timestamper feature. And the explanation why is quite long... Let's do it. The CGAL triangulations use the `Rebind_TDS` feature. During the instanciation of `Compact_container` it is important that `T` is allowed to be incomplete, otherwise the circular dependencies between TDS and Vertex/Cell cannot be resolved by the compiler. In previous implementation of the timestamper, to allow `T` to be an incomplete type, the compact container only carried a *point* to the time stamper, allocated on the heap. A moved-from compact container would only be valid if one recreated a time stamper for it on the head in the move-constructor. As I want move operations to be `noexcept`, I needed to change that implementation. Now the triangulation always carries a time stamp counter (`std::size`), independently of the type `T`, and the time stamper is an empty class, with static methods. That allows `T` to be incomplete during the declaration of `Compact_container`. --- .../include/CGAL/Compact_container.h | 148 +++++++++++------- .../CGAL/Concurrent_compact_container.h | 59 ++++--- STL_Extension/include/CGAL/Time_stamper.h | 33 +--- .../STL_Extension/test_Compact_container.cpp | 14 +- .../test_Concurrent_compact_container.cpp | 4 + 5 files changed, 147 insertions(+), 111 deletions(-) diff --git a/STL_Extension/include/CGAL/Compact_container.h b/STL_Extension/include/CGAL/Compact_container.h index ee83c6c4d0b..86dd6e45fe5 100644 --- a/STL_Extension/include/CGAL/Compact_container.h +++ b/STL_Extension/include/CGAL/Compact_container.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -223,7 +224,8 @@ class Compact_container public: typedef typename Default::Get< TimeStamper_, CGAL::Time_stamper_impl >::type - Time_stamper_impl; + Time_stamper; + typedef Time_stamper Time_stamper_impl; // backward-compatibility typedef T value_type; typedef Allocator allocator_type; @@ -250,16 +252,14 @@ public: explicit Compact_container(const Allocator &a = Allocator()) : alloc(a) - , time_stamper(new Time_stamper_impl()) { - init (); + init(); } template < class InputIterator > Compact_container(InputIterator first, InputIterator last, const Allocator & a = Allocator()) : alloc(a) - , time_stamper(new Time_stamper_impl()) { init(); std::copy(first, last, CGAL::inserter(*this)); @@ -268,14 +268,19 @@ public: // The copy constructor and assignment operator preserve the iterator order Compact_container(const Compact_container &c) : alloc(c.get_allocator()) - , time_stamper(new Time_stamper_impl()) { init(); block_size = c.block_size; - *time_stamper = *c.time_stamper; + time_stamp = c.time_stamp.load(); std::copy(c.begin(), c.end(), CGAL::inserter(*this)); } + Compact_container(Compact_container&& c) noexcept + : alloc(c.get_allocator()) + { + c.swap(*this); + } + Compact_container & operator=(const Compact_container &c) { if (&c != this) { @@ -285,10 +290,16 @@ public: return *this; } + Compact_container & operator=(Compact_container&& c) noexcept + { + Self tmp(std::move(c)); + tmp.swap(*this); + return *this; + } + ~Compact_container() { clear(); - delete time_stamper; } bool is_used(const_iterator ptr) const @@ -328,17 +339,8 @@ public: return all_items[block_number].first[index_in_block]; } - void swap(Self &c) - { - std::swap(alloc, c.alloc); - std::swap(capacity_, c.capacity_); - std::swap(size_, c.size_); - std::swap(block_size, c.block_size); - std::swap(first_item, c.first_item); - std::swap(last_item, c.last_item); - std::swap(free_list, c.free_list); - all_items.swap(c.all_items); - std::swap(time_stamper, c.time_stamper); + friend void swap(Compact_container& a, Compact_container b) { + a.swap(b); } iterator begin() { return iterator(first_item, 0, 0); } @@ -383,7 +385,7 @@ public: new (ret) value_type(args...); CGAL_assertion(type(ret) == USED); ++size_; - time_stamper->set_time_stamp(ret); + Time_stamper::set_time_stamp(ret, time_stamp); return iterator(ret, 0); } @@ -397,7 +399,7 @@ public: std::allocator_traits::construct(alloc, ret, t); CGAL_assertion(type(ret) == USED); ++size_; - time_stamper->set_time_stamp(ret); + Time_stamper::set_time_stamp(ret, time_stamp); return iterator(ret, 0); } @@ -652,7 +654,21 @@ public: static bool is_begin_or_end(const_pointer ptr) { return type(ptr)==START_END; } + void swap(Self &c) + { + std::swap(alloc, c.alloc); + std::swap(capacity_, c.capacity_); + std::swap(size_, c.size_); + std::swap(block_size, c.block_size); + std::swap(first_item, c.first_item); + std::swap(last_item, c.last_item); + std::swap(free_list, c.free_list); + all_items.swap(c.all_items); + // non-atomic swap of time_stamp: + c.time_stamp = time_stamp.exchange(c.time_stamp.load()); + } +private: // We store a vector of pointers to all allocated blocks and their sizes. // Knowing all pointers, we don't have to walk to the end of a block to reach // the pointer to the next block. @@ -660,7 +676,9 @@ public: // by walking through the block till its end. // This opens up the possibility for the compiler to optimize the clear() // function considerably when has_trivial_destructor. - typedef std::vector > All_items; + using All_items = std::vector >; + + using time_stamp_t = std::atomic; void init() { @@ -671,21 +689,18 @@ public: first_item = nullptr; last_item = nullptr; all_items = All_items(); - time_stamper->reset(); + time_stamp = 0; } allocator_type alloc; - size_type capacity_; - size_type size_; - size_type block_size; - pointer free_list; - pointer first_item; - pointer last_item; - All_items all_items; - - // This is a pointer, so that the definition of Compact_container does - // not require a complete type `T`. - Time_stamper_impl* time_stamper; + size_type capacity_ = 0; + size_type size_ = 0; + size_type block_size = Increment_policy::first_block_size; + pointer free_list = nullptr; + pointer first_item = nullptr; + pointer last_item = nullptr; + All_items all_items = {}; + time_stamp_t time_stamp = {}; }; template < class T, class Allocator, class Increment_policy, class TimeStamper > @@ -759,7 +774,7 @@ void Compact_container::allocate_ne for (size_type i = block_size; i >= 1; --i) { EraseCounterStrategy::set_erase_counter(*(new_block + i), 0); - time_stamper->initialize_time_stamp(new_block + i); + Time_stamper::initialize_time_stamp(new_block + i); put_on_free_list(new_block + i); } // We insert this new block at the end. @@ -839,7 +854,6 @@ namespace internal { template < class DSC, bool Const > class CC_iterator { - typedef typename DSC::iterator iterator; typedef CC_iterator Self; public: typedef DSC CC; @@ -861,26 +875,52 @@ namespace internal { m_ptr.p = nullptr; } - // Either a harmless copy-ctor, - // or a conversion from iterator to const_iterator. - CC_iterator (const iterator &it) + CC_iterator (const CC_iterator &it) #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP - : ts(Time_stamper_impl::time_stamp(it.operator->())) + : ts(Time_stamper::time_stamp(it.operator->())) #endif { m_ptr.p = it.operator->(); } - // Same for assignment operator (otherwise MipsPro warns) - CC_iterator & operator= (const iterator &it) - { - m_ptr.p = it.operator->(); + // Converting constructor from mutable to constant iterator + template + CC_iterator(const CC_iterator< + typename std::enable_if<(Const && !OtherConst), DSC>::type, + OtherConst> &const_it) #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP - ts = Time_stamper_impl::time_stamp(it.operator->()); + : ts(Time_stamper::time_stamp(const_it.operator->())) +#endif + { + m_ptr.p = const_it.operator->(); + } + + // Assignment operator from mutable to constant iterator + template + CC_iterator & operator= (const CC_iterator< + typename std::enable_if<(Const && !OtherConst), DSC>::type, + OtherConst> &const_it) + { + m_ptr.p = const_it.operator->(); +#ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP + ts = Time_stamper::time_stamp(const_it.operator->()); #endif return *this; } + CC_iterator(CC_iterator&& it) noexcept +#ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP + : ts(Time_stamper::time_stamp(it.operator->())) +#endif + { + m_ptr.p = it.operator->(); + it.m_ptr.p = nullptr; + } + + ~CC_iterator() = default; + CC_iterator& operator=(const CC_iterator&) = default; + CC_iterator& operator=(CC_iterator&&) = default; + // Construction from nullptr CC_iterator (std::nullptr_t CGAL_assertion_code(n)) #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP @@ -893,7 +933,7 @@ namespace internal { private: - typedef typename DSC::Time_stamper_impl Time_stamper_impl; + typedef typename DSC::Time_stamper Time_stamper; #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP std::size_t ts; #endif @@ -925,7 +965,7 @@ namespace internal { increment(); #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP else - ts = Time_stamper_impl::time_stamp(m_ptr.p); + ts = Time_stamper::time_stamp(m_ptr.p); #endif // CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP } @@ -938,7 +978,7 @@ namespace internal { m_ptr.p = ptr; #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP if(ptr != nullptr){ - ts = Time_stamper_impl::time_stamp(m_ptr.p); + ts = Time_stamper::time_stamp(m_ptr.p); } #endif // end CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP } @@ -959,7 +999,7 @@ namespace internal { DSC::type(m_ptr.p) == DSC::START_END) { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP - ts = Time_stamper_impl::time_stamp(m_ptr.p); + ts = Time_stamper::time_stamp(m_ptr.p); #endif return; } @@ -983,7 +1023,7 @@ namespace internal { DSC::type(m_ptr.p) == DSC::START_END) { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP - ts = Time_stamper_impl::time_stamp(m_ptr.p); + ts = Time_stamper::time_stamp(m_ptr.p); #endif return; } @@ -1022,7 +1062,7 @@ namespace internal { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP bool is_time_stamp_valid() const { - return (ts == 0) || (ts == Time_stamper_impl::time_stamp(m_ptr.p)); + return (ts == 0) || (ts == Time_stamper::time_stamp(m_ptr.p)); } #endif // CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP @@ -1036,7 +1076,7 @@ namespace internal { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP assert( is_time_stamp_valid() ); #endif - return Time_stamper_impl::less(m_ptr.p, other.m_ptr.p); + return Time_stamper::less(m_ptr.p, other.m_ptr.p); } bool operator>(const CC_iterator& other) const @@ -1044,7 +1084,7 @@ namespace internal { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP assert( is_time_stamp_valid() ); #endif - return Time_stamper_impl::less(other.m_ptr.p, m_ptr.p); + return Time_stamper::less(other.m_ptr.p, m_ptr.p); } bool operator<=(const CC_iterator& other) const @@ -1052,7 +1092,7 @@ namespace internal { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP assert( is_time_stamp_valid() ); #endif - return Time_stamper_impl::less(m_ptr.p, other.m_ptr.p) + return Time_stamper::less(m_ptr.p, other.m_ptr.p) || (*this == other); } @@ -1061,7 +1101,7 @@ namespace internal { #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP assert( is_time_stamp_valid() ); #endif - return Time_stamper_impl::less(other.m_ptr.p, m_ptr.p) + return Time_stamper::less(other.m_ptr.p, m_ptr.p) || (*this == other); } diff --git a/STL_Extension/include/CGAL/Concurrent_compact_container.h b/STL_Extension/include/CGAL/Concurrent_compact_container.h index 5fad68c1719..c9ee56954b7 100644 --- a/STL_Extension/include/CGAL/Concurrent_compact_container.h +++ b/STL_Extension/include/CGAL/Concurrent_compact_container.h @@ -210,7 +210,8 @@ class Concurrent_compact_container typedef Concurrent_compact_container_traits Traits; public: - typedef CGAL::Time_stamper_impl Time_stamper_impl; + typedef CGAL::Time_stamper_impl Time_stamper; + typedef Time_stamper Time_stamper_impl; // backward compatibility typedef T value_type; typedef Allocator allocator_type; @@ -241,7 +242,6 @@ public: explicit Concurrent_compact_container(const Allocator &a = Allocator()) : m_alloc(a) - , m_time_stamper(new Time_stamper_impl()) { init (); } @@ -250,7 +250,6 @@ public: Concurrent_compact_container(InputIterator first, InputIterator last, const Allocator & a = Allocator()) : m_alloc(a) - , m_time_stamper(new Time_stamper_impl()) { init(); std::copy(first, last, CGAL::inserter(*this)); @@ -259,13 +258,18 @@ public: // The copy constructor and assignment operator preserve the iterator order Concurrent_compact_container(const Concurrent_compact_container &c) : m_alloc(c.get_allocator()) - , m_time_stamper(new Time_stamper_impl()) { init(); m_block_size = c.m_block_size; std::copy(c.begin(), c.end(), CGAL::inserter(*this)); } + Concurrent_compact_container(Concurrent_compact_container&& c) noexcept + : m_alloc(c.get_allocator()) + { + c.swap(*this); + } + Concurrent_compact_container & operator=(const Concurrent_compact_container &c) { if (&c != this) { @@ -275,10 +279,16 @@ public: return *this; } + Concurrent_compact_container & operator=(Concurrent_compact_container&& c) noexcept + { + Self tmp(std::move(c)); + tmp.swap(*this); + return *this; + } + ~Concurrent_compact_container() { clear(); - delete m_time_stamper; } bool is_used(const_iterator ptr) const @@ -290,11 +300,8 @@ public: { std::swap(m_alloc, c.m_alloc); #if CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE - { // non-atomic swap - size_type other_capacity = c.m_capacity; - c.m_capacity = size_type(m_capacity); - m_capacity = other_capacity; - } + // non-atomic swap of m_capacity + c.m_capacity = m_capacity.exchange(c.m_capacity.load()); #else // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE std::swap(m_capacity, c.m_capacity); #endif // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE @@ -304,7 +311,12 @@ public: std::swap(m_last_item, c.m_last_item); std::swap(m_free_lists, c.m_free_lists); m_all_items.swap(c.m_all_items); - std::swap(m_time_stamper, c.m_time_stamper); + // non-atomic swap of m_time_stamp + c.m_time_stamp = m_time_stamp.exchange(c.m_time_stamp.load()); + } + + friend void swap(Concurrent_compact_container& a, Concurrent_compact_container& b) { + a.swap(b); } iterator begin() { return iterator(m_first_item, 0, 0); } @@ -544,7 +556,7 @@ private: { CGAL_assertion(type(ret) == USED); fl->dec_size(); - m_time_stamper->set_time_stamp(ret); + Time_stamper::set_time_stamp(ret, m_time_stamp); return iterator(ret, 0); } @@ -619,8 +631,9 @@ private: // by walking through the block till its end. // This opens up the possibility for the compiler to optimize the clear() // function considerably when has_trivial_destructor. - typedef std::vector > All_items; + using All_items = std::vector >; + using time_stamp_t = std::atomic; void init() { @@ -636,25 +649,23 @@ private: m_first_item = nullptr; m_last_item = nullptr; m_all_items = All_items(); - m_time_stamper->reset(); + m_time_stamp = 0; } allocator_type m_alloc; #if CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE - std::atomic m_capacity; + std::atomic m_capacity = {}; #else // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE - size_type m_capacity; + size_type m_capacity = {}; #endif // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE - size_type m_block_size; + size_type m_block_size = CGAL_INIT_CONCURRENT_COMPACT_CONTAINER_BLOCK_SIZE; Free_lists m_free_lists; - pointer m_first_item; - pointer m_last_item; - All_items m_all_items; + pointer m_first_item = nullptr; + pointer m_last_item = nullptr; + All_items m_all_items = {}; mutable Mutex m_mutex; + time_stamp_t m_time_stamp = {}; - // This is a pointer, so that the definition of Compact_container does - // not require a complete type `T`. - Time_stamper_impl* m_time_stamper; }; template < class T, class Allocator > @@ -770,7 +781,7 @@ void Concurrent_compact_container:: for (size_type i = old_block_size; i >= 1; --i) { EraseCounterStrategy::set_erase_counter(*(new_block + i), 0); - m_time_stamper->initialize_time_stamp(new_block + i); + Time_stamper::initialize_time_stamp(new_block + i); put_on_free_list(new_block + i, fl); } } diff --git a/STL_Extension/include/CGAL/Time_stamper.h b/STL_Extension/include/CGAL/Time_stamper.h index 6d65ec3cfb1..f0cfe44a574 100644 --- a/STL_Extension/include/CGAL/Time_stamper.h +++ b/STL_Extension/include/CGAL/Time_stamper.h @@ -28,26 +28,12 @@ constexpr size_t rounded_down_log2(size_t n) template struct Time_stamper { - Time_stamper() - : time_stamp_() {} - - Time_stamper(const Time_stamper& ts) - : time_stamp_() - { - time_stamp_ = std::size_t(ts.time_stamp_); - } - - Time_stamper& operator=(const Time_stamper& ts) - { - time_stamp_ = std::size_t(ts.time_stamp_); - return *this; - } - static void initialize_time_stamp(T* pt) { pt->set_time_stamp(std::size_t(-1)); } - void set_time_stamp(T* pt) { + template + static void set_time_stamp(T* pt, time_stamp_t& time_stamp_) { if(pt->time_stamp() == std::size_t(-1)) { const std::size_t new_ts = time_stamp_++; pt->set_time_stamp(new_ts); @@ -96,23 +82,14 @@ struct Time_stamper return time_stamp(p_t1) < time_stamp(p_t2); } } - - void reset() { - time_stamp_ = 0; - } -private: -#ifdef CGAL_NO_ATOMIC - std::size_t time_stamp_; -#else - CGAL::cpp11::atomic time_stamp_; -#endif }; // end class template Time_stamper template struct No_time_stamp { public: - void set_time_stamp(T*) {} + template + static void set_time_stamp(T*, time_stamp_t&) {} static bool less(const T* p_t1,const T* p_t2) { return p_t1 < p_t2; } @@ -130,8 +107,6 @@ public: constexpr std::size_t shift = internal::rounded_down_log2(sizeof(T)); return reinterpret_cast(p) >> shift; } - - void reset() {} }; // end class template No_time_stamp // That class template is an auxiliary class. It has a diff --git a/STL_Extension/test/STL_Extension/test_Compact_container.cpp b/STL_Extension/test/STL_Extension/test_Compact_container.cpp index 117b5d1ec44..3d370105a6a 100644 --- a/STL_Extension/test/STL_Extension/test_Compact_container.cpp +++ b/STL_Extension/test/STL_Extension/test_Compact_container.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -83,6 +84,11 @@ inline bool check_empty(const Cont &c) template < class Cont > void test(const Cont &) { + static_assert(std::is_nothrow_move_constructible::value, + "move cstr is missing"); + static_assert(std::is_nothrow_move_assignable::value, + "move assignment is missing"); + // Testing if all types are provided. typename Cont::value_type t0; @@ -319,22 +325,22 @@ int main() // Check the time stamper policies if(! boost::is_base_of, - C1::Time_stamper_impl>::value) + C1::Time_stamper>::value) { std::cerr << "Error timestamper of C1\n"; return 1; } if(! boost::is_base_of, - C2::Time_stamper_impl>::value) + C2::Time_stamper>::value) { std::cerr << "Error timestamper of C2\n"; return 1; } if(! boost::is_base_of, - C3::Time_stamper_impl>::value) + C3::Time_stamper>::value) { std::cerr << "Error timestamper of C3\n"; return 1; } if(! boost::is_base_of, - C4::Time_stamper_impl>::value) + C4::Time_stamper>::value) { std::cerr << "Error timestamper of C4\n"; return 1; } diff --git a/STL_Extension/test/STL_Extension/test_Concurrent_compact_container.cpp b/STL_Extension/test/STL_Extension/test_Concurrent_compact_container.cpp index afe1de689da..8db20f3b173 100644 --- a/STL_Extension/test/STL_Extension/test_Concurrent_compact_container.cpp +++ b/STL_Extension/test/STL_Extension/test_Concurrent_compact_container.cpp @@ -183,6 +183,10 @@ private: template < class Cont > void test(const Cont &) { + static_assert(std::is_nothrow_move_constructible::value, + "move cstr is missing"); + static_assert(std::is_nothrow_move_assignable::value, + "move assignment is missing"); // Testing if all types are provided. typename Cont::value_type t0;