From 7b0d4084cbab666ae923005d70ac6dc8754dd72f Mon Sep 17 00:00:00 2001 From: Arthur O'Dwyer Date: Wed, 21 Feb 2024 09:17:54 -0800 Subject: [PATCH] PR #1618: inlined_vector: Use trivial relocation for `SwapInlinedElements` Imported from GitHub PR https://github.com/abseil/abseil-cpp/pull/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 567a1dd9b6b3352f649e900b24834b59e39cfa14 into a7012a5bfcf26a41b9dd32d4c429004773503dd6 Merging this change closes #1618 COPYBARA_INTEGRATE_REVIEW=https://github.com/abseil/abseil-cpp/pull/1618 from Quuxplusone:trivial-swap 567a1dd9b6b3352f649e900b24834b59e39cfa14 PiperOrigin-RevId: 609019296 Change-Id: I4055ab790245752179e405b490fcd479e7389726 --- absl/container/inlined_vector_test.cc | 33 +++++++++++++++++++++++- absl/container/internal/inlined_vector.h | 31 +++++++++++----------- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/absl/container/inlined_vector_test.cc b/absl/container/inlined_vector_test.cc index 241389aef85..5ecf88a978c 100644 --- a/absl/container/inlined_vector_test.cc +++ b/absl/container/inlined_vector_test.cc @@ -304,6 +304,35 @@ TEST(UniquePtr, MoveAssign) { } } +// Swapping containers of unique pointers should work fine, with no +// leaks, despite the fact that unique pointers are trivially relocatable but +// not trivially destructible. +// TODO(absl-team): Using unique_ptr here is technically correct, but +// a trivially relocatable struct would be less semantically confusing. +TEST(UniquePtr, Swap) { + for (size_t size1 = 0; size1 < 5; ++size1) { + for (size_t size2 = 0; size2 < 5; ++size2) { + absl::InlinedVector, 2> a; + absl::InlinedVector, 2> b; + for (size_t i = 0; i < size1; ++i) { + a.push_back(std::make_unique(i + 10)); + } + for (size_t i = 0; i < size2; ++i) { + b.push_back(std::make_unique(i + 20)); + } + a.swap(b); + ASSERT_THAT(a, SizeIs(size2)); + ASSERT_THAT(b, SizeIs(size1)); + for (size_t i = 0; i < a.size(); ++i) { + ASSERT_THAT(a[i], Pointee(i + 20)); + } + for (size_t i = 0; i < b.size(); ++i) { + ASSERT_THAT(b[i], Pointee(i + 10)); + } + } + } +} + // At the end of this test loop, the elements between [erase_begin, erase_end) // should have reference counts == 0, and all others elements should have // reference counts == 1. @@ -783,7 +812,9 @@ TEST(OverheadTest, Storage) { // The union should be absorbing some of the allocation bookkeeping overhead // in the larger vectors, leaving only the size_ field as overhead. - struct T { void* val; }; + struct T { + void* val; + }; size_t expected_overhead = sizeof(T); EXPECT_EQ((2 * expected_overhead), diff --git a/absl/container/internal/inlined_vector.h b/absl/container/internal/inlined_vector.h index 0eb9c34ddd5..90a74dc7ffa 100644 --- a/absl/container/internal/inlined_vector.h +++ b/absl/container/internal/inlined_vector.h @@ -322,14 +322,13 @@ class Storage { // The policy to be used specifically when swapping inlined elements. using SwapInlinedElementsPolicy = absl::conditional_t< - // Fast path: if the value type can be trivially move constructed/assigned - // and destroyed, and we know the allocator doesn't do anything fancy, - // then it's safe for us to simply swap the bytes in the inline storage. - // It's as if we had move-constructed a temporary vector, move-assigned - // one to the other, then move-assigned the first from the temporary. - absl::conjunction>, - absl::is_trivially_move_assignable>, - absl::is_trivially_destructible>, + // Fast path: if the value type can be trivially relocated, and we + // know the allocator doesn't do anything fancy, then it's safe for us + // to simply swap the bytes in the inline storage. It's as if we had + // relocated the first vector's elements into temporary storage, + // relocated the second's elements into the (now-empty) first's, + // and then relocated from temporary storage into the second. + absl::conjunction>, std::is_same>>>::value, MemcpyPolicy, absl::conditional_t::value, ElementwiseSwapPolicy, @@ -624,8 +623,8 @@ void Storage::InitFrom(const Storage& other) { template template -auto Storage::Initialize(ValueAdapter values, SizeType new_size) - -> void { +auto Storage::Initialize(ValueAdapter values, + SizeType new_size) -> void { // Only callable from constructors! ABSL_HARDENING_ASSERT(!GetIsAllocated()); ABSL_HARDENING_ASSERT(GetSize() == 0); @@ -656,8 +655,8 @@ auto Storage::Initialize(ValueAdapter values, SizeType new_size) template template -auto Storage::Assign(ValueAdapter values, SizeType new_size) - -> void { +auto Storage::Assign(ValueAdapter values, + SizeType new_size) -> void { StorageView storage_view = MakeStorageView(); AllocationTransaction allocation_tx(GetAllocator()); @@ -699,8 +698,8 @@ auto Storage::Assign(ValueAdapter values, SizeType new_size) template template -auto Storage::Resize(ValueAdapter values, SizeType new_size) - -> void { +auto Storage::Resize(ValueAdapter values, + SizeType new_size) -> void { StorageView storage_view = MakeStorageView(); Pointer const base = storage_view.data; const SizeType size = storage_view.size; @@ -885,8 +884,8 @@ auto Storage::EmplaceBackSlow(Args&&... args) -> Reference { } template -auto Storage::Erase(ConstIterator from, ConstIterator to) - -> Iterator { +auto Storage::Erase(ConstIterator from, + ConstIterator to) -> Iterator { StorageView storage_view = MakeStorageView(); auto erase_size = static_cast>(std::distance(from, to));