From ad1485c8986246b2ae9105e512738d0e97aec887 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Tue, 23 Jul 2019 12:42:09 -0700 Subject: Export of internal Abseil changes. -- 1f44f8f487aa3afe8248132e4081519e85671965 by CJ Johnson : Updates ScopedAllocatorWorks test for InlinedVector to not depend on specific byte counts of standard library vectors. It's too brittle in the face of capacity-changing changes to InlinedVector and does not provide signal in those breakages. PiperOrigin-RevId: 259590332 -- fef7589547e9cdd04a254f6ae06e2bd9ec2b35f0 by CJ Johnson : Updates the implementation of InlinedVector::insert(...) to be exception safe and adds an exception safety tests for insert(...) PiperOrigin-RevId: 259542968 GitOrigin-RevId: 1f44f8f487aa3afe8248132e4081519e85671965 Change-Id: I514beff56159c9c717f8d29197728011af1fecd7 --- absl/container/inlined_vector.h | 168 +++------------------ .../inlined_vector_exception_safety_test.cc | 76 ++++++++++ absl/container/inlined_vector_test.cc | 101 ++++++------- absl/container/internal/inlined_vector.h | 97 ++++++++++++ 4 files changed, 236 insertions(+), 206 deletions(-) diff --git a/absl/container/inlined_vector.h b/absl/container/inlined_vector.h index 7552723d..84ac67eb 100644 --- a/absl/container/inlined_vector.h +++ b/absl/container/inlined_vector.h @@ -549,15 +549,15 @@ class InlinedVector { // of `v` starting at `pos`. Returns an `iterator` pointing to the first of // the newly inserted elements. iterator insert(const_iterator pos, size_type n, const_reference v) { - assert(pos >= begin() && pos <= end()); - if (ABSL_PREDICT_FALSE(n == 0)) { + assert(pos >= begin()); + assert(pos <= end()); + + if (ABSL_PREDICT_TRUE(n != 0)) { + value_type dealias = v; + return storage_.Insert(pos, CopyValueAdapter(dealias), n); + } else { return const_cast(pos); } - value_type copy = v; - std::pair it_pair = ShiftRight(pos, n); - std::fill(it_pair.first, it_pair.second, copy); - UninitializedFill(it_pair.second, it_pair.first + n, copy); - return it_pair.first; } // Overload of `InlinedVector::insert()` for copying the contents of the @@ -577,17 +577,15 @@ class InlinedVector { EnableIfAtLeastForwardIterator* = nullptr> iterator insert(const_iterator pos, ForwardIterator first, ForwardIterator last) { - assert(pos >= begin() && pos <= end()); - if (ABSL_PREDICT_FALSE(first == last)) { + assert(pos >= begin()); + assert(pos <= end()); + + if (ABSL_PREDICT_TRUE(first != last)) { + return storage_.Insert(pos, IteratorValueAdapter(first), + std::distance(first, last)); + } else { return const_cast(pos); } - auto n = std::distance(first, last); - std::pair it_pair = ShiftRight(pos, n); - size_type used_spots = it_pair.second - it_pair.first; - auto open_spot = std::next(first, used_spots); - std::copy(first, open_spot, it_pair.first); - UninitializedCopy(open_spot, last, it_pair.second); - return it_pair.first; } // Overload of `InlinedVector::insert()` for inserting elements constructed @@ -615,23 +613,12 @@ class InlinedVector { iterator emplace(const_iterator pos, Args&&... args) { assert(pos >= begin()); assert(pos <= end()); - if (ABSL_PREDICT_FALSE(pos == end())) { - emplace_back(std::forward(args)...); - return end() - 1; - } - T new_t = T(std::forward(args)...); - - auto range = ShiftRight(pos, 1); - if (range.first == range.second) { - // constructing into uninitialized memory - Construct(range.first, std::move(new_t)); - } else { - // assigning into moved-from object - *range.first = T(std::move(new_t)); - } - - return range.first; + value_type dealias(std::forward(args)...); + return storage_.Insert(pos, + IteratorValueAdapter( + MoveIterator(std::addressof(dealias))), + 1); } // `InlinedVector::emplace_back()` @@ -746,123 +733,6 @@ class InlinedVector { template friend H AbslHashValue(H h, const absl::InlinedVector& a); - void ResetAllocation(pointer new_data, size_type new_capacity, - size_type new_size) { - if (storage_.GetIsAllocated()) { - Destroy(storage_.GetAllocatedData(), - storage_.GetAllocatedData() + size()); - assert(begin() == storage_.GetAllocatedData()); - AllocatorTraits::deallocate(*storage_.GetAllocPtr(), - storage_.GetAllocatedData(), - storage_.GetAllocatedCapacity()); - } else { - Destroy(storage_.GetInlinedData(), storage_.GetInlinedData() + size()); - } - - storage_.SetAllocatedData(new_data, new_capacity); - storage_.SetAllocatedSize(new_size); - } - - template - reference Construct(pointer p, Args&&... args) { - absl::allocator_traits::construct( - *storage_.GetAllocPtr(), p, std::forward(args)...); - return *p; - } - - template - void UninitializedCopy(Iterator src, Iterator src_last, pointer dst) { - for (; src != src_last; ++dst, ++src) Construct(dst, *src); - } - - template - void UninitializedFill(pointer dst, pointer dst_last, const Args&... args) { - for (; dst != dst_last; ++dst) Construct(dst, args...); - } - - // Destroy [`from`, `to`) in place. - void Destroy(pointer from, pointer to) { - for (pointer cur = from; cur != to; ++cur) { - absl::allocator_traits::destroy(*storage_.GetAllocPtr(), - cur); - } -#if !defined(NDEBUG) - // Overwrite unused memory with `0xab` so we can catch uninitialized usage. - // Cast to `void*` to tell the compiler that we don't care that we might be - // scribbling on a vtable pointer. - if (from != to) { - auto len = sizeof(value_type) * std::distance(from, to); - std::memset(reinterpret_cast(from), 0xab, len); - } -#endif // !defined(NDEBUG) - } - - // Shift all elements from `position` to `end()` by `n` places to the right. - // If the vector needs to be enlarged, memory will be allocated. - // Returns `iterator`s pointing to the start of the previously-initialized - // portion and the start of the uninitialized portion of the created gap. - // The number of initialized spots is `pair.second - pair.first`. The number - // of raw spots is `n - (pair.second - pair.first)`. - // - // Updates the size of the InlinedVector internally. - std::pair ShiftRight(const_iterator position, - size_type n) { - iterator start_used = const_cast(position); - iterator start_raw = const_cast(position); - size_type s = size(); - size_type required_size = s + n; - - if (required_size > capacity()) { - // Compute new capacity by repeatedly doubling current capacity - size_type new_capacity = capacity(); - while (new_capacity < required_size) { - new_capacity <<= 1; - } - // Move everyone into the new allocation, leaving a gap of `n` for the - // requested shift. - pointer new_data = - AllocatorTraits::allocate(*storage_.GetAllocPtr(), new_capacity); - size_type index = position - begin(); - UninitializedCopy(std::make_move_iterator(data()), - std::make_move_iterator(data() + index), new_data); - UninitializedCopy(std::make_move_iterator(data() + index), - std::make_move_iterator(data() + s), - new_data + index + n); - ResetAllocation(new_data, new_capacity, s); - - // New allocation means our iterator is invalid, so we'll recalculate. - // Since the entire gap is in new space, there's no used space to reuse. - start_raw = begin() + index; - start_used = start_raw; - } else { - // If we had enough space, it's a two-part move. Elements going into - // previously-unoccupied space need an `UninitializedCopy()`. Elements - // going into a previously-occupied space are just a `std::move()`. - iterator pos = const_cast(position); - iterator raw_space = end(); - size_type slots_in_used_space = raw_space - pos; - size_type new_elements_in_used_space = (std::min)(n, slots_in_used_space); - size_type new_elements_in_raw_space = n - new_elements_in_used_space; - size_type old_elements_in_used_space = - slots_in_used_space - new_elements_in_used_space; - - UninitializedCopy( - std::make_move_iterator(pos + old_elements_in_used_space), - std::make_move_iterator(raw_space), - raw_space + new_elements_in_raw_space); - std::move_backward(pos, pos + old_elements_in_used_space, raw_space); - - // If the gap is entirely in raw space, the used space starts where the - // raw space starts, leaving no elements in used space. If the gap is - // entirely in used space, the raw space starts at the end of the gap, - // leaving all elements accounted for within the used space. - start_used = pos; - start_raw = pos + new_elements_in_used_space; - } - storage_.AddSize(n); - return std::make_pair(start_used, start_raw); - } - Storage storage_; }; diff --git a/absl/container/inlined_vector_exception_safety_test.cc b/absl/container/inlined_vector_exception_safety_test.cc index 2d1ce485..ff0da75b 100644 --- a/absl/container/inlined_vector_exception_safety_test.cc +++ b/absl/container/inlined_vector_exception_safety_test.cc @@ -279,6 +279,82 @@ TYPED_TEST(TwoSizeTest, Resize) { })); } +TYPED_TEST(OneSizeTest, Insert) { + using VecT = typename TypeParam::VecT; + using value_type = typename VecT::value_type; + constexpr static auto from_size = TypeParam::GetSizeAt(0); + + auto tester = testing::MakeExceptionSafetyTester() + .WithInitialValue(VecT{from_size}) + .WithContracts(InlinedVectorInvariants); + + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin(); + vec->insert(it, value_type{}); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin() + (vec->size() / 2); + vec->insert(it, value_type{}); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->end(); + vec->insert(it, value_type{}); + })); +} + +TYPED_TEST(TwoSizeTest, Insert) { + using VecT = typename TypeParam::VecT; + using value_type = typename VecT::value_type; + constexpr static auto from_size = TypeParam::GetSizeAt(0); + constexpr static auto count = TypeParam::GetSizeAt(1); + + auto tester = testing::MakeExceptionSafetyTester() + .WithInitialValue(VecT{from_size}) + .WithContracts(InlinedVectorInvariants); + + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin(); + vec->insert(it, count, value_type{}); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin() + (vec->size() / 2); + vec->insert(it, count, value_type{}); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->end(); + vec->insert(it, count, value_type{}); + })); + + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin(); + vec->insert(it, ABSL_INTERNAL_MAKE_INIT_LIST(value_type, count)); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin() + (vec->size() / 2); + vec->insert(it, ABSL_INTERNAL_MAKE_INIT_LIST(value_type, count)); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->end(); + vec->insert(it, ABSL_INTERNAL_MAKE_INIT_LIST(value_type, count)); + })); + + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin(); + std::array arr{}; + vec->insert(it, arr.begin(), arr.end()); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->begin() + (vec->size() / 2); + std::array arr{}; + vec->insert(it, arr.begin(), arr.end()); + })); + EXPECT_TRUE(tester.Test([](VecT* vec) { + auto it = vec->end(); + std::array arr{}; + vec->insert(it, arr.begin(), arr.end()); + })); +} + TYPED_TEST(OneSizeTest, EmplaceBack) { using VecT = typename TypeParam::VecT; constexpr static auto size = TypeParam::GetSizeAt(0); diff --git a/absl/container/inlined_vector_test.cc b/absl/container/inlined_vector_test.cc index 76c470f3..bada4fec 100644 --- a/absl/container/inlined_vector_test.cc +++ b/absl/container/inlined_vector_test.cc @@ -1675,66 +1675,53 @@ TEST(AllocatorSupportTest, SwapOneAllocated) { EXPECT_THAT(allocated2, 0); } -TEST(AllocatorSupportTest, ScopedAllocatorWorks) { +TEST(AllocatorSupportTest, ScopedAllocatorWorksInlined) { using StdVector = std::vector>; - using MyAlloc = std::scoped_allocator_adaptor>; - using AllocVec = absl::InlinedVector; - - // MSVC 2017's std::vector allocates different amounts of memory in debug - // versus opt mode. - int64_t test_allocated = 0; - StdVector v(CountingAllocator{&test_allocated}); - // The amount of memory allocated by a default constructed vector - auto default_std_vec_allocated = test_allocated; - v.push_back(1); - // The amound of memory allocated by a copy-constructed vector with one - // element. - int64_t one_element_std_vec_copy_allocated = test_allocated; + using Alloc = CountingAllocator; + using ScopedAlloc = std::scoped_allocator_adaptor; + using AllocVec = absl::InlinedVector; - int64_t allocated = 0; - AllocVec vec(MyAlloc{CountingAllocator{&allocated}}); - EXPECT_EQ(allocated, 0); + int64_t total_allocated_byte_count = 0; - // This default constructs a vector, but the allocator should pass itself - // into the vector, so check allocation compared to that. - // The absl::InlinedVector does not allocate any memory. - // The vector may allocate any memory. - auto expected = default_std_vec_allocated; - vec.resize(1); - EXPECT_EQ(allocated, expected); - - // We make vector allocate memory. - // It must go through the allocator even though we didn't construct the - // vector directly. This assumes that vec[0] doesn't need to grow its - // allocation. - expected += sizeof(int); - vec[0].push_back(1); - EXPECT_EQ(allocated, expected); - - // Another allocating vector. - expected += one_element_std_vec_copy_allocated; - vec.push_back(vec[0]); - EXPECT_EQ(allocated, expected); - - // Overflow the inlined memory. - // The absl::InlinedVector will now allocate. - expected += sizeof(StdVector) * 8 + default_std_vec_allocated * 3; - vec.resize(5); - EXPECT_EQ(allocated, expected); - - // Adding one more in external mode should also work. - expected += one_element_std_vec_copy_allocated; - vec.push_back(vec[0]); - EXPECT_EQ(allocated, expected); - - // And extending these should still work. This assumes that vec[0] does not - // need to grow its allocation. - expected += sizeof(int); - vec[0].push_back(1); - EXPECT_EQ(allocated, expected); - - vec.clear(); - EXPECT_EQ(allocated, 0); + AllocVec inlined_case(ScopedAlloc(Alloc(+&total_allocated_byte_count))); + + // Called only once to remain inlined + inlined_case.emplace_back(); + + int64_t absl_responsible_for_count = total_allocated_byte_count; + EXPECT_EQ(absl_responsible_for_count, 0); + + inlined_case[0].emplace_back(); + EXPECT_GT(total_allocated_byte_count, absl_responsible_for_count); + + inlined_case.clear(); + inlined_case.shrink_to_fit(); + EXPECT_EQ(total_allocated_byte_count, 0); +} + +TEST(AllocatorSupportTest, ScopedAllocatorWorksAllocated) { + using StdVector = std::vector>; + using Alloc = CountingAllocator; + using ScopedAlloc = std::scoped_allocator_adaptor; + using AllocVec = absl::InlinedVector; + + int64_t total_allocated_byte_count = 0; + + AllocVec allocated_case(ScopedAlloc(Alloc(+&total_allocated_byte_count))); + + // Called twice to force into being allocated + allocated_case.emplace_back(); + allocated_case.emplace_back(); + + int64_t absl_responsible_for_count = total_allocated_byte_count; + EXPECT_GT(absl_responsible_for_count, 0); + + allocated_case[1].emplace_back(); + EXPECT_GT(total_allocated_byte_count, absl_responsible_for_count); + + allocated_case.clear(); + allocated_case.shrink_to_fit(); + EXPECT_EQ(total_allocated_byte_count, 0); } TEST(AllocatorSupportTest, SizeAllocConstructor) { diff --git a/absl/container/internal/inlined_vector.h b/absl/container/internal/inlined_vector.h index 0ab2d7da..7954b2b5 100644 --- a/absl/container/internal/inlined_vector.h +++ b/absl/container/internal/inlined_vector.h @@ -380,6 +380,10 @@ class Storage { template void Resize(ValueAdapter values, size_type new_size); + template + iterator Insert(const_iterator pos, ValueAdapter values, + size_type insert_count); + template reference EmplaceBack(Args&&... args); @@ -563,6 +567,99 @@ auto Storage::Resize(ValueAdapter values, size_type new_size) -> void { SetSize(new_size); } +template +template +auto Storage::Insert(const_iterator pos, ValueAdapter values, + size_type insert_count) -> iterator { + StorageView storage_view = MakeStorageView(); + + size_type insert_index = + std::distance(const_iterator(storage_view.data), pos); + size_type insert_end_index = insert_index + insert_count; + size_type new_size = storage_view.size + insert_count; + + if (new_size > storage_view.capacity) { + AllocationTransaction allocation_tx(GetAllocPtr()); + ConstructionTransaction construction_tx(GetAllocPtr()); + ConstructionTransaction move_construciton_tx(GetAllocPtr()); + + IteratorValueAdapter move_values( + MoveIterator(storage_view.data)); + + pointer new_data = allocation_tx.Allocate( + LegacyNextCapacityFrom(storage_view.capacity, new_size)); + + construction_tx.Construct(new_data + insert_index, &values, insert_count); + + move_construciton_tx.Construct(new_data, &move_values, insert_index); + + inlined_vector_internal::ConstructElements( + GetAllocPtr(), new_data + insert_end_index, &move_values, + storage_view.size - insert_index); + + inlined_vector_internal::DestroyElements(GetAllocPtr(), storage_view.data, + storage_view.size); + + construction_tx.Commit(); + move_construciton_tx.Commit(); + DeallocateIfAllocated(); + AcquireAllocation(&allocation_tx); + + SetAllocatedSize(new_size); + return iterator(new_data + insert_index); + } else { + size_type move_construction_destination_index = + (std::max)(insert_end_index, storage_view.size); + + ConstructionTransaction move_construction_tx(GetAllocPtr()); + + IteratorValueAdapter move_construction_values( + MoveIterator(storage_view.data + + (move_construction_destination_index - insert_count))); + absl::Span move_construction = { + storage_view.data + move_construction_destination_index, + new_size - move_construction_destination_index}; + + pointer move_assignment_values = storage_view.data + insert_index; + absl::Span move_assignment = { + storage_view.data + insert_end_index, + move_construction_destination_index - insert_end_index}; + + absl::Span insert_assignment = {move_assignment_values, + move_construction.size()}; + + absl::Span insert_construction = { + insert_assignment.data() + insert_assignment.size(), + insert_count - insert_assignment.size()}; + + move_construction_tx.Construct(move_construction.data(), + &move_construction_values, + move_construction.size()); + + for (pointer destination = move_assignment.data() + move_assignment.size(), + last_destination = move_assignment.data(), + source = move_assignment_values + move_assignment.size(); + ;) { + --destination; + --source; + if (destination < last_destination) break; + *destination = std::move(*source); + } + + inlined_vector_internal::AssignElements(insert_assignment.data(), &values, + insert_assignment.size()); + + inlined_vector_internal::ConstructElements( + GetAllocPtr(), insert_construction.data(), &values, + insert_construction.size()); + + move_construction_tx.Commit(); + + AddSize(insert_count); + return iterator(storage_view.data + insert_index); + } +} + template template auto Storage::EmplaceBack(Args&&... args) -> reference { -- cgit v1.2.3