From c62a6ea1ef515873c9e923ffd7ee526972f87165 Mon Sep 17 00:00:00 2001 From: Hartmut Kaiser Date: Mon, 16 Oct 2023 09:12:24 -0500 Subject: [PATCH] Making sure serialize_buffer properly destroys buffer, if needed. --- examples/1d_stencil/1d_stencil_6.cpp | 4 +- examples/1d_stencil/1d_stencil_7.cpp | 4 +- examples/1d_stencil/1d_stencil_8.cpp | 4 +- examples/quickstart/zerocopy_rdma.cpp | 19 +- .../hpx/serialization/serialize_buffer.hpp | 177 +++++++++++++----- .../examples/1d_stencil_4_checkpoint.cpp | 7 +- 6 files changed, 145 insertions(+), 70 deletions(-) diff --git a/examples/1d_stencil/1d_stencil_6.cpp b/examples/1d_stencil/1d_stencil_6.cpp index 05154d56b92c..8dee84edae62 100644 --- a/examples/1d_stencil/1d_stencil_6.cpp +++ b/examples/1d_stencil/1d_stencil_6.cpp @@ -65,7 +65,7 @@ struct partition_data // Create a new (uninitialized) partition of the given size. explicit partition_data(std::size_t size) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) , min_index_(0) { @@ -73,7 +73,7 @@ struct partition_data // Create a new (initialized) partition of the given size. partition_data(std::size_t size, double initial_value) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) , min_index_(0) { diff --git a/examples/1d_stencil/1d_stencil_7.cpp b/examples/1d_stencil/1d_stencil_7.cpp index 71971fb927ef..f3c05da7de57 100644 --- a/examples/1d_stencil/1d_stencil_7.cpp +++ b/examples/1d_stencil/1d_stencil_7.cpp @@ -66,7 +66,7 @@ struct partition_data // Create a new (uninitialized) partition of the given size. explicit partition_data(std::size_t size) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) , min_index_(0) { @@ -74,7 +74,7 @@ struct partition_data // Create a new (initialized) partition of the given size. partition_data(std::size_t size, double initial_value) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) , min_index_(0) { diff --git a/examples/1d_stencil/1d_stencil_8.cpp b/examples/1d_stencil/1d_stencil_8.cpp index d01c227c3724..4efb9dc02c93 100644 --- a/examples/1d_stencil/1d_stencil_8.cpp +++ b/examples/1d_stencil/1d_stencil_8.cpp @@ -105,12 +105,12 @@ struct partition_data struct hold_reference { - hold_reference(buffer_type const& data) + explicit hold_reference(buffer_type const& data) : data_(data) { } - void operator()(double*) {} // no deletion necessary + void operator()(double const*) const {} // no deletion necessary buffer_type data_; }; diff --git a/examples/quickstart/zerocopy_rdma.cpp b/examples/quickstart/zerocopy_rdma.cpp index 9ff8e8db6803..6205a4d31ad8 100644 --- a/examples/quickstart/zerocopy_rdma.cpp +++ b/examples/quickstart/zerocopy_rdma.cpp @@ -49,11 +49,11 @@ class pointer_allocator { } - pointer address(reference value) const + static pointer address(reference value) { return &value; } - const_pointer address(const_reference value) const + static const_pointer address(const_reference value) { return &value; } @@ -118,7 +118,7 @@ struct zerocopy_server : hpx::components::component_base } public: - zerocopy_server(std::size_t size = 0) + explicit zerocopy_server(std::size_t size = 0) : data_(size, 3.1415) { } @@ -127,7 +127,7 @@ struct zerocopy_server : hpx::components::component_base // Retrieve an array of doubles to the given address transfer_buffer_type get_here(std::size_t size, std::size_t remote_buffer) { - pointer_allocator allocator( + pointer_allocator const allocator( reinterpret_cast(remote_buffer), size); // lock the mutex, will be unlocked by the transfer buffer's deleter @@ -225,9 +225,7 @@ struct zerocopy : hpx::components::client_base /////////////////////////////////////////////////////////////////////////////// int main() { - std::vector localities = hpx::find_all_localities(); - - for (hpx::id_type const& id : localities) + for (hpx::id_type const& id : hpx::find_all_localities()) { zerocopy zc = hpx::new_(id, ZEROCOPY_DATASIZE); @@ -238,7 +236,10 @@ int main() hpx::chrono::high_resolution_timer t; for (int i = 0; i != 100; ++i) - zc.get(hpx::launch::sync, ZEROCOPY_DATASIZE); + { + [[maybe_unused]] auto r = + zc.get(hpx::launch::sync, ZEROCOPY_DATASIZE); + } double d = t.elapsed(); std::cout << "Elapsed time 'get' (locality " @@ -252,7 +253,7 @@ int main() for (int i = 0; i != 100; ++i) zc.get_here(hpx::launch::sync, buffer); - double d = t.elapsed(); + double const d = t.elapsed(); std::cout << "Elapsed time 'get_here' (locality " << hpx::naming::get_locality_id_from_id(id) << "): " << d << "[s]\n"; diff --git a/libs/core/serialization/include/hpx/serialization/serialize_buffer.hpp b/libs/core/serialization/include/hpx/serialization/serialize_buffer.hpp index 8a4b032f3f14..b4e03586b1d7 100644 --- a/libs/core/serialization/include/hpx/serialization/serialize_buffer.hpp +++ b/libs/core/serialization/include/hpx/serialization/serialize_buffer.hpp @@ -20,12 +20,56 @@ #include #endif -#include #include #include namespace hpx::serialization { + namespace detail { + + template + struct array_allocator + { + auto* operator()(Allocator alloc, std::size_t size) const + { + auto* p = alloc.allocate(size); + std::uninitialized_default_construct(p, p + size); + return p; + } + }; + + template + struct array_allocator> + { + T* operator()(std::allocator, std::size_t size) const + { + return new T[size]; + } + }; + + template + struct array_deleter + { + template + void operator()( + T* p, Deallocator dealloc, std::size_t size) const noexcept + { + std::destroy(p, p + size); + dealloc.deallocate(p, size); + } + }; + + template + struct array_deleter> + { + void operator()( + T const* p, std::allocator, std::size_t) const noexcept + { + delete[] p; + } + }; + } // namespace detail + /////////////////////////////////////////////////////////////////////////// template class serialize_buffer @@ -41,20 +85,28 @@ namespace hpx::serialization { static constexpr void no_deleter(T*) noexcept {} template - static void deleter( - T* p, Deallocator dealloc, std::size_t size) noexcept + struct deleter { - dealloc.deallocate(p, size); - } + void operator()( + T* p, Deallocator dealloc, std::size_t size) const noexcept + { + std::destroy_at(p); + dealloc.deallocate(p, size); + } + }; public: enum init_mode { - copy = 0, // constructor copies data - reference = 1, // constructor does not copy data and does not - // manage the lifetime of it - take = 2 // constructor does not copy data but does take - // ownership and manages the lifetime of it + copy = 0, // constructor copies data + reference = 1, // constructor does not copy data and does not + // manage the lifetime of it + take = 2, // constructor does not copy data but takes + // ownership of array pointer and manages the + // lifetime of it + take_single = 3 // constructor does not copy data but takes + // ownership of pointer to non-array and manages + // the lifetime of it }; using value_type = T; @@ -72,9 +124,10 @@ namespace hpx::serialization { , size_(size) , alloc_(alloc) { - data_.reset(alloc_.allocate(size), - [alloc = this->alloc_, size = this->size_](T* p) noexcept { - serialize_buffer::deleter(p, alloc, size); + auto* p = detail::array_allocator()(alloc_, size); + data_.reset( + p, [alloc = this->alloc_, size = this->size_](T* p) noexcept { + serialize_buffer::deleter()(p, alloc, size); }); } @@ -88,27 +141,35 @@ namespace hpx::serialization { { if (mode == copy) { - data_.reset(alloc_.allocate(size), + auto* p = + detail::array_allocator()(alloc_, size); + data_.reset(p, [alloc = this->alloc_, size = this->size_](T* p) noexcept { - serialize_buffer::deleter( - p, alloc, size); + detail::array_deleter()(p, alloc, size); }); - if (size != 0) - std::copy(data, data + size, data_.get()); + std::uninitialized_copy(data, data + size, p); } else if (mode == reference) { data_ = buffer_type(data, &serialize_buffer::no_deleter); } - else + else if (mode == take_single) { // take ownership data_ = buffer_type(data, [alloc = this->alloc_, size = this->size_](T* p) noexcept { - serialize_buffer::deleter( + serialize_buffer::deleter()( p, alloc, size); }); } + else if (mode == take) + { + // take ownership + data_ = buffer_type(data, + [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); + }); + } } template @@ -120,49 +181,60 @@ namespace hpx::serialization { { // if 2 allocators are specified we assume mode 'take' data_ = buffer_type(data, [this, dealloc](T* p) noexcept { - serialize_buffer::deleter(p, dealloc, size_); + detail::array_deleter()(p, dealloc, size_); }); } template serialize_buffer(T* data, std::size_t size, init_mode mode, - Deleter const& deleter, - allocator_type const& alloc = allocator_type()) + Deleter&& deleter, allocator_type const& alloc = allocator_type()) : data_() , size_(size) , alloc_(alloc) { if (mode == copy) { - data_.reset(alloc_.allocate(size), deleter); - if (size != 0) - std::copy(data, data + size, data_.get()); + auto* p = + detail::array_allocator()(alloc_, size); + data_ = buffer_type(p, + [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); + }); + std::uninitialized_copy(data, data + size, p); } else { // reference or take ownership, behavior is defined by deleter - data_ = buffer_type(data, deleter); + data_ = buffer_type(data, + [deleter = HPX_FORWARD(Deleter, deleter)]( + T* p) noexcept { deleter(p); }); } } template serialize_buffer(T const* data, std::size_t size, init_mode mode, //-V659 - Deleter const& deleter, - allocator_type const& alloc = allocator_type()) + Deleter&& deleter, allocator_type const& alloc = allocator_type()) : data_() , size_(size) , alloc_(alloc) { if (mode == copy) { - data_.reset(alloc_.allocate(size), deleter); - if (size != 0) - std::copy(data, data + size, data_.get()); + auto* p = + detail::array_allocator()(alloc_, size); + data_ = buffer_type(p, + [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); + }); + std::uninitialized_copy(data, data + size, p); } else if (mode == reference) { - data_ = buffer_type(const_cast(data), deleter); + // reference behavior is defined by deleter + data_ = buffer_type(const_cast(data), + [deleter = HPX_FORWARD(Deleter, deleter)]( + T* p) noexcept { deleter(p); }); } else { @@ -193,26 +265,28 @@ namespace hpx::serialization { , alloc_(alloc) { // create from const data implies 'copy' mode - data_.reset(alloc_.allocate(size), - [alloc = this->alloc_, size = this->size_](T* p) noexcept { - serialize_buffer::deleter(p, alloc, size); + auto* p = detail::array_allocator()(alloc_, size); + data_ = buffer_type( + p, [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); }); - if (size != 0) - std::copy(data, data + size, data_.get()); + std::uninitialized_copy(data, data + size, p); } template - serialize_buffer(T const* data, std::size_t size, - Deleter const& deleter, + serialize_buffer(T const* data, std::size_t size, Deleter&&, allocator_type const& alloc = allocator_type()) : data_() , size_(size) , alloc_(alloc) { // create from const data implies 'copy' mode - data_.reset(alloc_.allocate(size), deleter); - if (size != 0) - std::copy(data, data + size, data_.get()); + auto* p = detail::array_allocator()(alloc_, size); + data_ = buffer_type( + p, [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); + }); + std::uninitialized_copy(data, data + size, p); } serialize_buffer(T const* data, std::size_t size, init_mode mode, @@ -223,13 +297,13 @@ namespace hpx::serialization { { if (mode == copy) { - data_.reset(alloc_.allocate(size), + auto* p = + detail::array_allocator()(alloc_, size); + data_ = buffer_type(p, [alloc = this->alloc_, size = this->size_](T* p) noexcept { - serialize_buffer::deleter( - p, alloc, size); + detail::array_deleter()(p, alloc, size); }); - if (size != 0) - std::copy(data, data + size, data_.get()); + std::uninitialized_copy(data, data + size, p); } else if (mode == reference) { @@ -316,9 +390,10 @@ namespace hpx::serialization { { ar >> size_ >> alloc_; // -V128 - data_.reset(alloc_.allocate(size_), - [alloc = this->alloc_, size = this->size_](T* p) { - serialize_buffer::deleter(p, alloc, size); + data_ = buffer_type( + detail::array_allocator()(alloc_, size_), + [alloc = this->alloc_, size = this->size_](T* p) noexcept { + detail::array_deleter()(p, alloc, size); }); if (size_ != 0) diff --git a/libs/full/checkpoint/examples/1d_stencil_4_checkpoint.cpp b/libs/full/checkpoint/examples/1d_stencil_4_checkpoint.cpp index c25d206a50df..740ab8396614 100644 --- a/libs/full/checkpoint/examples/1d_stencil_4_checkpoint.cpp +++ b/libs/full/checkpoint/examples/1d_stencil_4_checkpoint.cpp @@ -70,13 +70,13 @@ struct partition_data partition_data() = default; explicit partition_data(std::size_t size) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) { } partition_data(std::size_t size, double initial_value) - : data_(std::allocator().allocate(size), size, buffer_type::take) + : data_(new double[size], size, buffer_type::take) , size_(size) { double const base_value = static_cast(initial_value * size); @@ -85,8 +85,7 @@ struct partition_data } partition_data(partition_data const& old_part) - : data_(std::allocator().allocate(old_part.size()), - old_part.size(), buffer_type::take) + : data_(new double[old_part.size()], old_part.size(), buffer_type::take) , size_(old_part.size()) { for (std::size_t i = 0; i < old_part.size(); i++)