Use best-fit strategy in Arena, now O(log(n)) instead O(n)

Summary:
PR12048:
5fbf7c4 fix nits: variable naming, typos (Martin Ankerl)
1e0ee90 Use best-fit strategy in Arena, now O(log(n)) instead O(n) (Martin Ankerl)

Pull request description:

  This replaces the first-fit algorithm used in the Arena with a best-fit. According to "Dynamic Storage Allocation: A Survey and Critical Review", Wilson et. al. 1995, http://www.scs.stanford.edu/14wi-cs140/sched/readings/wilson.pdf, both startegies work well in practice.

  The advantage of using best-fit is that we can switch the O(n) allocation to O(log(n)). Additionally, some previously O(log(n)) operations are now O(1) operations by using hash maps. The end effect is that the benchmark runs about 2.5 times faster on my machine:

      # Benchmark, evals, iterations, total, min, max, median
      old: BenchLockedPool, 5, 530, 5.25749, 0.00196938, 0.00199755, 0.00198172
      new: BenchLockedPool, 5, 1300, 5.11313, 0.000781493, 0.000793314, 0.00078606

  I've run all unit tests and benchmarks, and increased the number of iterations so that BenchLockedPool takes about 5 seconds again.

Tree-SHA512: 6551e384671f93f10c60df530a29a1954bd265cc305411f665a8756525e5afe2873a8032c797d00b6e8c07e16d9827465d0b662875433147381474a44119ccce

Also included PR16161 which fixes a bug introduced by PR12048:
  Changes in #12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Second commit fixes segfault in allocator_tests/arena_tests
  The test uses reinterpret_cast<void*> on unallocated memory. Using this
  memory in printchunk as char* causes a segfault, so have printchunk take
  void* instead.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

Backport of Core PR12048 and PR16161
bitcoin/bitcoin#12048
bitcoin/bitcoin#16161

Reviewer note: the last commit in PR16161 is TRAVIS related and was therefore modified to our code base.

Test Plan:
  make check
  test_runner.py
  ./bitcoin-qt -> settings -> Encrypt Wallet -> set password and/or change password if already encrypted
  ./bench_bitcoin
old: BenchLockedPool, 5, 530, 5.15069, 0.00192059, 0.00199956, 0.00193796
new: BenchLockedPool, 5, 1300, 4.44411, 0.00067821, 0.000698535, 0.000678916

  ../configure CPPFLAGS=-DARENA_DEBUG
  make check
  test_runner.py
  ABC_BUILD_NAME=build-asan build-configurations.sh

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3898

Author: laanwj

contrib/teamcity/build-configurations.sh

@@ -61,6 +61,7 @@ case "$ABC_BUILD_NAME" in
   build-asan)
     # Build with the address sanitizer, then run unit tests and functional tests.
     CMAKE_FLAGS=(
+      "-DCMAKE_CXX_FLAGS=-DARENA_DEBUG"
       "-DCMAKE_BUILD_TYPE=Debug"
       # ASAN does not support assembly code: https://github.com/google/sanitizers/issues/192
       # This will trigger a segfault if the SSE4 implementation is selected for SHA256.

src/bench/lockedpool.cpp

@@ -41,4 +41,4 @@ static void BenchLockedPool(benchmark::State &state) {
     addr.clear();
 }
 
-BENCHMARK(BenchLockedPool, 530);
+BENCHMARK(BenchLockedPool, 1300);

src/support/lockedpool.cpp

@@ -28,6 +28,10 @@
 
 #include <algorithm>
 #include <memory>
+#ifdef ARENA_DEBUG
+#include <iomanip>
+#include <iostream>
+#endif
 
 LockedPoolManager *LockedPoolManager::_instance = nullptr;
 std::once_flag LockedPoolManager::init_flag;
@@ -47,7 +51,9 @@ Arena::Arena(void *base_in, size_t size_in, size_t alignment_in)
     : base(static_cast<char *>(base_in)),
       end(static_cast<char *>(base_in) + size_in), alignment(alignment_in) {
     // Start with one free chunk that covers the entire arena
-    chunks_free.emplace(base, size_in);
+    auto it = size_to_free_chunk.emplace(size_in, base);
+    chunks_free.emplace(base, it);
+    chunks_free_end.emplace(base + size_in, it);
 }
 
 Arena::~Arena() {}
@@ -61,33 +67,36 @@ void *Arena::alloc(size_t size) {
         return nullptr;
     }
 
-    // Pick a large enough free-chunk
-    auto it =
-        std::find_if(chunks_free.begin(), chunks_free.end(),
-                     [=](const std::map<char *, size_t>::value_type &chunk) {
-                         return chunk.second >= size;
-                     });
-    if (it == chunks_free.end()) {
+    // Pick a large enough free-chunk. Returns an iterator pointing to the first
+    // element that is not less than key. This allocation strategy is best-fit.
+    // According to "Dynamic Storage Allocation: A Survey and Critical Review",
+    // Wilson et. al. 1995,
+    // http://www.scs.stanford.edu/14wi-cs140/sched/readings/wilson.pdf,
+    // best-fit and first-fit policies seem to work well in practice.
+    auto size_ptr_it = size_to_free_chunk.lower_bound(size);
+    if (size_ptr_it == size_to_free_chunk.end()) {
         return nullptr;
     }
 
     // Create the used-chunk, taking its space from the end of the free-chunk
+    const size_t size_remaining = size_ptr_it->first - size;
     auto alloced =
-        chunks_used.emplace(it->first + it->second - size, size).first;
-    if (!(it->second -= size)) {
-        chunks_free.erase(it);
+        chunks_used.emplace(size_ptr_it->second + size_remaining, size).first;
+    chunks_free_end.erase(size_ptr_it->second + size_ptr_it->first);
+    if (size_ptr_it->first == size) {
+        // whole chunk is used up
+        chunks_free.erase(size_ptr_it->second);
+    } else {
+        // still some memory left in the chunk
+        auto it_remaining =
+            size_to_free_chunk.emplace(size_remaining, size_ptr_it->second);
+        chunks_free[size_ptr_it->second] = it_remaining;
+        chunks_free_end.emplace(size_ptr_it->second + size_remaining,
+                                it_remaining);
     }
-    return reinterpret_cast<void *>(alloced->first);
-}
+    size_to_free_chunk.erase(size_ptr_it);
 
-/* extend the Iterator if other begins at its end */
-template <class Iterator, class Pair>
-bool extend(Iterator it, const Pair &other) {
-    if (it->first + it->second == other.first) {
-        it->second += other.second;
-        return true;
-    }
-    return false;
+    return reinterpret_cast<void *>(alloced->first);
 }
 
 void Arena::free(void *ptr) {
@@ -101,19 +110,30 @@ void Arena::free(void *ptr) {
     if (i == chunks_used.end()) {
         throw std::runtime_error("Arena: invalid or double free");
     }
-    auto freed = *i;
+    std::pair<char *, size_t> freed = *i;
     chunks_used.erase(i);
 
-    // Add space to free map, coalescing contiguous chunks
-    auto next = chunks_free.upper_bound(freed.first);
-    auto prev =
-        (next == chunks_free.begin()) ? chunks_free.end() : std::prev(next);
-    if (prev == chunks_free.end() || !extend(prev, freed)) {
-        prev = chunks_free.emplace_hint(next, freed);
+    // coalesce freed with previous chunk
+    auto prev = chunks_free_end.find(freed.first);
+    if (prev != chunks_free_end.end()) {
+        freed.first -= prev->second->first;
+        freed.second += prev->second->first;
+        size_to_free_chunk.erase(prev->second);
+        chunks_free_end.erase(prev);
     }
-    if (next != chunks_free.end() && extend(prev, *next)) {
+
+    // coalesce freed with chunk after freed
+    auto next = chunks_free.find(freed.first + freed.second);
+    if (next != chunks_free.end()) {
+        freed.second += next->second->first;
+        size_to_free_chunk.erase(next->second);
         chunks_free.erase(next);
     }
+
+    // Add/set space with coalesced free chunk
+    auto it = size_to_free_chunk.emplace(freed.second, freed.first);
+    chunks_free[freed.first] = it;
+    chunks_free_end[freed.first + freed.second] = it;
 }
 
 Arena::Stats Arena::stats() const {
@@ -122,14 +142,14 @@ Arena::Stats Arena::stats() const {
         r.used += chunk.second;
     }
     for (const auto &chunk : chunks_free) {
-        r.free += chunk.second;
+        r.free += chunk.second->first;
     }
     r.total = r.used + r.free;
     return r;
 }
 
 #ifdef ARENA_DEBUG
-static void printchunk(char *base, size_t sz, bool used) {
+static void printchunk(void *base, size_t sz, bool used) {
     std::cout << "0x" << std::hex << std::setw(16) << std::setfill('0') << base
               << " 0x" << std::hex << std::setw(16) << std::setfill('0') << sz
               << " 0x" << used << std::endl;
@@ -140,7 +160,7 @@ void Arena::walk() const {
     }
     std::cout << std::endl;
     for (const auto &chunk : chunks_free) {
-        printchunk(chunk.first, chunk.second, false);
+        printchunk(chunk.first, chunk.second->first, false);
     }
     std::cout << std::endl;
 }

src/support/lockedpool.h

@@ -10,6 +10,7 @@
 #include <map>
 #include <memory>
 #include <mutex>
+#include <unordered_map>
 
 /**
  * OS-dependent allocation and deallocation of locked/pinned memory pages.
@@ -92,12 +93,20 @@ class Arena {
     bool addressInArena(void *ptr) const { return ptr >= base && ptr < end; }
 
 private:
-    /**
-     * Map of chunk address to chunk information. This class makes use of the
-     * sorted order to merge previous and next chunks during deallocation.
-     */
-    std::map<char *, size_t> chunks_free;
-    std::map<char *, size_t> chunks_used;
+    typedef std::multimap<size_t, char *> SizeToChunkSortedMap;
+    /** Map to enable O(log(n)) best-fit allocation, as it's sorted by size */
+    SizeToChunkSortedMap size_to_free_chunk;
+
+    typedef std::unordered_map<char *, SizeToChunkSortedMap::const_iterator>
+        ChunkToSizeMap;
+    /** Map from begin of free chunk to its node in size_to_free_chunk */
+    ChunkToSizeMap chunks_free;
+    /** Map from end of free chunk to its node in size_to_free_chunk */
+    ChunkToSizeMap chunks_free_end;
+
+    /** Map from begin of used chunk to its size */
+    std::unordered_map<char *, size_t> chunks_used;
+
     /** Base address of arena */
     char *base;
     /** End address of arena */