[libc-commits] [libc] Revert "[libc] Use best-fit binary trie to make malloc logarithmic" (PR #117065)
via libc-commits
libc-commits at lists.llvm.org
Wed Nov 20 14:00:44 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libc
Author: Daniel Thornburgh (mysterymath)
<details>
<summary>Changes</summary>
Reverts 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:
- (modified) libc/fuzzing/__support/CMakeLists.txt (-8)
- (removed) libc/fuzzing/__support/freelist_heap_fuzz.cpp (-227)
- (modified) libc/src/__support/CMakeLists.txt (+2-24)
- (modified) libc/src/__support/block.h (+11-24)
- (removed) libc/src/__support/freelist.cpp (-42)
- (modified) libc/src/__support/freelist.h (+174-54)
- (modified) libc/src/__support/freelist_heap.h (+85-68)
- (removed) libc/src/__support/freestore.h (-114)
- (removed) libc/src/__support/freetrie.cpp (-64)
- (removed) libc/src/__support/freetrie.h (-237)
- (modified) libc/src/stdlib/freelist_malloc.cpp (+2-2)
- (modified) libc/test/src/__support/CMakeLists.txt (-27)
- (modified) libc/test/src/__support/block_test.cpp (+14)
- (modified) libc/test/src/__support/freelist_heap_test.cpp (+28-25)
- (modified) libc/test/src/__support/freelist_malloc_test.cpp (+6-5)
- (modified) libc/test/src/__support/freelist_test.cpp (+150-35)
- (removed) libc/test/src/__support/freestore_test.cpp (-101)
- (removed) libc/test/src/__support/freetrie_test.cpp (-125)
``````````diff
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/117065
More information about the libc-commits
mailing list