From 0576f22a67162eceb470c9946f9fd58ed24f0536 Mon Sep 17 00:00:00 2001 From: Guillermo Venegas Date: Wed, 11 Dec 2024 17:43:44 +0100 Subject: [PATCH] BLI: allow extending a Vector with another Vector with move-semantics This enables moving elements form one vector to another. Usually this is being doing by extending a vector with the content from the secondary vector and then clearing the secondary vector. However sometimes this being performed to transfer ownership of managed elements, if elements are copied from the secondary vector, but not cleared, this could lead to 2 vectors to share ownership of objects. Pull Request: https://projects.blender.org/blender/blender/pulls/131560 --- source/blender/blenlib/BLI_vector.hh | 23 ++++++- .../blender/blenlib/tests/BLI_vector_test.cc | 63 +++++++++++++++++++ source/blender/gpu/vulkan/vk_resource_pool.cc | 22 +++---- 3 files changed, 93 insertions(+), 15 deletions(-) diff --git a/source/blender/blenlib/BLI_vector.hh b/source/blender/blenlib/BLI_vector.hh index 8a947061da2..67f83264db3 100644 --- a/source/blender/blenlib/BLI_vector.hh +++ b/source/blender/blenlib/BLI_vector.hh @@ -566,6 +566,22 @@ class Vector { this->extend_unchecked(start, amount); } + /** + * Moves the elements of another vector to the end of this vector. + * + * This may result in reallocation to fit other vector elements, other vector will keep it + * buffer allocation, but it will become empty. + * This can be used in vectors that manages resources, allowing acquiring resources from another + * vector, preventing shared ownership of managed resources. + */ + template + void extend(Vector &&other) + { + BLI_assert(this != &other); + this->extend(std::make_move_iterator(other.begin()), std::make_move_iterator(other.end())); + other.clear(); + } + /** * Adds all elements from the array that are not already in the vector. This is an expensive * operation when the vector is large, but can be very cheap when it is known that the vector is @@ -651,7 +667,12 @@ class Vector { } try { - std::uninitialized_copy_n(first, insert_amount, begin_ + insert_index); + if constexpr (std::is_rvalue_reference_v) { + std::uninitialized_move_n(first, insert_amount, begin_ + insert_index); + } + else { + std::uninitialized_copy_n(first, insert_amount, begin_ + insert_index); + } } catch (...) { /* Destruct all values that have been moved. */ diff --git a/source/blender/blenlib/tests/BLI_vector_test.cc b/source/blender/blenlib/tests/BLI_vector_test.cc index 2d13e0a2075..140bdae65b9 100644 --- a/source/blender/blenlib/tests/BLI_vector_test.cc +++ b/source/blender/blenlib/tests/BLI_vector_test.cc @@ -468,6 +468,69 @@ TEST(vector, ExtendArray) EXPECT_EQ(a[1], 4); } +TEST(vector, ExtendMoveFromSmallVector) +{ + Vector> a = { + {1, 2, 3, 4, 5}, + {6, 7, 8}, + }; + Vector> b = { + {9, 10, 11, 12}, + {13, 14, 15, 16}, + }; + const Vector> c = { + {1, 2, 3, 4, 5}, + {6, 7, 8}, + {9, 10, 11, 12}, + {13, 14, 15, 16}, + }; + + a.extend(std::move(b)); + + EXPECT_EQ(a, c); + EXPECT_TRUE(b.is_empty()); +} + +TEST(vector, ExtendMoveFromUniquePtrVector) +{ + Vector ptr_vec; + + Vector> a; + a.append(std::make_unique(0)); + a.append(std::make_unique(1)); + a.append(std::make_unique(2)); + + for (std::unique_ptr &i : a) { + ptr_vec.append(i.get()); + } + + Vector> b; + b.append(std::make_unique(7)); + b.append(std::make_unique(8)); + b.append(std::make_unique(9)); + b.append(std::make_unique(20)); + + for (std::unique_ptr &i : b) { + ptr_vec.append(i.get()); + } + + ASSERT_EQ(a.size(), 3); + ASSERT_EQ(b.size(), 4); + + a.extend(std::move(b)); + + std::array values = {0, 1, 2, 7, 8, 9, 20}; + + ASSERT_EQ(size_t(a.size()), values.size()); + ASSERT_TRUE(b.is_empty()); + ASSERT_EQ(a.size(), ptr_vec.size()); + + for (int64_t i = 0; i < a.size(); i++) { + ASSERT_EQ(*a[i], values[size_t(i)]); + ASSERT_EQ(a[i].get(), ptr_vec[i]); + } +} + TEST(vector, Last) { Vector a{3, 5, 7}; diff --git a/source/blender/gpu/vulkan/vk_resource_pool.cc b/source/blender/gpu/vulkan/vk_resource_pool.cc index 9761143ce0d..9bbbe197f6c 100644 --- a/source/blender/gpu/vulkan/vk_resource_pool.cc +++ b/source/blender/gpu/vulkan/vk_resource_pool.cc @@ -37,20 +37,14 @@ void VKDiscardPool::move_data(VKDiscardPool &src_pool) { std::scoped_lock mutex(mutex_); std::scoped_lock mutex_src(src_pool.mutex_); - buffers_.extend(src_pool.buffers_); - image_views_.extend(src_pool.image_views_); - images_.extend(src_pool.images_); - shader_modules_.extend(src_pool.shader_modules_); - pipeline_layouts_.extend(src_pool.pipeline_layouts_); - framebuffers_.extend(src_pool.framebuffers_); - render_passes_.extend(src_pool.render_passes_); - src_pool.buffers_.clear(); - src_pool.image_views_.clear(); - src_pool.images_.clear(); - src_pool.shader_modules_.clear(); - src_pool.pipeline_layouts_.clear(); - src_pool.framebuffers_.clear(); - src_pool.render_passes_.clear(); + buffers_.extend(std::move(src_pool.buffers_)); + image_views_.extend(std::move(src_pool.image_views_)); + images_.extend(std::move(src_pool.images_)); + shader_modules_.extend(std::move(src_pool.shader_modules_)); + pipeline_layouts_.extend(std::move(src_pool.pipeline_layouts_)); + framebuffers_.extend(std::move(src_pool.framebuffers_)); + render_passes_.extend(std::move(src_pool.render_passes_)); + for (const Map>::Item &item : src_pool.command_buffers_.items()) {