Skip to content

Commit

Permalink
refactor(container): make Linked_Bump_Allocator use run-time alignment
Browse files Browse the repository at this point in the history
Linked_Bump_Allocator is coded to align all allocations to 'alignment'
(typically 8). This is problematic for a few reasons:

1. 'alignment' is specified as a compile-time template parameter,
   forcing Linked_Bump_Allocator to be a template. This probably slows
   down compilation and clutters the header file. (This is the main
   problem motivating this patch.)
2. Allocations perform run-time alignment anyways, but on the size
   rather than on the pointer.
3. The forced alignment wastes some space.
4. Linked_Bump_Allocator cannot support overaligned data. (This isn't a
   problem yet, and I don't forsee this being a real problem. I mention
   this for completeness.)

Teach Linked_Bump_Allocator to align everything according to a run-time
alignment. This will let us fix issue #1 later, directly fixes issue #3,
and (with some work included in this patch) fixes #4.

This patch might have negative performance costs. I did not measure.
  • Loading branch information
strager committed Nov 3, 2023
1 parent 6b5fd05 commit a87a431
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 36 deletions.
6 changes: 6 additions & 0 deletions src/quick-lint-js/container/flexible-array.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ class alignas(maximum(alignof(T),
alignof(Flexible_Array_Base<Header>))) Flexible_Array
: public Flexible_Array_Base<Header> {
public:
// The guaranteed alignment for the capacity.
//
// Invariant: capacity_alignment >= alignof(T)
static inline constexpr std::size_t capacity_alignment =
alignof(Flexible_Array);

T* flexible_capacity_begin() { return reinterpret_cast<T*>(&this[1]); }

T* flexible_capacity_end() {
Expand Down
89 changes: 55 additions & 34 deletions src/quick-lint-js/container/linked-bump-allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <quick-lint-js/port/memory-resource.h>
#include <quick-lint-js/port/span.h>
#include <quick-lint-js/util/cast.h>
#include <quick-lint-js/util/math-overflow.h>
#include <quick-lint-js/util/pointer.h>
#include <utility>

Expand All @@ -32,9 +33,10 @@ namespace quick_lint_js {
// * no per-object free()/delete
// * bulk free() via rewind() (doesn't call destructors)
// * in-place growing of allocations
// * all allocations have the same alignment
//
// Internally, Linked_Bump_Allocator maintains a linked list of chunks.
//
// TODO(strager): Remove alignment parameter.
template <std::size_t alignment>
class Linked_Bump_Allocator final : public Memory_Resource {
private:
Expand Down Expand Up @@ -133,11 +135,8 @@ class Linked_Bump_Allocator final : public Memory_Resource {

template <class T, class... Args>
T* new_object(Args&&... args) {
static_assert(alignof(T) <= alignment,
"T is not allowed by this allocator; this allocator's "
"alignment is insufficient for T");
constexpr std::size_t byte_size = align_up(sizeof(T));
return new (this->allocate_bytes(byte_size)) T(std::forward<Args>(args)...);
return new (this->allocate_bytes(sizeof(T), alignof(T)))
T(std::forward<Args>(args)...);
}

template <class T>
Expand All @@ -156,11 +155,9 @@ class Linked_Bump_Allocator final : public Memory_Resource {

template <class T>
[[nodiscard]] Span<T> allocate_uninitialized_span(std::size_t size) {
static_assert(alignof(T) <= alignment,
"T is not allowed by this allocator; this allocator's "
"alignment is insufficient for T");
std::size_t byte_size = this->align_up(size * sizeof(T));
T* items = reinterpret_cast<T*>(this->allocate_bytes(byte_size));
std::size_t byte_size = size * sizeof(T);
T* items =
reinterpret_cast<T*>(this->allocate_bytes(byte_size, alignof(T)));
return Span<T>(items, narrow_cast<Span_Size>(size));
}

Expand All @@ -185,7 +182,7 @@ class Linked_Bump_Allocator final : public Memory_Resource {
std::size_t new_size) {
this->assert_not_disabled();
QLJS_ASSERT(new_size > old_size);
std::size_t old_byte_size = this->align_up(old_size * sizeof(T));
std::size_t old_byte_size = old_size * sizeof(T);
bool array_is_last_allocation =
reinterpret_cast<char*>(array) + old_byte_size ==
this->next_allocation_;
Expand All @@ -194,7 +191,7 @@ class Linked_Bump_Allocator final : public Memory_Resource {
return false;
}

std::size_t extra_bytes = this->align_up((new_size - old_size) * sizeof(T));
std::size_t extra_bytes = (new_size - old_size) * sizeof(T);
if (extra_bytes > this->remaining_bytes_in_current_chunk()) {
return false;
}
Expand Down Expand Up @@ -235,37 +232,47 @@ class Linked_Bump_Allocator final : public Memory_Resource {

protected:
void* do_allocate(std::size_t bytes, std::size_t align) override {
QLJS_ASSERT(align <= alignment);
return this->allocate_bytes(this->align_up(bytes));
return this->allocate_bytes(bytes, align);
}

void do_deallocate(void* p, std::size_t bytes, std::size_t align) override {
QLJS_ASSERT(align <= alignment);
void do_deallocate(void* p, std::size_t bytes,
[[maybe_unused]] std::size_t align) override {
this->deallocate_bytes(p, bytes);
}

private:
struct alignas(maximum(alignment, alignof(void*))) Chunk_Header {
struct alignas(alignof(void*)) Chunk_Header {
explicit Chunk_Header(Chunk* previous) : previous(previous) {}

Chunk* previous; // Linked list.
};

static inline constexpr std::size_t default_chunk_size = 4096 - sizeof(Chunk);

static constexpr std::size_t align_up(std::size_t size) {
return (size + alignment - 1) & ~(alignment - 1);
}

[[nodiscard]] void* allocate_bytes(std::size_t size) {
[[nodiscard]] void* allocate_bytes(std::size_t size, std::size_t align) {
this->assert_not_disabled();
QLJS_SLOW_ASSERT(size % alignment == 0);
if (this->remaining_bytes_in_current_chunk() < size) {
this->append_chunk(maximum(size, this->default_chunk_size));
QLJS_ASSERT(this->remaining_bytes_in_current_chunk() >= size);
}
void* result = this->next_allocation_;
this->next_allocation_ += size;
QLJS_SLOW_ASSERT(size % align == 0);

std::uintptr_t next_allocation_int =
reinterpret_cast<std::uintptr_t>(this->next_allocation_);
// NOTE(strager): align_up might overflow. If it does, the allocation
// couldn't fit due to alignment alone
// (alignment_padding > this->remaining_bytes_in_current_chunk()).
std::uintptr_t result_int = align_up(next_allocation_int, align);
char* result = reinterpret_cast<char*>(result_int);

// alignment_padding is how much the pointer moved due to alignment, i.e.
// how much padding is necessary due to alignment.
std::uintptr_t alignment_padding = result_int - next_allocation_int;
bool have_enough_space =
(alignment_padding + size) <= this->remaining_bytes_in_current_chunk();
if (!have_enough_space) [[unlikely]] {
this->append_chunk(maximum(size, this->default_chunk_size), align);
result = this->next_allocation_;
QLJS_ASSERT(is_aligned(result, align));
}

this->next_allocation_ = result + size;
this->did_allocate_bytes(result, size);
return result;
}
Expand All @@ -291,11 +298,25 @@ class Linked_Bump_Allocator final : public Memory_Resource {
// TODO(strager): Mark memory as unusable for Valgrind.
}

void append_chunk(std::size_t size) {
this->chunk_ = Chunk::allocate_and_construct_header(new_delete_resource(),
size, this->chunk_);
this->next_allocation_ = this->chunk_->flexible_capacity_begin();
void append_chunk(std::size_t size, std::size_t align) {
// Over-alignment is tested by
// NOTE[Linked_Bump_Allocator-append_chunk-alignment].
bool is_over_aligned = align > Chunk::capacity_alignment;
std::size_t allocated_size = size;
if (is_over_aligned) {
allocated_size += align;
}
this->chunk_ = Chunk::allocate_and_construct_header(
new_delete_resource(), allocated_size, this->chunk_);
std::uintptr_t next_allocation_int = reinterpret_cast<std::uintptr_t>(
this->chunk_->flexible_capacity_begin());
if (is_over_aligned) {
next_allocation_int = align_up(next_allocation_int, align);
}
this->next_allocation_ = reinterpret_cast<char*>(next_allocation_int);
this->chunk_end_ = this->chunk_->flexible_capacity_end();
QLJS_ASSERT(is_aligned(this->next_allocation_, align));
QLJS_ASSERT(this->remaining_bytes_in_current_chunk() >= size);
}

void assert_not_disabled() const {
Expand Down
4 changes: 3 additions & 1 deletion src/quick-lint-js/util/pointer.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ inline bool is_aligned(void* p, std::size_t alignment) {
return (reinterpret_cast<std::uintptr_t>(p) & alignment_mask) == 0;
}

// Does not check for pointer overflow.
template <class Integer_Pointer>
Integer_Pointer align_up(Integer_Pointer p, std::size_t alignment) {
[[nodiscard]] Integer_Pointer align_up(Integer_Pointer p,
std::size_t alignment) {
Integer_Pointer alignment_mask = static_cast<Integer_Pointer>(alignment - 1);
// TODO(strager): What about integer overflow?
return ((p - 1) | alignment_mask) + 1;
Expand Down
22 changes: 21 additions & 1 deletion test/test-linked-bump-allocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ TEST(Test_Linked_Bump_Allocator,

TEST(Test_Linked_Bump_Allocator,
less_aligned_pre_grown_and_grown_array_keeps_next_allocation_aligned) {
Linked_Bump_Allocator<4> alloc("test");
Linked_Bump_Allocator<1> alloc("test");

Span<char> chars = alloc.allocate_uninitialized_span<char>(3);
bool grew = alloc.try_grow_array_in_place(chars.data(), 3, 6);
Expand Down Expand Up @@ -136,6 +136,26 @@ TEST(Test_Linked_Bump_Allocator, filling_first_chunk_allocates_second_chunk) {
assert_valid_memory(new_chunk_object);
}

TEST(Test_Linked_Bump_Allocator,
allocate_new_chunk_with_over_alignment_aligns) {
constexpr std::size_t chunk_size =
4096 - sizeof(void*) * 2; // Implementation detail.

Linked_Bump_Allocator<alignof(std::uint32_t)> alloc("test");
[[maybe_unused]] Span<char> padding = alloc.allocate_span<char>(chunk_size);
ASSERT_EQ(alloc.remaining_bytes_in_current_chunk(), 0)
<< "First chunk should be consumed entirely";

// NOTE[Linked_Bump_Allocator-append_chunk-alignment]: This should allocate a
// brand new chunk, thus exercising the alignment code in
// Linked_Bump_Allocator::append_chunk.
struct alignas(128) Over_Aligned_Struct {
char data[chunk_size];
};
Over_Aligned_Struct* s = alloc.new_object<Over_Aligned_Struct>();
assert_valid_memory(s);
}

TEST(Test_Linked_Bump_Allocator, rewinding_within_chunk_reuses_memory) {
Linked_Bump_Allocator<1> alloc("test");

Expand Down

0 comments on commit a87a431

Please sign in to comment.