Revert "[libc] Use best-fit binary trie to make malloc logarithmic" #117065
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Daniel Thornburgh (mysterymath) ChangesReverts llvm/llvm-project#106259 Unit tests break on AArch64. Patch is 71.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117065.diff 18 Files Affected:
diff --git a/libc/fuzzing/__support/CMakeLists.txt b/libc/fuzzing/__support/CMakeLists.txt
index 9d6589d78fb819..b088761f4586a7 100644
--- a/libc/fuzzing/__support/CMakeLists.txt
+++ b/libc/fuzzing/__support/CMakeLists.txt
@@ -23,11 +23,3 @@ add_libc_fuzzer(
COMPILE_OPTIONS
-D__LIBC_EXPLICIT_SIMD_OPT
)
-
-add_libc_fuzzer(
- freelist_heap_fuzz
- SRCS
- freelist_heap_fuzz.cpp
- DEPENDS
- libc.src.__support.freelist_heap
-)
diff --git a/libc/fuzzing/__support/freelist_heap_fuzz.cpp b/libc/fuzzing/__support/freelist_heap_fuzz.cpp
deleted file mode 100644
index d152d0f35499f8..00000000000000
--- a/libc/fuzzing/__support/freelist_heap_fuzz.cpp
+++ /dev/null
@@ -1,227 +0,0 @@
-//===-- freelist_heap_fuzz.cpp --------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-///
-/// Fuzzing test for llvm-libc freelist-based heap implementation.
-///
-//===----------------------------------------------------------------------===//
-
-#include "src/__support/CPP/bit.h"
-#include "src/__support/CPP/optional.h"
-#include "src/__support/freelist_heap.h"
-#include "src/string/memory_utils/inline_memcpy.h"
-#include "src/string/memory_utils/inline_memmove.h"
-#include "src/string/memory_utils/inline_memset.h"
-
-using LIBC_NAMESPACE::FreeListHeap;
-using LIBC_NAMESPACE::inline_memset;
-using LIBC_NAMESPACE::cpp::nullopt;
-using LIBC_NAMESPACE::cpp::optional;
-
-// Record of an outstanding allocation.
-struct Alloc {
- void *ptr;
- size_t size;
- size_t alignment;
- uint8_t canary; // Byte written to the allocation
-};
-
-// A simple vector that tracks allocations using the heap.
-class AllocVec {
-public:
- AllocVec(FreeListHeap &heap) : heap(&heap), size_(0), capacity(0) {
- allocs = nullptr;
- }
-
- bool empty() const { return !size_; }
-
- size_t size() const { return size_; }
-
- bool push_back(Alloc alloc) {
- if (size_ == capacity) {
- size_t new_cap = capacity ? capacity * 2 : 1;
- Alloc *new_allocs = reinterpret_cast<Alloc *>(
- heap->realloc(allocs, new_cap * sizeof(Alloc)));
- if (!new_allocs)
- return false;
- allocs = new_allocs;
- capacity = new_cap;
- }
- allocs[size_++] = alloc;
- return true;
- }
-
- Alloc &operator[](size_t idx) { return allocs[idx]; }
-
- void erase_idx(size_t idx) {
- LIBC_NAMESPACE::inline_memmove(&allocs[idx], &allocs[idx + 1],
- sizeof(Alloc) * (size_ - idx - 1));
- --size_;
- }
-
-private:
- FreeListHeap *heap;
- Alloc *allocs;
- size_t size_;
- size_t capacity;
-};
-
-// Choose a T value by casting libfuzzer data or exit.
-template <typename T>
-optional<T> choose(const uint8_t *&data, size_t &remainder) {
- if (sizeof(T) > remainder)
- return nullopt;
- T out;
- LIBC_NAMESPACE::inline_memcpy(&out, data, sizeof(T));
- data += sizeof(T);
- remainder -= sizeof(T);
- return out;
-}
-
-// The type of allocation to perform
-enum class AllocType : uint8_t {
- MALLOC,
- ALIGNED_ALLOC,
- REALLOC,
- CALLOC,
- NUM_ALLOC_TYPES,
-};
-
-template <>
-optional<AllocType> choose<AllocType>(const uint8_t *&data, size_t &remainder) {
- auto raw = choose<uint8_t>(data, remainder);
- if (!raw)
- return nullopt;
- return static_cast<AllocType>(
- *raw % static_cast<uint8_t>(AllocType::NUM_ALLOC_TYPES));
-}
-
-constexpr size_t heap_size = 64 * 1024;
-
-optional<size_t> choose_size(const uint8_t *&data, size_t &remainder) {
- auto raw = choose<size_t>(data, remainder);
- if (!raw)
- return nullopt;
- return *raw % heap_size;
-}
-
-optional<size_t> choose_alloc_idx(const AllocVec &allocs, const uint8_t *&data,
- size_t &remainder) {
- if (allocs.empty())
- return nullopt;
- auto raw = choose<size_t>(data, remainder);
- if (!raw)
- return nullopt;
- return *raw % allocs.size();
-}
-
-#define ASSIGN_OR_RETURN(TYPE, NAME, EXPR) \
- auto maybe_##NAME = EXPR; \
- if (!maybe_##NAME) \
- return 0; \
- TYPE NAME = *maybe_##NAME
-
-extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t remainder) {
- LIBC_NAMESPACE::FreeListHeapBuffer<heap_size> heap;
- AllocVec allocs(heap);
-
- uint8_t canary = 0;
- while (true) {
- ASSIGN_OR_RETURN(auto, should_alloc, choose<bool>(data, remainder));
- if (should_alloc) {
- ASSIGN_OR_RETURN(auto, alloc_type, choose<AllocType>(data, remainder));
- ASSIGN_OR_RETURN(size_t, alloc_size, choose_size(data, remainder));
-
- // Perform allocation.
- void *ptr = nullptr;
- size_t alignment = alignof(max_align_t);
- switch (alloc_type) {
- case AllocType::MALLOC:
- ptr = heap.allocate(alloc_size);
- break;
- case AllocType::ALIGNED_ALLOC: {
- ASSIGN_OR_RETURN(size_t, alignment, choose_size(data, remainder));
- alignment = LIBC_NAMESPACE::cpp::bit_ceil(alignment);
- ptr = heap.aligned_allocate(alignment, alloc_size);
- break;
- }
- case AllocType::REALLOC: {
- if (!alloc_size)
- return 0;
- ASSIGN_OR_RETURN(size_t, idx,
- choose_alloc_idx(allocs, data, remainder));
- Alloc &alloc = allocs[idx];
- ptr = heap.realloc(alloc.ptr, alloc_size);
- if (ptr) {
- // Extend the canary region if necessary.
- if (alloc_size > alloc.size)
- inline_memset(static_cast<char *>(ptr) + alloc.size, alloc.canary,
- alloc_size - alloc.size);
- alloc.ptr = ptr;
- alloc.size = alloc_size;
- alloc.alignment = alignof(max_align_t);
- }
- break;
- }
- case AllocType::CALLOC: {
- ASSIGN_OR_RETURN(size_t, count, choose_size(data, remainder));
- size_t total;
- if (__builtin_mul_overflow(count, alloc_size, &total))
- return 0;
- ptr = heap.calloc(count, alloc_size);
- if (ptr)
- for (size_t i = 0; i < total; ++i)
- if (static_cast<char *>(ptr)[i] != 0)
- __builtin_trap();
- break;
- }
- case AllocType::NUM_ALLOC_TYPES:
- __builtin_unreachable();
- }
-
- if (ptr) {
- // aligned_allocate should automatically apply a minimum alignment.
- if (alignment < alignof(max_align_t))
- alignment = alignof(max_align_t);
- // Check alignment.
- if (reinterpret_cast<uintptr_t>(ptr) % alignment)
- __builtin_trap();
-
- // Reallocation is treated specially above, since we would otherwise
- // lose the original size.
- if (alloc_type != AllocType::REALLOC) {
- // Fill the object with a canary byte.
- inline_memset(ptr, canary, alloc_size);
-
- // Track the allocation.
- if (!allocs.push_back({ptr, alloc_size, alignment, canary}))
- return 0;
- ++canary;
- }
- }
- } else {
- // Select a random allocation.
- ASSIGN_OR_RETURN(size_t, idx, choose_alloc_idx(allocs, data, remainder));
- Alloc &alloc = allocs[idx];
-
- // Check alignment.
- if (reinterpret_cast<uintptr_t>(alloc.ptr) % alloc.alignment)
- __builtin_trap();
-
- // Check the canary.
- uint8_t *ptr = reinterpret_cast<uint8_t *>(alloc.ptr);
- for (size_t i = 0; i < alloc.size; ++i)
- if (ptr[i] != alloc.canary)
- __builtin_trap();
-
- // Free the allocation and untrack it.
- heap.free(alloc.ptr);
- allocs.erase_idx(idx);
- }
- }
- return 0;
-}
diff --git a/libc/src/__support/CMakeLists.txt b/libc/src/__support/CMakeLists.txt
index 8f85740f70a06e..6637ff9d56f4bc 100644
--- a/libc/src/__support/CMakeLists.txt
+++ b/libc/src/__support/CMakeLists.txt
@@ -14,14 +14,11 @@ add_header_library(
libc.src.__support.CPP.type_traits
)
-add_object_library(
+add_header_library(
freelist
HDRS
freelist.h
- SRCS
- freelist.cpp
DEPENDS
- .block
libc.src.__support.fixedvector
libc.src.__support.CPP.array
libc.src.__support.CPP.cstddef
@@ -29,32 +26,13 @@ add_object_library(
libc.src.__support.CPP.span
)
-add_object_library(
- freetrie
- HDRS
- freetrie.h
- SRCS
- freetrie.cpp
- DEPENDS
- .block
- .freelist
-)
-
-add_header_library(
- freestore
- HDRS
- freestore.h
- DEPENDS
- .freetrie
-)
-
add_header_library(
freelist_heap
HDRS
freelist_heap.h
DEPENDS
.block
- .freestore
+ .freelist
libc.src.__support.CPP.cstddef
libc.src.__support.CPP.array
libc.src.__support.CPP.optional
diff --git a/libc/src/__support/block.h b/libc/src/__support/block.h
index e63801301ac752..96021b99587c87 100644
--- a/libc/src/__support/block.h
+++ b/libc/src/__support/block.h
@@ -174,32 +174,16 @@ class Block {
return inner_size - sizeof(prev_) + BLOCK_OVERHEAD;
}
- /// @returns The number of usable bytes inside the block were it to be
- /// allocated.
+ /// @returns The number of usable bytes inside the block.
size_t inner_size() const {
if (!next())
return 0;
return inner_size(outer_size());
}
- /// @returns The number of usable bytes inside a block with the given outer
- /// size were it to be allocated.
static size_t inner_size(size_t outer_size) {
// The usable region includes the prev_ field of the next block.
- return inner_size_free(outer_size) + sizeof(prev_);
- }
-
- /// @returns The number of usable bytes inside the block if it remains free.
- size_t inner_size_free() const {
- if (!next())
- return 0;
- return inner_size_free(outer_size());
- }
-
- /// @returns The number of usable bytes inside a block with the given outer
- /// size if it remains free.
- static size_t inner_size_free(size_t outer_size) {
- return outer_size - BLOCK_OVERHEAD;
+ return outer_size - BLOCK_OVERHEAD + sizeof(prev_);
}
/// @returns A pointer to the usable space inside this block.
@@ -217,11 +201,14 @@ class Block {
/// Attempts to split this block.
///
- /// If successful, the block will have an inner size of at least
- /// `new_inner_size`, rounded to ensure that the split point is on an
- /// ALIGNMENT boundary. The remaining space will be returned as a new block.
- /// Note that the prev_ field of the next block counts as part of the inner
- /// size of the returnd block.
+ /// If successful, the block will have an inner size of `new_inner_size`,
+ /// rounded to ensure that the split point is on an ALIGNMENT boundary. The
+ /// remaining space will be returned as a new block. Note that the prev_ field
+ /// of the next block counts as part of the inner size of the returnd block.
+ ///
+ /// This method may fail if the remaining space is too small to hold a new
+ /// block. If this method fails for any reason, the original block is
+ /// unmodified.
optional<Block *> split(size_t new_inner_size);
/// Merges this block with the one that comes after it.
@@ -455,7 +442,7 @@ Block<OffsetType, kAlign>::split(size_t new_inner_size) {
// The prev_ field of the next block is always available, so there is a
// minimum size to a block created through splitting.
if (new_inner_size < sizeof(prev_))
- new_inner_size = sizeof(prev_);
+ return {};
size_t old_inner_size = inner_size();
new_inner_size =
diff --git a/libc/src/__support/freelist.cpp b/libc/src/__support/freelist.cpp
deleted file mode 100644
index d3dd44895130cd..00000000000000
--- a/libc/src/__support/freelist.cpp
+++ /dev/null
@@ -1,42 +0,0 @@
-//===-- Implementation for freelist ---------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "freelist.h"
-
-namespace LIBC_NAMESPACE_DECL {
-
-void FreeList::push(Node *node) {
- if (begin_) {
- LIBC_ASSERT(Block<>::from_usable_space(node)->outer_size() ==
- begin_->block()->outer_size() &&
- "freelist entries must have the same size");
- // Since the list is circular, insert the node immediately before begin_.
- node->prev = begin_->prev;
- node->next = begin_;
- begin_->prev->next = node;
- begin_->prev = node;
- } else {
- begin_ = node->prev = node->next = node;
- }
-}
-
-void FreeList::remove(Node *node) {
- LIBC_ASSERT(begin_ && "cannot remove from empty list");
- if (node == node->next) {
- LIBC_ASSERT(node == begin_ &&
- "a self-referential node must be the only element");
- begin_ = nullptr;
- } else {
- node->prev->next = node->next;
- node->next->prev = node->prev;
- if (begin_ == node)
- begin_ = node->next;
- }
-}
-
-} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/freelist.h b/libc/src/__support/freelist.h
index eaeaeb013eeaec..a54cf953fe7ab6 100644
--- a/libc/src/__support/freelist.h
+++ b/libc/src/__support/freelist.h
@@ -1,4 +1,4 @@
-//===-- Interface for freelist --------------------------------------------===//
+//===-- Interface for freelist_malloc -------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -9,80 +9,200 @@
#ifndef LLVM_LIBC_SRC___SUPPORT_FREELIST_H
#define LLVM_LIBC_SRC___SUPPORT_FREELIST_H
-#include "block.h"
+#include "src/__support/CPP/array.h"
+#include "src/__support/CPP/cstddef.h"
+#include "src/__support/CPP/new.h"
+#include "src/__support/CPP/span.h"
+#include "src/__support/fixedvector.h"
+#include "src/__support/macros/config.h"
namespace LIBC_NAMESPACE_DECL {
-/// A circularly-linked FIFO list storing free Blocks. All Blocks on a list
-/// are the same size. The blocks are referenced by Nodes in the list; the list
-/// refers to these, but it does not own them.
+using cpp::span;
+
+/// Basic [freelist](https://en.wikipedia.org/wiki/Free_list) implementation
+/// for an allocator. This implementation buckets by chunk size, with a list
+/// of user-provided buckets. Each bucket is a linked list of storage chunks.
+/// Because this freelist uses the added chunks themselves as list nodes, there
+/// is a lower bound of `sizeof(FreeList.FreeListNode)` bytes for chunks which
+/// can be added to this freelist. There is also an implicit bucket for
+/// "everything else", for chunks which do not fit into a bucket.
+///
+/// Each added chunk will be added to the smallest bucket under which it fits.
+/// If it does not fit into any user-provided bucket, it will be added to the
+/// default bucket.
+///
+/// As an example, assume that the `FreeList` is configured with buckets of
+/// sizes {64, 128, 256, and 512} bytes. The internal state may look like the
+/// following:
///
-/// Allocating free blocks in FIFO order maximizes the amount of time before a
-/// free block is reused. This in turn maximizes the number of opportunities for
-/// it to be coalesced with an adjacent block, which tends to reduce heap
-/// fragmentation.
-class FreeList {
+/// @code{.unparsed}
+/// bucket[0] (64B) --> chunk[12B] --> chunk[42B] --> chunk[64B] --> NULL
+/// bucket[1] (128B) --> chunk[65B] --> chunk[72B] --> NULL
+/// bucket[2] (256B) --> NULL
+/// bucket[3] (512B) --> chunk[312B] --> chunk[512B] --> chunk[416B] --> NULL
+/// bucket[4] (implicit) --> chunk[1024B] --> chunk[513B] --> NULL
+/// @endcode
+///
+/// Note that added chunks should be aligned to a 4-byte boundary.
+template <size_t NUM_BUCKETS = 6> class FreeList {
public:
- class Node {
- public:
- /// @returns The block containing this node.
- LIBC_INLINE const Block<> *block() const {
- return Block<>::from_usable_space(this);
+ // Remove copy/move ctors
+ FreeList(const FreeList &other) = delete;
+ FreeList(FreeList &&other) = delete;
+ FreeList &operator=(const FreeList &other) = delete;
+ FreeList &operator=(FreeList &&other) = delete;
+
+ /// Adds a chunk to this freelist.
+ bool add_chunk(cpp::span<cpp::byte> chunk);
+
+ /// Finds an eligible chunk for an allocation of size `size`.
+ ///
+ /// @note This returns the first allocation possible within a given bucket;
+ /// It does not currently optimize for finding the smallest chunk.
+ ///
+ /// @returns
+ /// * On success - A span representing the chunk.
+ /// * On failure (e.g. there were no chunks available for that allocation) -
+ /// A span with a size of 0.
+ cpp::span<cpp::byte> find_chunk(size_t size) const;
+
+ template <typename Cond> cpp::span<cpp::byte> find_chunk_if(Cond op) const;
+
+ /// Removes a chunk from this freelist.
+ bool remove_chunk(cpp::span<cpp::byte> chunk);
+
+ /// For a given size, find which index into chunks_ the node should be written
+ /// to.
+ constexpr size_t find_chunk_ptr_for_size(size_t size, bool non_null) const;
+
+ struct FreeListNode {
+ FreeListNode *next;
+ size_t size;
+ };
+
+ constexpr void set_freelist_node(FreeListNode &node,
+ cpp::span<cpp::byte> chunk);
+
+ constexpr explicit FreeList(const cpp::array<size_t, NUM_BUCKETS> &sizes)
+ : chunks_(NUM_BUCKETS + 1, 0), sizes_(sizes.begin(), sizes.end()) {}
+
+private:
+ FixedVector<FreeList::FreeListNode *, NUM_BUCKETS + 1> chunks_;
+ FixedVector<size_t, NUM_BUCKETS> sizes_;
+};
+
+template <size_t NUM_BUCKETS>
+constexpr void FreeList<NUM_BUCKETS>::set_freelist_node(FreeListNode &node,
+ span<cpp::byte> chunk) {
+ // Add it to the correct list.
+ size_t chunk_ptr = find_chunk_ptr_for_size(chunk.size(), false);
+ node.size = chunk.size();
+ node.next = chunks_[chunk_ptr];
+ chunks_[chunk_ptr] = &node;
+}
+
+template <size_t NUM_BUCKETS>
+bool FreeList<NUM_BUCKETS>::add_chunk(span<cpp::byte> chunk) {
+ // Check that the size is enough to actually store what we need
+ if (chunk.size() < sizeof(FreeListNode))
+ return false;
+
+ FreeListNode *node = ::new (chunk.data()) FreeListNode;
+ set_freelist_node(*node, chunk);
+
+ return true;
+}
+
+template <size_t NUM_BUCKETS>
+template <typename Cond>
+span<cpp::byte> FreeList<NUM_BUCKETS>::find_chunk_if(Cond op) const {
+ for (FreeListNode *node : chunks_) {
+ while (node != nullptr) {
+ span<cpp::byte> chunk(reinterpret_cast<cpp::byte *>(node), node->size);
+ if (op(chunk))
+ return chunk;
+
+ node = node->next;
}
+ }
- /// @returns The block containing this node.
- LIBC_INLINE Block<> *block() { return Block<>::from_usable_space(this); }
+ return {};
+}
- /// @returns The inner size of blocks in the list containing this node.
- LIBC_INLINE size_t size() const { return block()->inner_size(); }
+template <size_t NUM_BUCKETS>
+span<cpp::byte> FreeList<NUM_BUCKETS>::find_chunk(size_t size) const {
+ if (size == 0)
+ return span<cpp::byte>();
- private:
- // Circularly linked pointers to adjacent nodes.
- Node *prev;
- Node *next;
- friend class FreeList;
- };
+ size_t chunk_ptr = find_chunk_ptr_for_size(size, true);
+
+ // Check that there's data. This catches the case where we run off the
+ // end of the array
+ if (chunks_[chunk_ptr] == nullptr)
+ return span<cpp::byte>();
- LIBC_INLINE constexpr FreeList() : FreeList(nullptr) {}
- LIBC_INLINE constexpr FreeList(Node *begin) : begin_(begin) {}
+ // Now iterate up the buckets, walking each list to find a good candidate
+ for (size_t i = chunk_ptr; i < chunks_.size(); i++) {
+ FreeListNode *node = chunks_[static_cast<unsigned short>(i)];
- LIBC_INLINE bool empty() const { return !begin_; }
+ while (node != nullptr) {
+ if (node->size >= size)
+ return span<cpp::byte>(reinterpret_cast<cpp::byte *>(node), node->size);
- /// @returns The inner size of blocks ...
[truncated]
|
mysterymath
added a commit
that referenced
this pull request
Nov 21, 2024
…117065) - Fix assertion expressions. - Fix incorrect small size in freestore_test. - There may only be one small size for high alignment and small pointers (riscv32). - Don't rely on stack alignment in FreeList test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #106259
Unit tests break on AArch64.