From 075dfafcc3404afedd2f3f9e0871f1a4fd3a08d7 Mon Sep 17 00:00:00 2001 From: Thomas Heller Date: Wed, 15 Nov 2017 15:28:37 +0100 Subject: [PATCH] Fixing wrapper_heap alignment problems This patch ensures that the addresses returned from the wrapper_heap are correctly aligned for the underlying objects. --- .../components/server/wrapper_heap.hpp | 17 ++- .../components/server/wrapper_heap_list.hpp | 17 ++- hpx/util/one_size_heap_list.hpp | 27 ++--- hpx/util/wrapper_heap_base.hpp | 7 ++ .../components/server/wrapper_heap.cpp | 106 ++++++++++-------- src/util/one_size_heap_list.cpp | 4 +- 6 files changed, 99 insertions(+), 79 deletions(-) diff --git a/hpx/runtime/components/server/wrapper_heap.hpp b/hpx/runtime/components/server/wrapper_heap.hpp index ec36d6b15ebd..f09ca00efc82 100644 --- a/hpx/runtime/components/server/wrapper_heap.hpp +++ b/hpx/runtime/components/server/wrapper_heap.hpp @@ -76,6 +76,7 @@ namespace hpx { namespace components { namespace detail typedef one_size_heap_allocators::fixed_mallocator allocator_type; typedef hpx::lcos::local::spinlock mutex_type; typedef std::unique_lock scoped_lock; + typedef wrapper_heap_base::heap_parameters heap_parameters; #if HPX_DEBUG_WRAPPER_HEAP != 0 enum guard_value @@ -87,7 +88,7 @@ namespace hpx { namespace components { namespace detail public: explicit wrapper_heap(char const* class_name, std::size_t count, - std::size_t heap_size, std::size_t step); + heap_parameters parameters); wrapper_heap(); ~wrapper_heap() override; @@ -120,10 +121,9 @@ namespace hpx { namespace components { namespace detail void tidy(); protected: - void* pool_; - void* first_free_; - std::size_t step_; - std::size_t size_; + char* pool_; + char* first_free_; + heap_parameters const parameters_; std::size_t free_size_; // these values are used for AGAS registration of all elements of this @@ -140,8 +140,6 @@ namespace hpx { namespace components { namespace detail std::size_t heap_count_; #endif - std::size_t const element_size_; // size of one element in the heap - // make sure the ABI of this is stable across configurations #if defined(HPX_DEBUG) std::size_t heap_count() const override { return heap_count_; } @@ -161,13 +159,14 @@ namespace hpx { namespace components { namespace detail { private: typedef wrapper_heap base_type; + typedef base_type::heap_parameters heap_parameters; public: typedef T value_type; explicit fixed_wrapper_heap(char const* class_name, - std::size_t count, std::size_t step, std::size_t element_size) - : base_type(class_name, count, step, element_size) + std::size_t count, heap_parameters parameters) + : base_type(class_name, count, parameters) {} }; }}} // namespace hpx::components::detail diff --git a/hpx/runtime/components/server/wrapper_heap_list.hpp b/hpx/runtime/components/server/wrapper_heap_list.hpp index 617bf27c45d4..3ec654855176 100644 --- a/hpx/runtime/components/server/wrapper_heap_list.hpp +++ b/hpx/runtime/components/server/wrapper_heap_list.hpp @@ -23,22 +23,27 @@ namespace hpx { namespace components { namespace detail class wrapper_heap_list : public util::one_size_heap_list { typedef util::one_size_heap_list base_type; - typedef typename Heap::value_type value_ype; + typedef typename Heap::value_type value_type; typedef typename std::aligned_storage< - sizeof(value_ype), std::alignment_of::value + sizeof(value_type), std::alignment_of::value >::type storage_type; enum { - heap_step = 0xFFF, // default initial number of elements - heap_size = sizeof(storage_type) // size of one element in the heap + // default initial number of elements + heap_capacity = 0xFFF, + // Alignment of one element + heap_element_alignment = std::alignment_of::value, + // size of one element in the heap + heap_element_size = sizeof(storage_type) }; public: wrapper_heap_list(component_type type) - : base_type(get_component_type_name(type), heap_step, heap_size, - (Heap*) nullptr) + : base_type(get_component_type_name(type), + base_type::heap_parameters{heap_capacity, heap_element_alignment, + heap_element_size}, (Heap*) nullptr) , type_(type) {} diff --git a/hpx/util/one_size_heap_list.hpp b/hpx/util/one_size_heap_list.hpp index f207d8ce639b..cec4f498c5e3 100644 --- a/hpx/util/one_size_heap_list.hpp +++ b/hpx/util/one_size_heap_list.hpp @@ -34,16 +34,17 @@ namespace hpx { namespace util typedef std::unique_lock unique_lock_type; + typedef wrapper_heap_base::heap_parameters heap_parameters; + private: template static std::shared_ptr create_heap( - char const* name, std::size_t counter, std::size_t step, - std::size_t heap_size) + char const* name, std::size_t counter, heap_parameters parameters) { #if defined(HPX_DEBUG) - return std::make_shared(name, counter, step, heap_size); + return std::make_shared(name, counter, parameters); #else - return std::make_shared(name, 0, step, heap_size); + return std::make_shared(name, 0, parameters); #endif } @@ -57,15 +58,14 @@ namespace hpx { namespace util , max_alloc_count_(0) #endif , create_heap_(nullptr) - , heap_step_(0) - , heap_size_(0) + , parameters_({0, 0, 0}) { HPX_ASSERT(false); // shouldn't ever be called } template explicit one_size_heap_list(char const* class_name, - std::size_t heap_step, std::size_t heap_size, Heap* = nullptr) + heap_parameters parameters, Heap* = nullptr) : class_name_(class_name) #if defined(HPX_DEBUG) , alloc_count_(0L) @@ -74,13 +74,12 @@ namespace hpx { namespace util , max_alloc_count_(0L) #endif , create_heap_(&one_size_heap_list::create_heap) - , heap_step_(heap_step) - , heap_size_(heap_size) + , parameters_(parameters) {} template explicit one_size_heap_list(std::string const& class_name, - std::size_t heap_step, std::size_t heap_size, Heap* = nullptr) + heap_parameters parameters, Heap* = nullptr) : class_name_(class_name) #if defined(HPX_DEBUG) , alloc_count_(0L) @@ -89,8 +88,7 @@ namespace hpx { namespace util , max_alloc_count_(0L) #endif , create_heap_(&one_size_heap_list::create_heap) - , heap_step_(heap_step) - , heap_size_(heap_size) + , parameters_(parameters) {} ~one_size_heap_list() noexcept; @@ -122,10 +120,9 @@ namespace hpx { namespace util std::size_t max_alloc_count_; #endif std::shared_ptr (*create_heap_)( - char const*, std::size_t, std::size_t, std::size_t); + char const*, std::size_t, heap_parameters); - std::size_t const heap_step_; // default grow step - std::size_t const heap_size_; // size of the object + heap_parameters const parameters_; }; }} diff --git a/hpx/util/wrapper_heap_base.hpp b/hpx/util/wrapper_heap_base.hpp index e66f9f060e8d..00109c5ec5b2 100644 --- a/hpx/util/wrapper_heap_base.hpp +++ b/hpx/util/wrapper_heap_base.hpp @@ -17,6 +17,13 @@ namespace hpx { namespace util { struct wrapper_heap_base { + struct heap_parameters + { + std::size_t capacity; + std::size_t element_alignment; + std::size_t element_size; + }; + virtual ~wrapper_heap_base() {} virtual bool alloc(void** result, std::size_t count = 1) = 0; diff --git a/src/runtime/components/server/wrapper_heap.cpp b/src/runtime/components/server/wrapper_heap.cpp index d68caa2d3f44..de39dad152d6 100644 --- a/src/runtime/components/server/wrapper_heap.cpp +++ b/src/runtime/components/server/wrapper_heap.cpp @@ -69,12 +69,10 @@ namespace hpx { namespace components { namespace detail #else , std::size_t #endif - , std::size_t heap_size - , std::size_t step) + , heap_parameters parameters) : pool_(nullptr) , first_free_(nullptr) - , step_(step) - , size_(0) + , parameters_(parameters) , free_size_(0) , base_gid_(naming::invalid_gid) , class_name_(class_name) @@ -83,7 +81,6 @@ namespace hpx { namespace components { namespace detail , free_count_(0) , heap_count_(count) #endif - , element_size_(heap_size) , heap_alloc_function_("wrapper_heap::alloc", class_name) , heap_free_function_("wrapper_heap::free", class_name) { @@ -95,8 +92,7 @@ namespace hpx { namespace components { namespace detail wrapper_heap::wrapper_heap() : pool_(nullptr) , first_free_(nullptr) - , step_(0) - , size_(0) + , parameters_({0, 0, 0}) , free_size_(0) , base_gid_(naming::invalid_gid) #if defined(HPX_DEBUG) @@ -104,7 +100,6 @@ namespace hpx { namespace components { namespace detail , free_count_(0) , heap_count_(0) #endif - , element_size_(0) , heap_alloc_function_("wrapper_heap::alloc", "") , heap_free_function_("wrapper_heap::free", "") { @@ -120,7 +115,7 @@ namespace hpx { namespace components { namespace detail std::size_t wrapper_heap::size() const { util::itt::heap_internal_access hia; HPX_UNUSED(hia); - return size_ - free_size_; + return parameters_.capacity - free_size_; } std::size_t wrapper_heap::free_size() const @@ -138,13 +133,14 @@ namespace hpx { namespace components { namespace detail bool wrapper_heap::has_allocatable_slots() const { util::itt::heap_internal_access hia; HPX_UNUSED(hia); - return first_free_ < static_cast(pool_) + size_ * element_size_; + std::size_t const num_bytes = parameters_.capacity * parameters_.element_size; + return first_free_ < pool_ + num_bytes; } bool wrapper_heap::alloc(void** result, std::size_t count) { util::itt::heap_allocate heap_allocate( - heap_alloc_function_, result, count * element_size_, + heap_alloc_function_, result, count * parameters_.element_size, HPX_WRAPPER_HEAP_INITIALIZED_MEMORY); scoped_lock l(mtx_); @@ -159,14 +155,14 @@ namespace hpx { namespace components { namespace detail void* p = first_free_; HPX_ASSERT(p != nullptr); - first_free_ = static_cast(first_free_) + count * element_size_; + first_free_ = first_free_ + count * parameters_.element_size; HPX_ASSERT(free_size_ >= count); free_size_ -= count; #if HPX_DEBUG_WRAPPER_HEAP != 0 // init memory blocks - debug::fill_bytes(p, initial_value, count * element_size_); + debug::fill_bytes(p, initial_value, count * parameters_.element_size); #endif *result = p; @@ -183,20 +179,23 @@ namespace hpx { namespace components { namespace detail scoped_lock l(mtx_); #if HPX_DEBUG_WRAPPER_HEAP != 0 - char* p1 = static_cast(p); + char* p1 = p; + std::size_t const total_num_bytes = + parameters_.capacity * parameters_.element_size; + std::size_t const num_bytes = count * parameters_.element_size; HPX_ASSERT(nullptr != pool_ && p1 >= pool_); HPX_ASSERT(nullptr != pool_ && - p1 + count * element_size_ <= - static_cast(pool_) + size_ * element_size_); + p1 + num_bytes <= + pool_ + total_num_bytes); HPX_ASSERT(first_free_ == nullptr || p1 != first_free_); - HPX_ASSERT(free_size_ + count <= size_); + HPX_ASSERT(free_size_ + count <= parameters_.capacity); // make sure this has not been freed yet HPX_ASSERT(!debug::test_fill_bytes(p1, freed_value, - count * element_size_)); + num_bytes)); // give memory back to pool - debug::fill_bytes(p1, freed_value, element_size_); + debug::fill_bytes(p1, freed_value, num_bytes); #else HPX_UNUSED(p); #endif @@ -214,9 +213,13 @@ namespace hpx { namespace components { namespace detail { // no lock is necessary here as all involved variables are immutable util::itt::heap_internal_access hia; HPX_UNUSED(hia); - return nullptr != pool_ && nullptr != p && pool_ <= p && - static_cast(p) < - static_cast(pool_) + size_ * element_size_; + if (nullptr == pool_) return false; + if (nullptr == p) return false; + + std::size_t const total_num_bytes = + parameters_.capacity * parameters_.element_size; + return p >= pool_ && + static_cast(p) < pool_ + total_num_bytes; } naming::gid_type wrapper_heap::get_gid( @@ -227,7 +230,6 @@ namespace hpx { namespace components { namespace detail HPX_ASSERT(did_alloc(p)); scoped_lock l(mtx_); - void* addr = pool_; if (!base_gid_) { @@ -237,13 +239,13 @@ namespace hpx { namespace components { namespace detail // this is the first call to get_gid() for this heap - allocate // a sufficiently large range of global ids util::unlock_guard ul(l); - base_gid = ids.get_id(step_); + base_gid = ids.get_id(parameters_.capacity); // register the global ids and the base address of this heap // with the AGAS - if (!applier::bind_range_local(base_gid, step_, - naming::address(hpx::get_locality(), type, addr), - element_size_)) + if (!applier::bind_range_local(base_gid, parameters_.capacity, + naming::address(hpx::get_locality(), type, pool_), + parameters_.element_size)) { return naming::invalid_gid; } @@ -261,12 +263,12 @@ namespace hpx { namespace components { namespace detail { // unbind the range which is not needed anymore util::unlock_guard ul(l); - applier::unbind_range_local(base_gid, step_); + applier::unbind_range_local(base_gid, parameters_.capacity); } } - return base_gid_ + static_cast( - (static_cast(p) - static_cast(addr)) / element_size_); + std::uint64_t const distance = static_cast(p) - pool_; + return base_gid_ + distance / parameters_.element_size; } void wrapper_heap::set_gid(naming::gid_type const& g) @@ -279,14 +281,18 @@ namespace hpx { namespace components { namespace detail bool wrapper_heap::test_release(scoped_lock& lk) { - if (pool_ == nullptr || free_size_ < size_ || - static_cast(first_free_) < - static_cast(pool_) + size_ * element_size_) + if (pool_ == nullptr) + return false; + + std::size_t const total_num_bytes = + parameters_.capacity * parameters_.element_size; + + if (first_free_ < pool_ + total_num_bytes) { return false; } - HPX_ASSERT(free_size_ == size_); + HPX_ASSERT(first_free_ == pool_ + total_num_bytes); // unbind in AGAS service if (base_gid_) @@ -295,7 +301,7 @@ namespace hpx { namespace components { namespace detail base_gid_ = naming::invalid_gid; util::unlock_guard ull(lk); - applier::unbind_range_local(base_gid, step_); + applier::unbind_range_local(base_gid, parameters_.capacity); } tidy(); @@ -309,8 +315,12 @@ namespace hpx { namespace components { namespace detail return false; } - if (static_cast(first_free_) + count * element_size_ > - static_cast(pool_) + size_ * element_size_) + std::size_t const num_bytes = + count * parameters_.element_size; + std::size_t const total_num_bytes = + parameters_.capacity * parameters_.element_size; + + if (first_free_ + num_bytes > pool_ + total_num_bytes) { return false; } @@ -319,25 +329,27 @@ namespace hpx { namespace components { namespace detail bool wrapper_heap::init_pool() { - HPX_ASSERT(size_ == 0); HPX_ASSERT(first_free_ == nullptr); - std::size_t s = step_ * element_size_; //-V104 //-V707 - pool_ = allocator_type::alloc(s); + std::size_t const total_num_bytes = + parameters_.capacity * parameters_.element_size; + pool_ = static_cast(allocator_type::alloc(total_num_bytes)); if (nullptr == pool_) { return false; } - first_free_ = pool_; - size_ = s / element_size_; //-V104 - free_size_ = size_; + first_free_ = (reinterpret_cast(pool_) + % parameters_.element_alignment == 0) ? pool_ : + pool_ + parameters_.element_alignment; + + free_size_ = parameters_.capacity; LOSH_(info) //-V128 << "wrapper_heap (" << (!class_name_.empty() ? class_name_.c_str() : "") << "): init_pool (" << std::hex << pool_ << ")" - << " size: " << s << "."; + << " size: " << total_num_bytes << "."; return true; } @@ -356,7 +368,7 @@ namespace hpx { namespace components { namespace detail #endif << "."; - if (free_size_ != size_ + if (size() > 0 #if defined(HPX_DEBUG) || alloc_count_ != free_count_ #endif @@ -366,12 +378,12 @@ namespace hpx { namespace components { namespace detail << "wrapper_heap (" << (!class_name_.empty() ? class_name_.c_str() : "") << "): releasing heap (" << std::hex << pool_ << ")" - << " with " << size_-free_size_ << " allocated object(s)!"; + << " with " << size() << " allocated object(s)!"; } allocator_type::free(pool_); pool_ = first_free_ = nullptr; - size_ = free_size_ = 0; + free_size_ = 0; } } }}} diff --git a/src/util/one_size_heap_list.cpp b/src/util/one_size_heap_list.cpp index 4941fc3077cf..849875d80fe5 100644 --- a/src/util/one_size_heap_list.cpp +++ b/src/util/one_size_heap_list.cpp @@ -103,10 +103,10 @@ namespace hpx { namespace util { #if defined(HPX_DEBUG) heap_list_.push_front(create_heap_( - class_name_.c_str(), heap_count_ + 1, heap_step_, heap_size_)); + class_name_.c_str(), heap_count_ + 1, parameters_)); #else heap_list_.push_front(create_heap_( - class_name_.c_str(), 0, heap_step_, heap_size_)); + class_name_.c_str(), 0, parameters_)); #endif iterator itnew = heap_list_.begin();