Skip to content

Commit d7c2803

Browse files
bernhardmgruberbernhardmgruber
committed
Refactor allocator handling of contiguous_storage
I was hunting a potential bug where the allocator was seemingly swapped twice in some cases, only to find out later that the code is actually correct. This PR proposes the simplifications I made on the way.
1 parent 72f39d9 commit d7c2803

File tree

2 files changed

+96
-199
lines changed

2 files changed

+96
-199
lines changed

‎thrust/thrust/detail/contiguous_storage.h

+88-35
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
#include <thrust/detail/execution_policy.h>
3131
#include <thrust/iterator/detail/normal_iterator.h>
3232

33+
#include <cuda/std/utility>
34+
3335
THRUST_NAMESPACE_BEGIN
3436

3537
namespace detail
@@ -38,6 +40,13 @@ namespace detail
3840
struct copy_allocator_t
3941
{};
4042

43+
struct allocator_mismatch_on_swap : std::runtime_error
44+
{
45+
allocator_mismatch_on_swap()
46+
: std::runtime_error("swap called on containers with allocators that propagate on swap, but compare non-equal")
47+
{}
48+
};
49+
4150
// XXX parameter T is redundant with parameter Alloc
4251
template <typename T, typename Alloc>
4352
class contiguous_storage
@@ -70,6 +79,8 @@ class contiguous_storage
7079
_CCCL_EXEC_CHECK_DISABLE
7180
_CCCL_HOST_DEVICE explicit contiguous_storage(copy_allocator_t, const contiguous_storage& other, size_type n);
7281

82+
contiguous_storage& operator=(const contiguous_storage& x) = delete;
83+
7384
_CCCL_EXEC_CHECK_DISABLE
7485
_CCCL_HOST_DEVICE ~contiguous_storage();
7586

@@ -100,7 +111,33 @@ class contiguous_storage
100111

101112
_CCCL_HOST_DEVICE void deallocate() noexcept;
102113

103-
_CCCL_HOST_DEVICE void swap(contiguous_storage& x);
114+
_CCCL_EXEC_CHECK_DISABLE
115+
_CCCL_HOST_DEVICE void swap(contiguous_storage& x) noexcept(
116+
allocator_traits<Alloc>::propagate_on_container_swap::value || allocator_traits<Alloc>::is_always_equal::value)
117+
{
118+
using ::cuda::std::swap;
119+
swap(m_begin, x.m_begin);
120+
swap(m_size, x.m_size);
121+
122+
// From C++ standard [container.reqmts]
123+
// If allocator_traits<allocator_type>::propagate_on_container_swap::value is true, then allocator_type
124+
// shall meet the Cpp17Swappable requirements and the allocators of a and b shall also be exchanged by calling
125+
// swap as described in [swappable.requirements]. Otherwise, the allocators shall not be swapped, and the behavior
126+
// is undefined unless a.get_allocator() == b.get_allocator().
127+
_CCCL_IF_CONSTEXPR (allocator_traits<Alloc>::propagate_on_container_swap::value)
128+
{
129+
swap(m_allocator, x.m_allocator);
130+
}
131+
else
132+
{
133+
_CCCL_IF_CONSTEXPR (!allocator_traits<Alloc>::is_always_equal::value)
134+
{
135+
NV_IF_TARGET(NV_IS_DEVICE, (assert(m_allocator == other);), (if (m_allocator != x.m_allocator) {
136+
throw allocator_mismatch_on_swap();
137+
}));
138+
}
139+
}
140+
}
104141

105142
_CCCL_HOST_DEVICE void value_initialize_n(iterator first, size_type n);
106143

@@ -122,23 +159,42 @@ class contiguous_storage
122159

123160
_CCCL_HOST_DEVICE void destroy(iterator first, iterator last) noexcept;
124161

125-
_CCCL_HOST_DEVICE void deallocate_on_allocator_mismatch(const contiguous_storage& other) noexcept;
162+
_CCCL_HOST_DEVICE void deallocate_on_allocator_mismatch(const contiguous_storage& other) noexcept
163+
{
164+
// TODO(bgruber): replace dispatch by if constexpr in C++17
165+
integral_constant<bool, allocator_traits<Alloc>::propagate_on_container_copy_assignment::value> c;
166+
deallocate_on_allocator_mismatch_dispatch(c, other);
167+
}
126168

127169
_CCCL_HOST_DEVICE void
128-
destroy_on_allocator_mismatch(const contiguous_storage& other, iterator first, iterator last) noexcept;
170+
destroy_on_allocator_mismatch(const contiguous_storage& other, iterator first, iterator last) noexcept
171+
{
172+
// TODO(bgruber): replace dispatch by if constexpr in C++17
173+
integral_constant<bool, allocator_traits<Alloc>::propagate_on_container_copy_assignment::value> c;
174+
destroy_on_allocator_mismatch_dispatch(c, other, first, last);
175+
}
129176

130177
_CCCL_HOST_DEVICE void set_allocator(const allocator_type& alloc);
131178

132-
_CCCL_HOST_DEVICE bool is_allocator_not_equal(const allocator_type& alloc) const;
133-
134-
_CCCL_HOST_DEVICE bool is_allocator_not_equal(const contiguous_storage& other) const;
135-
136-
_CCCL_HOST_DEVICE void propagate_allocator(const contiguous_storage& other);
179+
_CCCL_EXEC_CHECK_DISABLE
180+
_CCCL_HOST_DEVICE void propagate_allocator(const contiguous_storage& other)
181+
{
182+
_CCCL_IF_CONSTEXPR (allocator_traits<Alloc>::propagate_on_container_copy_assignment::value)
183+
{
184+
m_allocator = other.m_allocator;
185+
}
186+
}
137187

138-
_CCCL_HOST_DEVICE void propagate_allocator(contiguous_storage& other);
188+
_CCCL_EXEC_CHECK_DISABLE
189+
_CCCL_HOST_DEVICE void propagate_allocator(contiguous_storage& other)
190+
{
191+
_CCCL_IF_CONSTEXPR (allocator_traits<Alloc>::propagate_on_container_move_assignment::value)
192+
{
193+
m_allocator = ::cuda::std::move(other.m_allocator);
194+
}
195+
}
139196

140197
// allow move assignment for a sane implementation of allocator propagation
141-
// on move assignment
142198
_CCCL_HOST_DEVICE contiguous_storage& operator=(contiguous_storage&& other);
143199

144200
_CCCL_SYNTHESIZE_SEQUENCE_ACCESS(contiguous_storage, const_iterator);
@@ -151,34 +207,31 @@ class contiguous_storage
151207

152208
size_type m_size;
153209

154-
// disallow assignment
155-
contiguous_storage& operator=(const contiguous_storage& x);
156-
157-
_CCCL_HOST_DEVICE void swap_allocators(true_type, const allocator_type&);
158-
159-
_CCCL_HOST_DEVICE void swap_allocators(false_type, allocator_type&);
160-
161-
_CCCL_HOST_DEVICE bool is_allocator_not_equal_dispatch(true_type, const allocator_type&) const;
162-
163-
_CCCL_HOST_DEVICE bool is_allocator_not_equal_dispatch(false_type, const allocator_type&) const;
164-
165-
_CCCL_HOST_DEVICE void deallocate_on_allocator_mismatch_dispatch(true_type, const contiguous_storage& other) noexcept;
166-
167-
_CCCL_HOST_DEVICE void deallocate_on_allocator_mismatch_dispatch(false_type, const contiguous_storage& other) noexcept;
210+
_CCCL_EXEC_CHECK_DISABLE
211+
_CCCL_HOST_DEVICE void deallocate_on_allocator_mismatch_dispatch(true_type, const contiguous_storage& other) noexcept
212+
{
213+
if (m_allocator != other.m_allocator)
214+
{
215+
deallocate();
216+
}
217+
}
168218

169-
_CCCL_HOST_DEVICE void destroy_on_allocator_mismatch_dispatch(
170-
true_type, const contiguous_storage& other, iterator first, iterator last) noexcept;
219+
_CCCL_HOST_DEVICE static void deallocate_on_allocator_mismatch_dispatch(false_type, const contiguous_storage&) noexcept
220+
{}
171221

222+
_CCCL_EXEC_CHECK_DISABLE
172223
_CCCL_HOST_DEVICE void destroy_on_allocator_mismatch_dispatch(
173-
false_type, const contiguous_storage& other, iterator first, iterator last) noexcept;
174-
175-
_CCCL_HOST_DEVICE void propagate_allocator_dispatch(true_type, const contiguous_storage& other);
176-
177-
_CCCL_HOST_DEVICE void propagate_allocator_dispatch(false_type, const contiguous_storage& other);
178-
179-
_CCCL_HOST_DEVICE void propagate_allocator_dispatch(true_type, contiguous_storage& other);
180-
181-
_CCCL_HOST_DEVICE void propagate_allocator_dispatch(false_type, contiguous_storage& other);
224+
true_type, const contiguous_storage& other, iterator first, iterator last) noexcept
225+
{
226+
if (m_allocator != other.m_allocator)
227+
{
228+
destroy(first, last);
229+
}
230+
} // end contiguous_storage::destroy_on_allocator_mismatch()
231+
232+
_CCCL_HOST_DEVICE static void destroy_on_allocator_mismatch_dispatch(
233+
false_type, const contiguous_storage& other, iterator first, iterator last) noexcept
234+
{}
182235

183236
friend _CCCL_HOST_DEVICE void swap(contiguous_storage& lhs, contiguous_storage& rhs) noexcept(noexcept(lhs.swap(rhs)))
184237
{

0 commit comments

Comments
 (0)