From bb832de60856fe9be3d2121d65f32ea5dada96d9 Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Fri, 23 Aug 2019 13:46:27 +0200 Subject: [PATCH] Prepend MonotonicBufferResource::Buffer to an allocation Summary: This is a different scheme for setting up a bookkeeping object while still supporting arbitrary allocation alignment requests. The previous scheme was simpler as it always allocated a power of 2 bytes, but the trade-off was increased memory usage. This should waste less memory. Reviewers: mtomic, mferencevic, ipaljak Reviewed By: mtomic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2321 --- src/utils/memory.cpp | 38 +++++++++++++++++++++++--------------- src/utils/memory.hpp | 27 ++++++++++++++++++--------- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/utils/memory.cpp b/src/utils/memory.cpp index 306714445..f9d894a72 100644 --- a/src/utils/memory.cpp +++ b/src/utils/memory.cpp @@ -69,11 +69,10 @@ MonotonicBufferResource &MonotonicBufferResource::operator=( void MonotonicBufferResource::Release() { for (auto *b = current_buffer_; b;) { auto *next = b->next; - auto *ptr = b->data(); auto alloc_size = b->size(); auto alignment = b->alignment; b->~Buffer(); - memory_->Deallocate(ptr, alloc_size, alignment); + memory_->Deallocate(b, alloc_size, alignment); b = next; } current_buffer_ = nullptr; @@ -85,22 +84,31 @@ void *MonotonicBufferResource::DoAllocate(size_t bytes, size_t alignment) { auto push_current_buffer = [this, bytes, alignment](size_t next_size) { // Set size so that the bytes fit. size_t size = next_size > bytes ? next_size : bytes; - // Handle the case when we need to align `Buffer::data` to a greater - // `alignment`. We will simply always allocate Ceil2(required_size), so that - // we can use the end of the allocated bytes for Buffer instance. + // Simplify alignment by always using values greater or equal to max_align + size_t alloc_align = std::max(alignment, alignof(std::max_align_t)); + // Setup the Buffer area before `Buffer::data` such that `Buffer::data` is + // correctly aligned. We do this by allocating an additional `multiple` of + // `alignment` of bytes. `multiple` is determined by the size of Buffer. + // This will ensure that both Buffer fits and is correctly aligned. + // `Buffer::data` is also correctly aligned as we use the pointer after this + // `multiple` of `alignment` bytes. static_assert(IsPow2(alignof(Buffer)), "Buffer should not be a packed struct in order to be placed " - "at the end of an allocation request"); - size_t bytes = sizeof(Buffer) + size; - if (bytes < size) throw BadAlloc("Allocation size overflow"); - size_t alloc_size = Ceil2(bytes); - if (alloc_size < bytes) throw BadAlloc("Allocation size overflow"); - size_t alloc_align = std::max(alignment, alignof(std::max_align_t)); + "at the start of an allocation request"); + size_t bytes_for_buffer = std::max(alloc_align, sizeof(Buffer)); + size_t multiple = bytes_for_buffer / alloc_align; + if (bytes_for_buffer % alloc_align != 0) ++multiple; + bytes_for_buffer = multiple * alloc_align; + if (bytes_for_buffer < sizeof(Buffer) || + bytes_for_buffer % alloc_align != 0) { + throw BadAlloc("Allocation size overflow"); + } + size_t alloc_size = bytes_for_buffer + size; + if (alloc_size < size) throw BadAlloc("Allocation size overflow"); void *ptr = memory_->Allocate(alloc_size, alloc_align); - // Instantiate the Buffer at the end of the allocated block. - current_buffer_ = - new (reinterpret_cast(ptr) + alloc_size - sizeof(Buffer)) - Buffer{current_buffer_, alloc_size - sizeof(Buffer), alloc_align}; + // Instantiate the Buffer at the start of the allocated block. + current_buffer_ = new (ptr) + Buffer{current_buffer_, alloc_size - bytes_for_buffer, alloc_align}; allocated_ = 0; }; diff --git a/src/utils/memory.hpp b/src/utils/memory.hpp index 28c48b919..567665fa1 100644 --- a/src/utils/memory.hpp +++ b/src/utils/memory.hpp @@ -328,12 +328,8 @@ inline MemoryResource *NewDeleteResource() noexcept { /// buffer is exhausted, a new one is requested from the upstream memory /// resource. /// -/// Note that each buffer of memory is actually a block of `Ceil2(size + -/// sizeof(Buffer))` due to bookkeeping `Buffer` object being appended at the end. -/// This means that if you use an `initial_size` of 1024 bytes, you will -/// actually allocate `Ceil2(1024 + sizeof(Buffer))` which will be 2048 bytes. -/// Therefore you will have `2048 - sizeof(Buffer)` bytes available before a new -/// buffer will need to be allocated. +/// Note that each buffer of memory is actually a larger block of at *least* +/// `(size + sizeof(Buffer))` bytes due to bookkeeping `Buffer` object. class MonotonicBufferResource final : public MemoryResource { public: /// Construct the resource with the buffer size of at least `initial_size`. @@ -377,10 +373,23 @@ class MonotonicBufferResource final : public MemoryResource { Buffer *next; size_t capacity; size_t alignment; + + /// Get the size of the area reserved for `this` + size_t bytes_for_buffer() const { + size_t bytes = std::max(alignment, sizeof(*this)); + if (bytes > alignment) { + size_t multiple = bytes / alignment; + if (bytes % alignment != 0) ++multiple; + bytes = multiple * alignment; + } + return bytes; + } + /// Get total allocated size. - size_t size() const { return sizeof(*this) + capacity; } - /// Get the pointer to data which is before the Buffer instance itself. - char *data() { return reinterpret_cast(this) - capacity; } + size_t size() const { return bytes_for_buffer() + capacity; } + + /// Get the pointer to data which is after the Buffer instance itself. + char *data() { return reinterpret_cast(this) + bytes_for_buffer(); } }; MemoryResource *memory_{NewDeleteResource()};