Skip to content

Commit 92c8575

Browse files
Quuxplusonecopybara-github
authored andcommitted
PR #1618: inlined_vector: Use trivial relocation for SwapInlinedElements
Imported from GitHub PR #1618 I noticed while working on #1615 that `inlined_vector` could use the trivial relocatability trait here, too. Here the memcpy codepath already exists; we just have to opt in to using it. Merge 567a1dd into a7012a5 Merging this change closes #1618 COPYBARA_INTEGRATE_REVIEW=#1618 from Quuxplusone:trivial-swap 567a1dd PiperOrigin-RevId: 609019296 Change-Id: I4055ab790245752179e405b490fcd479e7389726
1 parent b0f85e2 commit 92c8575

File tree

2 files changed

+47
-17
lines changed

2 files changed

+47
-17
lines changed

absl/container/inlined_vector_test.cc

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,35 @@ TEST(UniquePtr, MoveAssign) {
304304
}
305305
}
306306

307+
// Swapping containers of unique pointers should work fine, with no
308+
// leaks, despite the fact that unique pointers are trivially relocatable but
309+
// not trivially destructible.
310+
// TODO(absl-team): Using unique_ptr here is technically correct, but
311+
// a trivially relocatable struct would be less semantically confusing.
312+
TEST(UniquePtr, Swap) {
313+
for (size_t size1 = 0; size1 < 5; ++size1) {
314+
for (size_t size2 = 0; size2 < 5; ++size2) {
315+
absl::InlinedVector<std::unique_ptr<size_t>, 2> a;
316+
absl::InlinedVector<std::unique_ptr<size_t>, 2> b;
317+
for (size_t i = 0; i < size1; ++i) {
318+
a.push_back(std::make_unique<size_t>(i + 10));
319+
}
320+
for (size_t i = 0; i < size2; ++i) {
321+
b.push_back(std::make_unique<size_t>(i + 20));
322+
}
323+
a.swap(b);
324+
ASSERT_THAT(a, SizeIs(size2));
325+
ASSERT_THAT(b, SizeIs(size1));
326+
for (size_t i = 0; i < a.size(); ++i) {
327+
ASSERT_THAT(a[i], Pointee(i + 20));
328+
}
329+
for (size_t i = 0; i < b.size(); ++i) {
330+
ASSERT_THAT(b[i], Pointee(i + 10));
331+
}
332+
}
333+
}
334+
}
335+
307336
// At the end of this test loop, the elements between [erase_begin, erase_end)
308337
// should have reference counts == 0, and all others elements should have
309338
// reference counts == 1.
@@ -783,7 +812,9 @@ TEST(OverheadTest, Storage) {
783812
// The union should be absorbing some of the allocation bookkeeping overhead
784813
// in the larger vectors, leaving only the size_ field as overhead.
785814

786-
struct T { void* val; };
815+
struct T {
816+
void* val;
817+
};
787818
size_t expected_overhead = sizeof(T);
788819

789820
EXPECT_EQ((2 * expected_overhead),

absl/container/internal/inlined_vector.h

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -322,14 +322,13 @@ class Storage {
322322

323323
// The policy to be used specifically when swapping inlined elements.
324324
using SwapInlinedElementsPolicy = absl::conditional_t<
325-
// Fast path: if the value type can be trivially move constructed/assigned
326-
// and destroyed, and we know the allocator doesn't do anything fancy,
327-
// then it's safe for us to simply swap the bytes in the inline storage.
328-
// It's as if we had move-constructed a temporary vector, move-assigned
329-
// one to the other, then move-assigned the first from the temporary.
330-
absl::conjunction<absl::is_trivially_move_constructible<ValueType<A>>,
331-
absl::is_trivially_move_assignable<ValueType<A>>,
332-
absl::is_trivially_destructible<ValueType<A>>,
325+
// Fast path: if the value type can be trivially relocated, and we
326+
// know the allocator doesn't do anything fancy, then it's safe for us
327+
// to simply swap the bytes in the inline storage. It's as if we had
328+
// relocated the first vector's elements into temporary storage,
329+
// relocated the second's elements into the (now-empty) first's,
330+
// and then relocated from temporary storage into the second.
331+
absl::conjunction<absl::is_trivially_relocatable<ValueType<A>>,
333332
std::is_same<A, std::allocator<ValueType<A>>>>::value,
334333
MemcpyPolicy,
335334
absl::conditional_t<IsSwapOk<A>::value, ElementwiseSwapPolicy,
@@ -624,8 +623,8 @@ void Storage<T, N, A>::InitFrom(const Storage& other) {
624623

625624
template <typename T, size_t N, typename A>
626625
template <typename ValueAdapter>
627-
auto Storage<T, N, A>::Initialize(ValueAdapter values, SizeType<A> new_size)
628-
-> void {
626+
auto Storage<T, N, A>::Initialize(ValueAdapter values,
627+
SizeType<A> new_size) -> void {
629628
// Only callable from constructors!
630629
ABSL_HARDENING_ASSERT(!GetIsAllocated());
631630
ABSL_HARDENING_ASSERT(GetSize() == 0);
@@ -656,8 +655,8 @@ auto Storage<T, N, A>::Initialize(ValueAdapter values, SizeType<A> new_size)
656655

657656
template <typename T, size_t N, typename A>
658657
template <typename ValueAdapter>
659-
auto Storage<T, N, A>::Assign(ValueAdapter values, SizeType<A> new_size)
660-
-> void {
658+
auto Storage<T, N, A>::Assign(ValueAdapter values,
659+
SizeType<A> new_size) -> void {
661660
StorageView<A> storage_view = MakeStorageView();
662661

663662
AllocationTransaction<A> allocation_tx(GetAllocator());
@@ -699,8 +698,8 @@ auto Storage<T, N, A>::Assign(ValueAdapter values, SizeType<A> new_size)
699698

700699
template <typename T, size_t N, typename A>
701700
template <typename ValueAdapter>
702-
auto Storage<T, N, A>::Resize(ValueAdapter values, SizeType<A> new_size)
703-
-> void {
701+
auto Storage<T, N, A>::Resize(ValueAdapter values,
702+
SizeType<A> new_size) -> void {
704703
StorageView<A> storage_view = MakeStorageView();
705704
Pointer<A> const base = storage_view.data;
706705
const SizeType<A> size = storage_view.size;
@@ -885,8 +884,8 @@ auto Storage<T, N, A>::EmplaceBackSlow(Args&&... args) -> Reference<A> {
885884
}
886885

887886
template <typename T, size_t N, typename A>
888-
auto Storage<T, N, A>::Erase(ConstIterator<A> from, ConstIterator<A> to)
889-
-> Iterator<A> {
887+
auto Storage<T, N, A>::Erase(ConstIterator<A> from,
888+
ConstIterator<A> to) -> Iterator<A> {
890889
StorageView<A> storage_view = MakeStorageView();
891890

892891
auto erase_size = static_cast<SizeType<A>>(std::distance(from, to));

0 commit comments

Comments
 (0)