From ee88150bfbbe976ff9d7aab7e953e58b236c89a8 Mon Sep 17 00:00:00 2001 From: YexuanXiao Date: Mon, 23 Sep 2024 17:16:41 +0800 Subject: [PATCH 1/5] Stateful allocator support for concurrent_queue and concurrent_bounded_queue --- include/oneapi/tbb/concurrent_queue.h | 82 +++++++++++-------- .../tbb/detail/_concurrent_queue_base.h | 8 +- 2 files changed, 51 insertions(+), 39 deletions(-) diff --git a/include/oneapi/tbb/concurrent_queue.h b/include/oneapi/tbb/concurrent_queue.h index cfd5db6a55..cab15e76c9 100644 --- a/include/oneapi/tbb/concurrent_queue.h +++ b/include/oneapi/tbb/concurrent_queue.h @@ -137,25 +137,29 @@ class concurrent_queue { } concurrent_queue& operator=( const concurrent_queue& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_copy_assignment - if (my_queue_representation != other.my_queue_representation) { - clear(); + if (my_queue_representation == other.my_queue_representation) + return *this; + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { my_allocator = other.my_allocator; - my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); } + my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); return *this; } concurrent_queue& operator=( concurrent_queue&& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_move_assignment - if (my_queue_representation != other.my_queue_representation) { - clear(); + if (my_queue_representation == other.my_queue_representation) + return *this; + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { + my_allocator = std::move(other.my_allocator); + internal_swap(other); + } else { if (my_allocator == other.my_allocator) { internal_swap(other); } else { - my_queue_representation->assign(*other.my_queue_representation, other.my_allocator, move_construct_item); + my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); other.clear(); - my_allocator = std::move(other.my_allocator); } } return *this; @@ -178,8 +182,12 @@ class concurrent_queue { } void swap ( concurrent_queue& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_swap - __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); + if (queue_allocator_traits::propagate_on_container_swap::value) { + using std::swap; + swap(my_allocator, other.my_allocator); + } else { + __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); + } internal_swap(other); } @@ -253,15 +261,13 @@ class concurrent_queue { template friend class concurrent_queue_iterator; - static void copy_construct_item(T* location, const void* src) { - // TODO: use allocator_traits for copy construction - new (location) value_type(*static_cast(src)); - // queue_allocator_traits::construct(my_allocator, location, *static_cast(src)); + + static void copy_construct_item(queue_allocator_type& allocator, T* location, const void* src) { + queue_allocator_traits::construct(allocator, location, *static_cast(src)); } - static void move_construct_item(T* location, const void* src) { - // TODO: use allocator_traits for move construction - new (location) value_type(std::move(*static_cast(const_cast(src)))); + static void move_construct_item(queue_allocator_type& allocator, T* location, const void* src) { + queue_allocator_traits::construct(allocator, location, std::move(*static_cast(const_cast(src)))); } queue_allocator_type my_allocator; @@ -416,25 +422,29 @@ class concurrent_bounded_queue { } concurrent_bounded_queue& operator=( const concurrent_bounded_queue& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_copy_assignment - if (my_queue_representation != other.my_queue_representation) { - clear(); + if (my_queue_representation == other.my_queue_representation) + return *this; + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { my_allocator = other.my_allocator; - my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); } + my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); return *this; } concurrent_bounded_queue& operator=( concurrent_bounded_queue&& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_move_assignment - if (my_queue_representation != other.my_queue_representation) { - clear(); + if (my_queue_representation == other.my_queue_representation) + return *this; + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { + my_allocator = std::move(other.my_allocator); + internal_swap(other); + } else { if (my_allocator == other.my_allocator) { internal_swap(other); } else { - my_queue_representation->assign(*other.my_queue_representation, other.my_allocator, move_construct_item); + my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); other.clear(); - my_allocator = std::move(other.my_allocator); } } return *this; @@ -457,8 +467,12 @@ class concurrent_bounded_queue { } void swap ( concurrent_bounded_queue& other ) { - //TODO: implement support for std::allocator_traits::propagate_on_container_swap - __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); + if (queue_allocator_traits::propagate_on_container_swap::value) { + using std::swap; + swap(my_allocator, other.my_allocator); + } else { + __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); + } internal_swap(other); } @@ -641,14 +655,12 @@ class concurrent_bounded_queue { r1::abort_bounded_queue_monitors(my_monitors); } - static void copy_construct_item(T* location, const void* src) { - // TODO: use allocator_traits for copy construction - new (location) value_type(*static_cast(src)); + static void copy_construct_item(queue_allocator_type& ator, T* location, const void* src) { + queue_allocator_traits::construct(ator, location, *static_cast(src)); } - static void move_construct_item(T* location, const void* src) { - // TODO: use allocator_traits for move construction - new (location) value_type(std::move(*static_cast(const_cast(src)))); + static void move_construct_item(queue_allocator_type& ator, T* location, const void* src) { + queue_allocator_traits::construct(ator, location, std::move(*static_cast(const_cast(src)))); } template diff --git a/include/oneapi/tbb/detail/_concurrent_queue_base.h b/include/oneapi/tbb/detail/_concurrent_queue_base.h index ee628e1e89..7a14bffc92 100644 --- a/include/oneapi/tbb/detail/_concurrent_queue_base.h +++ b/include/oneapi/tbb/detail/_concurrent_queue_base.h @@ -103,7 +103,7 @@ class micro_queue { using page_allocator_traits = tbb::detail::allocator_traits; public: - using item_constructor_type = void (*)(value_type* location, const void* src); + using item_constructor_type = void (*)(queue_allocator_type&, value_type* location, const void* src); micro_queue() = default; micro_queue( const micro_queue& ) = delete; micro_queue& operator=( const micro_queue& ) = delete; @@ -254,7 +254,7 @@ class micro_queue { new_page->mask.store(src_page->mask.load(std::memory_order_relaxed), std::memory_order_relaxed); for (; begin_in_page!=end_in_page; ++begin_in_page, ++g_index) { if (new_page->mask.load(std::memory_order_relaxed) & uintptr_t(1) << begin_in_page) { - copy_item(*new_page, begin_in_page, *src_page, begin_in_page, construct_item); + copy_item(allocator, *new_page, begin_in_page, *src_page, begin_in_page, construct_item); } } return new_page; @@ -324,11 +324,11 @@ class micro_queue { ~destroyer() {my_value.~T();} }; // class destroyer - void copy_item( padded_page& dst, size_type dindex, const padded_page& src, size_type sindex, + void copy_item( queue_allocator_type& allocator, padded_page& dst, size_type dindex, const padded_page& src, size_type sindex, item_constructor_type construct_item ) { auto& src_item = src[sindex]; - construct_item( &dst[dindex], static_cast(&src_item) ); + construct_item( allocator, &dst[dindex], static_cast(&src_item) ); } void assign_and_destroy_item( void* dst, padded_page& src, size_type index ) { From 94fc13fd3ee3538693a221ce6f49d121fbfb92b2 Mon Sep 17 00:00:00 2001 From: YexuanXiao Date: Tue, 24 Sep 2024 13:44:17 +0800 Subject: [PATCH 2/5] Use TBB helper functions instead of straightforward code --- include/oneapi/tbb/concurrent_queue.h | 26 +++++-------------- include/oneapi/tbb/detail/_allocator_traits.h | 5 +++- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/include/oneapi/tbb/concurrent_queue.h b/include/oneapi/tbb/concurrent_queue.h index cab15e76c9..4749bd9ac9 100644 --- a/include/oneapi/tbb/concurrent_queue.h +++ b/include/oneapi/tbb/concurrent_queue.h @@ -140,9 +140,7 @@ class concurrent_queue { if (my_queue_representation == other.my_queue_representation) return *this; clear(); - if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - my_allocator = other.my_allocator; - } + tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); return *this; } @@ -152,7 +150,7 @@ class concurrent_queue { return *this; clear(); if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - my_allocator = std::move(other.my_allocator); + tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); internal_swap(other); } else { if (my_allocator == other.my_allocator) { @@ -182,12 +180,7 @@ class concurrent_queue { } void swap ( concurrent_queue& other ) { - if (queue_allocator_traits::propagate_on_container_swap::value) { - using std::swap; - swap(my_allocator, other.my_allocator); - } else { - __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); - } + tbb::detail::swap_allocators(my_allocator, other.my_allocator); internal_swap(other); } @@ -425,9 +418,7 @@ class concurrent_bounded_queue { if (my_queue_representation == other.my_queue_representation) return *this; clear(); - if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - my_allocator = other.my_allocator; - } + tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); return *this; } @@ -437,7 +428,7 @@ class concurrent_bounded_queue { return *this; clear(); if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - my_allocator = std::move(other.my_allocator); + tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); internal_swap(other); } else { if (my_allocator == other.my_allocator) { @@ -467,12 +458,7 @@ class concurrent_bounded_queue { } void swap ( concurrent_bounded_queue& other ) { - if (queue_allocator_traits::propagate_on_container_swap::value) { - using std::swap; - swap(my_allocator, other.my_allocator); - } else { - __TBB_ASSERT(my_allocator == other.my_allocator, "unequal allocators"); - } + tbb::detail::swap_allocators(my_allocator, other.my_allocator); internal_swap(other); } diff --git a/include/oneapi/tbb/detail/_allocator_traits.h b/include/oneapi/tbb/detail/_allocator_traits.h index 8c60e25e7e..c5575ab336 100644 --- a/include/oneapi/tbb/detail/_allocator_traits.h +++ b/include/oneapi/tbb/detail/_allocator_traits.h @@ -91,7 +91,10 @@ void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::true } template -void swap_allocators_impl( Allocator&, Allocator&, /*pocs = */ std::false_type ) {} +void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::false_type ) { + // If the lhs and rhs are not equal, the behavior is undefined + __TBB_ASSERT(lhs == rhs, "unequal allocators"); +} // Swaps allocators only if propagate_on_container_swap is true template From 248c40c45e0664748aaa3b8332aab5ef0ac4033d Mon Sep 17 00:00:00 2001 From: YexuanXiao Date: Tue, 24 Sep 2024 15:31:02 +0800 Subject: [PATCH 3/5] Reverse condition --- include/oneapi/tbb/concurrent_queue.h | 60 +++++++++++++-------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/include/oneapi/tbb/concurrent_queue.h b/include/oneapi/tbb/concurrent_queue.h index 4749bd9ac9..ca9a1d814e 100644 --- a/include/oneapi/tbb/concurrent_queue.h +++ b/include/oneapi/tbb/concurrent_queue.h @@ -137,27 +137,27 @@ class concurrent_queue { } concurrent_queue& operator=( const concurrent_queue& other ) { - if (my_queue_representation == other.my_queue_representation) - return *this; - clear(); - tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); - my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); + if (this != &other) { + clear(); + tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); + my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); + } return *this; } concurrent_queue& operator=( concurrent_queue&& other ) { - if (my_queue_representation == other.my_queue_representation) - return *this; - clear(); - if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); - internal_swap(other); - } else { - if (my_allocator == other.my_allocator) { + if (this != &other) { + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { + tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); internal_swap(other); } else { - my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); - other.clear(); + if (my_allocator == other.my_allocator) { + internal_swap(other); + } else { + my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); + other.clear(); + } } } return *this; @@ -415,27 +415,27 @@ class concurrent_bounded_queue { } concurrent_bounded_queue& operator=( const concurrent_bounded_queue& other ) { - if (my_queue_representation == other.my_queue_representation) - return *this; - clear(); - tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); - my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); + if (this != &other) { + clear(); + tbb::detail::copy_assign_allocators(my_allocator, other.my_allocator); + my_queue_representation->assign(*other.my_queue_representation, my_allocator, copy_construct_item); + } return *this; } concurrent_bounded_queue& operator=( concurrent_bounded_queue&& other ) { - if (my_queue_representation == other.my_queue_representation) - return *this; - clear(); - if (queue_allocator_traits::propagate_on_container_move_assignment::value) { - tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); - internal_swap(other); - } else { - if (my_allocator == other.my_allocator) { + if (this != &other) { + clear(); + if (queue_allocator_traits::propagate_on_container_move_assignment::value) { + tbb::detail::move_assign_allocators(my_allocator, other.my_allocator); internal_swap(other); } else { - my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); - other.clear(); + if (my_allocator == other.my_allocator) { + internal_swap(other); + } else { + my_queue_representation->assign(*other.my_queue_representation, my_allocator, move_construct_item); + other.clear(); + } } } return *this; From d129813702eeb4e15165b488e2fc61622f12623a Mon Sep 17 00:00:00 2001 From: YexuanXiao Date: Thu, 3 Oct 2024 12:40:21 +0800 Subject: [PATCH 4/5] update the copyright year --- include/oneapi/tbb/concurrent_queue.h | 4 +++- include/oneapi/tbb/detail/_allocator_traits.h | 4 ++-- include/oneapi/tbb/detail/_concurrent_queue_base.h | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/oneapi/tbb/concurrent_queue.h b/include/oneapi/tbb/concurrent_queue.h index ca9a1d814e..8bccd0a972 100644 --- a/include/oneapi/tbb/concurrent_queue.h +++ b/include/oneapi/tbb/concurrent_queue.h @@ -1,5 +1,5 @@ /* - Copyright (c) 2005-2023 Intel Corporation + Copyright (c) 2005-2024 Intel Corporation Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -237,6 +237,8 @@ class concurrent_queue { private: void internal_swap(concurrent_queue& src) { + if (!allocator_traits_type::queue_allocator_traits::propagate_on_container) + __TBB_ASSERT(my_allocator == src.my_allocator, "Swapping with unequal allocators is not allowed"); using std::swap; swap(my_queue_representation, src.my_queue_representation); } diff --git a/include/oneapi/tbb/detail/_allocator_traits.h b/include/oneapi/tbb/detail/_allocator_traits.h index c5575ab336..27209f8c80 100644 --- a/include/oneapi/tbb/detail/_allocator_traits.h +++ b/include/oneapi/tbb/detail/_allocator_traits.h @@ -1,5 +1,5 @@ /* - Copyright (c) 2005-2021 Intel Corporation + Copyright (c) 2005-2024 Intel Corporation Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -93,7 +93,7 @@ void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::true template void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::false_type ) { // If the lhs and rhs are not equal, the behavior is undefined - __TBB_ASSERT(lhs == rhs, "unequal allocators"); + __TBB_ASSERT(lhs == rhs, "Swapping with unequal allocators is not allowed"); } // Swaps allocators only if propagate_on_container_swap is true diff --git a/include/oneapi/tbb/detail/_concurrent_queue_base.h b/include/oneapi/tbb/detail/_concurrent_queue_base.h index 7a14bffc92..4baef94706 100644 --- a/include/oneapi/tbb/detail/_concurrent_queue_base.h +++ b/include/oneapi/tbb/detail/_concurrent_queue_base.h @@ -1,5 +1,5 @@ /* - Copyright (c) 2005-2022 Intel Corporation + Copyright (c) 2005-2024 Intel Corporation Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From 0767ceb5ccaddc503149f57e2d301d2f45489708 Mon Sep 17 00:00:00 2001 From: YexuanXiao Date: Thu, 3 Oct 2024 13:16:02 +0800 Subject: [PATCH 5/5] suppress unused warning and avoid call == when always equal --- include/oneapi/tbb/concurrent_queue.h | 2 +- include/oneapi/tbb/detail/_allocator_traits.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/include/oneapi/tbb/concurrent_queue.h b/include/oneapi/tbb/concurrent_queue.h index 8bccd0a972..5a1dea42ed 100644 --- a/include/oneapi/tbb/concurrent_queue.h +++ b/include/oneapi/tbb/concurrent_queue.h @@ -237,7 +237,7 @@ class concurrent_queue { private: void internal_swap(concurrent_queue& src) { - if (!allocator_traits_type::queue_allocator_traits::propagate_on_container) + if (!queue_allocator_traits::propagate_on_container_swap::value) __TBB_ASSERT(my_allocator == src.my_allocator, "Swapping with unequal allocators is not allowed"); using std::swap; swap(my_queue_representation, src.my_queue_representation); diff --git a/include/oneapi/tbb/detail/_allocator_traits.h b/include/oneapi/tbb/detail/_allocator_traits.h index 27209f8c80..3734313ec0 100644 --- a/include/oneapi/tbb/detail/_allocator_traits.h +++ b/include/oneapi/tbb/detail/_allocator_traits.h @@ -93,7 +93,11 @@ void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::true template void swap_allocators_impl( Allocator& lhs, Allocator& rhs, /*pocs = */ std::false_type ) { // If the lhs and rhs are not equal, the behavior is undefined - __TBB_ASSERT(lhs == rhs, "Swapping with unequal allocators is not allowed"); + if (!allocator_traits::is_always_equal::value) { + __TBB_ASSERT(lhs == rhs, "Swapping with unequal allocators is not allowed"); + } + tbb::detail::suppress_unused_warning(lhs); + tbb::detail::suppress_unused_warning(rhs); } // Swaps allocators only if propagate_on_container_swap is true