Revert D130865756

Summary:
This diff reverts D130865756
The original diff was mistakenly reverted after the issue has been fixed.

Depends on D130865756

Reviewed By: vitaut

Differential Revision: D130936823

fbshipit-source-id: 217e3ddc7088b614e7fb077ae0f3beec1f724497

Author: generatedunixname89002005232357

folly/memory/Arena-inl.h

@@ -28,24 +28,26 @@ template <class Alloc>
 void* Arena<Alloc>::allocateSlow(size_t size) {
   char* start;
 
+  const bool large = size > minBlockSize();
   size_t allocSize;
   if (!checked_add(
-          &allocSize, std::max(size, minBlockSize()), roundUp(sizeof(Block)))) {
+          &allocSize,
+          std::max(size, minBlockSize()),
+          large ? headerSize<LargeBlock>() : headerSize<Block>())) {
     throw_exception<std::bad_alloc>();
   }
   if (sizeLimit_ != kNoSizeLimit &&
       allocSize > sizeLimit_ - totalAllocatedSize_) {
     throw_exception<std::bad_alloc>();
   }
 
-  if (size > minBlockSize()) {
+  if (large) {
     // Allocate a large block for this chunk only; don't change ptr_ and end_,
     // let them point into a normal block (or none, if they're null)
-    allocSize = roundUp(sizeof(LargeBlock)) + size;
     void* mem = AllocTraits::allocate(alloc(), allocSize);
     auto blk = new (mem) LargeBlock(allocSize);
     start = align(blk->start());
-    largeBlocks_.push_back(*blk);
+    append(firstLargeBlock_, lastLargeBlock_, blk, blk);
   } else {
     // Allocate a normal sized block and carve out size bytes from it
     // Will allocate more than size bytes if convenient
@@ -55,8 +57,8 @@ void* Arena<Alloc>::allocateSlow(size_t size) {
     void* mem = AllocTraits::allocate(alloc(), allocSize);
     auto blk = new (mem) Block();
     start = align(blk->start());
-    blocks_.push_back(*blk);
-    currentBlock_ = blocks_.last();
+    append(firstBlock_, lastBlock_, blk, blk);
+    currentBlock_ = blk;
     ptr_ = start + size;
     end_ = static_cast<char*>(mem) + allocSize;
     assert(ptr_ <= end_);
@@ -71,16 +73,26 @@ void Arena<Alloc>::merge(Arena<Alloc>&& other) {
   FOLLY_SAFE_CHECK(
       blockGoodAllocSize() == other.blockGoodAllocSize(),
       "cannot merge arenas of different minBlockSize");
-  const bool wasEmpty = blocks_.empty();
-  blocks_.splice_after(blocks_.before_begin(), other.blocks_);
-  other.blocks_.clear();
-  if (wasEmpty) {
-    // currentBlock_ is still before_begin(); leaving it there would let
-    // canReuseExistingBlock() bump-allocate into a merged block.
-    currentBlock_ = blocks_.last();
+  // Put the other arena's blocks in front of ours, which keeps them out of
+  // reach of canReuseExistingBlock(). Without blocks of our own, they are all
+  // in use, so the last one becomes current.
+  if (other.firstBlock_) {
+    append(other.firstBlock_, other.lastBlock_, firstBlock_, lastBlock_);
+    firstBlock_ = other.firstBlock_;
+    lastBlock_ = other.lastBlock_;
+    if (!currentBlock_) {
+      currentBlock_ = lastBlock_;
+    }
   }
-  largeBlocks_.splice_after(largeBlocks_.before_begin(), other.largeBlocks_);
-  other.largeBlocks_.clear();
+  append(
+      other.firstLargeBlock_,
+      other.lastLargeBlock_,
+      firstLargeBlock_,
+      lastLargeBlock_);
+  firstLargeBlock_ = other.firstLargeBlock_;
+  lastLargeBlock_ = other.lastLargeBlock_;
+  other.firstBlock_ = other.lastBlock_ = other.currentBlock_ = nullptr;
+  other.firstLargeBlock_ = other.lastLargeBlock_ = nullptr;
   other.ptr_ = other.end_ = nullptr;
   totalAllocatedSize_ += other.totalAllocatedSize_;
   other.totalAllocatedSize_ = 0;

folly/memory/Arena.h

@@ -17,13 +17,12 @@
 #pragma once
 #define FOLLY_ARENA_H_
 
+#include <algorithm>
 #include <cassert>
 #include <limits>
 #include <stdexcept>
 #include <utility>
 
-#include <boost/intrusive/slist.hpp>
-
 #include <folly/Conv.h>
 #include <folly/Likely.h>
 #include <folly/Memory.h>
@@ -67,7 +66,6 @@ class Arena {
       size_t sizeLimit = kNoSizeLimit,
       size_t maxAlign = kDefaultMaxAlign)
       : allocAndSize_(alloc, minBlockSize),
-        currentBlock_(blocks_.last()),
         ptr_(nullptr),
         end_(nullptr),
         totalAllocatedSize_(0),
@@ -99,7 +97,7 @@ class Arena {
     }
 
     if (canReuseExistingBlock(size)) {
-      currentBlock_++;
+      currentBlock_ = currentBlock_->next;
       char* r = align(currentBlock_->start());
       ptr_ = r + size;
       end_ = currentBlock_->start() + blockGoodAllocSize() - sizeof(Block);
@@ -124,10 +122,10 @@ class Arena {
   void clear() {
     bytesUsed_ = 0;
     freeLargeBlocks(); // We don't reuse large blocks
-    if (blocks_.empty()) {
+    if (!firstBlock_) {
       return;
     }
-    currentBlock_ = blocks_.begin();
+    currentBlock_ = firstBlock_;
     ptr_ = align(currentBlock_->start());
     end_ = currentBlock_->start() + blockGoodAllocSize() - sizeof(Block);
     assert(ptr_ <= end_);
@@ -151,10 +149,8 @@ class Arena {
  private:
   using AllocTraits =
       typename std::allocator_traits<Alloc>::template rebind_traits<char>;
-  using BlockLink = boost::intrusive::slist_member_hook<>;
-
   struct alignas(max_align_v) Block {
-    BlockLink link;
+    Block* next = nullptr;
 
     char* start() { return reinterpret_cast<char*>(this + 1); }
 
@@ -164,11 +160,11 @@ class Arena {
 
   constexpr size_t blockGoodAllocSize() {
     return ArenaAllocatorTraits<Alloc>::goodSize(
-        alloc(), roundUp(sizeof(Block)) + minBlockSize());
+        alloc(), headerSize<Block>() + minBlockSize());
   }
 
   struct alignas(max_align_v) LargeBlock {
-    BlockLink link;
+    LargeBlock* next = nullptr;
     const size_t allocSize;
 
     char* start() { return reinterpret_cast<char*>(this + 1); }
@@ -182,35 +178,39 @@ class Arena {
       // We don't reuse large blocks
       return false;
     }
-    if (blocks_.empty() || currentBlock_ == blocks_.last()) {
-      // No regular blocks to reuse
-      return false;
-    }
-    return true;
+    // No regular blocks to reuse past the current one
+    return currentBlock_ != lastBlock_;
   }
 
   void freeBlocks() {
-    blocks_.clear_and_dispose([this](Block* b) {
+    for (Block* b = firstBlock_; b;) {
+      Block* const next = b->next;
       b->~Block();
       AllocTraits::deallocate(
           alloc(), reinterpret_cast<char*>(b), blockGoodAllocSize());
-    });
+      b = next;
+    }
+    firstBlock_ = lastBlock_ = currentBlock_ = nullptr;
   }
 
   void freeLargeBlocks() {
-    largeBlocks_.clear_and_dispose([this](LargeBlock* b) {
+    for (LargeBlock* b = firstLargeBlock_; b;) {
+      LargeBlock* const next = b->next;
       auto size = b->allocSize;
       totalAllocatedSize_ -= size;
       b->~LargeBlock();
       AllocTraits::deallocate(alloc(), reinterpret_cast<char*>(b), size);
-    });
+      b = next;
+    }
+    firstLargeBlock_ = lastLargeBlock_ = nullptr;
   }
 
  public:
   static constexpr size_t kDefaultMinBlockSize = 4096 - sizeof(Block);
   static constexpr size_t kNoSizeLimit = 0;
   static constexpr size_t kDefaultMaxAlign = alignof(Block);
   static constexpr size_t kBlockOverhead = sizeof(Block);
+  static constexpr size_t kLargeBlockOverhead = sizeof(LargeBlock);
 
  private:
   bool isAligned(uintptr_t address) const {
@@ -232,19 +232,23 @@ class Arena {
 
   char* align(char* ptr) { return align_ceil(ptr, maxAlign_); }
 
-  // cache_last<true> makes the list keep a pointer to the last element, so we
-  // have push_back() and constant time splice_after()
-  using BlockList = boost::intrusive::slist<
-      Block,
-      boost::intrusive::member_hook<Block, BlockLink, &Block::link>,
-      boost::intrusive::constant_time_size<false>,
-      boost::intrusive::cache_last<true>>;
+  // A block header plus the most padding align() can add after it, since the
+  // allocator only guarantees alignment for the header.
+  template <typename Header>
+  size_t headerSize() const {
+    return sizeof(Header) + std::max(maxAlign_, alignof(Header)) -
+        alignof(Header);
+  }
 
-  using LargeBlockList = boost::intrusive::slist<
-      LargeBlock,
-      boost::intrusive::member_hook<LargeBlock, BlockLink, &LargeBlock::link>,
-      boost::intrusive::constant_time_size<false>,
-      boost::intrusive::cache_last<true>>;
+  // Appends the chain [first, last], if not empty, to the chain [head, tail].
+  template <typename B>
+  static void append(B*& head, B*& tail, B* first, B* last) {
+    if (!first) {
+      return;
+    }
+    (tail ? tail->next : head) = first;
+    tail = last;
+  }
 
   void* allocateSlow(size_t size);
 
@@ -262,9 +266,14 @@ class Arena {
   const Alloc& alloc() const { return allocAndSize_; }
 
   AllocAndSize allocAndSize_;
-  BlockList blocks_;
-  typename BlockList::iterator currentBlock_;
-  LargeBlockList largeBlocks_;
+  // Blocks are chained through their next pointers. Blocks after
+  // currentBlock_ are free for reuse; currentBlock_ is null only when there
+  // are no blocks.
+  Block* firstBlock_ = nullptr;
+  Block* lastBlock_ = nullptr;
+  Block* currentBlock_ = nullptr;
+  LargeBlock* firstLargeBlock_ = nullptr;
+  LargeBlock* lastLargeBlock_ = nullptr;
   char* ptr_;
   char* end_;
   size_t totalAllocatedSize_;

folly/memory/BUCK

@@ -26,7 +26,6 @@ fb_dirsync_cpp_library(
     ],
     use_raw_headers = True,
     exported_deps = [
-        "fbsource//third-party/boost:boost",
         ":allocator",
         ":malloc",
         "//folly:conv",

folly/memory/CMakeLists.txt

@@ -32,7 +32,6 @@ folly_add_library(
     Arena-inl.h
     Arena.h
   EXPORTED_DEPS
-    Boost::headers
     folly_conv
     folly_lang_align
     folly_lang_checked_math

folly/memory/test/ArenaTest.cpp

@@ -22,6 +22,7 @@
 
 #include <glog/logging.h>
 
+#include <folly/Memory.h>
 #include <folly/Random.h>
 #include <folly/memory/Allocator.h>
 #include <folly/memory/MallctlHelper.h>
@@ -86,11 +87,7 @@ TEST(Arena, SizeSanity) {
   allocatedItems.insert(ptr);
   minimum_size += 10 * requestedBlockSize;
   maximum_size +=
-      goodMallocSize(10 * requestedBlockSize + SysArena::kBlockOverhead);
-#if defined(__APPLE__) && defined(FOLLY_AARCH64)
-  // expectation is a bit too small
-  maximum_size += 8;
-#endif
+      goodMallocSize(10 * requestedBlockSize + SysArena::kLargeBlockOverhead);
   EXPECT_GE(arena.totalSize(), minimum_size);
   EXPECT_LE(arena.totalSize(), maximum_size);
   VLOG(4) << minimum_size << " < " << arena.totalSize() << " < "
@@ -263,6 +260,70 @@ TEST(Arena, ExtremeSize) {
   EXPECT_THROW(alloc(arena, SIZE_MAX - 2), std::bad_alloc);
 }
 
+TEST(Arena, SizeLimitCoversLargeBlockHeader) {
+  static const size_t size = 4096;
+
+  for (size_t sizeLimit = size; sizeLimit < size + 64; ++sizeLimit) {
+    SCOPED_TRACE(sizeLimit);
+    SysArena arena(64, sizeLimit);
+    try {
+      void* ptr = alloc(arena, size);
+      EXPECT_EQ(
+          reinterpret_cast<uintptr_t>(ptr) % SysArena::kDefaultMaxAlign, 0);
+    } catch (const std::bad_alloc&) {
+      continue;
+    }
+    EXPECT_LE(arena.totalSize() - sizeof(SysArena), sizeLimit);
+  }
+}
+
+TEST(Arena, LargeBlockSizeOverflow) {
+  SysArena arena;
+  // Adding the large block header to this size overflows.
+  EXPECT_THROW(arena.allocate(SIZE_MAX - 15), std::bad_alloc);
+}
+
+namespace {
+// Returns memory at a given offset past a 64-byte boundary, followed by guard
+// bytes that must stay untouched.
+struct OffsetAllocator {
+  using value_type = char;
+  template <typename T>
+  struct rebind {
+    using other = OffsetAllocator;
+  };
+  static constexpr size_t kGuard = 64;
+  size_t offset = 0;
+
+  char* allocate(size_t n) {
+    auto p = static_cast<char*>(aligned_malloc(offset + n + kGuard, 64));
+    memset(p + offset + n, 0x5a, kGuard);
+    return p + offset;
+  }
+  void deallocate(char* p, size_t n) {
+    for (size_t i = 0; i < kGuard; ++i) {
+      EXPECT_EQ(p[n + i], 0x5a) << "overrun at " << i;
+    }
+    aligned_free(p - offset);
+  }
+};
+} // namespace
+
+TEST(Arena, LargeBlockPaddingForCustomAlign) {
+  // The allocator only guarantees the alignment of the block header, so a
+  // large block's payload may need up to 64 - that alignment bytes of padding.
+  for (size_t offset = 0; offset < 64; offset += max_align_v) {
+    SCOPED_TRACE(offset);
+    Arena<OffsetAllocator> arena(
+        OffsetAllocator{offset}, 64, Arena<OffsetAllocator>::kNoSizeLimit, 64);
+    for (size_t size : {128, 1024}) {
+      void* ptr = arena.allocate(size);
+      EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % 64, 0);
+      memset(ptr, 0xff, size);
+    }
+  }
+}
+
 TEST(Arena, MaxAlign) {
   // Results are still aligned with nonstandard size and allocations
   static const size_t blockSize = 123;
@@ -381,7 +442,7 @@ TEST(Arena, MergeIntoUntouchedArena) {
   auto* live = static_cast<char*>(alloc(src, 1));
   *live = 'x';
 
-  // dst has never allocated, so its currentBlock_ is still before_begin().
+  // dst has never allocated, so it has no current block.
   SysArena dst;
   dst.merge(std::move(src));
 

folly/memory/test/BUCK

@@ -9,6 +9,7 @@ fb_dirsync_cpp_unittest(
     labels = ["oss-broken"],
     deps = [
         "fbsource//third-party/glog:glog",
+        "//folly:memory",
         "//folly:random",
         "//folly/memory:allocator",
         "//folly/memory:arena",