Bug fix for superslab meta-data (#302)

* Replace time measuring macro

The DO_TIME macro was used originally to get performance numbers. The
macro makes tests hard to debug. This commit replaces it with a proper
C++ class with destructor.

* Bug fix

If the superslab meta data is large, then the calculation for the
sizeclasses that could use the short slab was incorrect.  This fixes
that calculation.

Co-authored-by: Nathaniel Wesley Filardo <nfilardo@microsoft.com>

src/mem/slab.h

@@ -32,8 +32,7 @@ namespace snmalloc
     static SNMALLOC_FAST_PATH void
     alloc_new_list(void*& bumpptr, FreeListIter& fast_free_list, size_t rsize)
     {
-      void* slab_end =
-        pointer_align_up<SLAB_SIZE>(pointer_offset(bumpptr, rsize));
+      void* slab_end = pointer_align_up<SLAB_SIZE>(pointer_offset(bumpptr, 1));
 
       FreeListBuilder b;
       SNMALLOC_ASSERT(b.empty());

src/mem/superslab.h

@@ -4,14 +4,32 @@
 #include "allocslab.h"
 #include "metaslab.h"
 
+#include <iostream>
 #include <new>
 
 namespace snmalloc
 {
+  /**
+   * Superslabs are, to first approximation, a `CHUNK_SIZE`-sized and -aligned
+   * region of address space, internally composed of a header (a `Superslab`
+   * structure) followed by an array of `Slab`s, each `SLAB_SIZE`-sized and
+   * -aligned.  Each active `Slab` holds an array of identically sized
+   * allocations strung on an invasive free list, which is lazily constructed
+   * from a bump-pointer allocator (see `Metaslab::alloc_new_list`).
+   *
+   * In order to minimize overheads, Slab metadata is held externally, in
+   * `Metaslab` structures; all `Metaslab`s for the Slabs within a Superslab are
+   * densely packed within the `Superslab` structure itself.  Moreover, as the
+   * `Superslab` structure is typically much smaller than `SLAB_SIZE`, a "short
+   * Slab" is overlaid with the `Superslab`.  This short Slab can hold only
+   * allocations that are smaller than the `SLAB_SIZE - sizeof(Superslab)`
+   * bytes; see `Superslab::is_short_sizeclass`.  The Metaslab state for a short
+   * slabs is constructed in a way that avoids branches on fast paths;
+   * effectively, the object slots that overlay the `Superslab` at the start are
+   * omitted from consideration.
+   */
   class Superslab : public Allocslab
   {
-    // This is the view of a 16 mb superslab when it is being used to allocate
-    // 64 kb slabs.
   private:
     friend DLList<Superslab>;
 
@@ -73,8 +91,17 @@ namespace snmalloc
 
     static bool is_short_sizeclass(sizeclass_t sizeclass)
     {
-      constexpr sizeclass_t h = size_to_sizeclass_const(sizeof(Superslab));
-      return sizeclass <= h;
+      static_assert(SLAB_SIZE > sizeof(Superslab), "Meta data requires this.");
+      /*
+       * size_to_sizeclass_const rounds *up* and returns the smallest class that
+       * could contain (and so may be larger than) the free space available for
+       * the short slab.  While we could detect the exact fit case and compare
+       * `<= h` therein, it's simpler to just treat this class as a strict upper
+       * bound and only permit strictly smaller classes in short slabs.
+       */
+      constexpr sizeclass_t h =
+        size_to_sizeclass_const(SLAB_SIZE - sizeof(Superslab));
+      return sizeclass < h;
     }
 
     void init(RemoteAllocator* alloc)

src/test/measuretime.h

@@ -3,12 +3,19 @@
 #include <chrono>
 #include <iomanip>
 #include <iostream>
+#include <sstream>
 
-#define DO_TIME(name, code) \
-  { \
-    auto start__ = std::chrono::high_resolution_clock::now(); \
-    code auto finish__ = std::chrono::high_resolution_clock::now(); \
-    auto diff__ = finish__ - start__; \
-    std::cout << name << ": " << std::setw(12) << diff__.count() << " ns" \
-              << std::endl; \
+class MeasureTime : public std::stringstream
+{
+  std::chrono::time_point<std::chrono::high_resolution_clock> start =
+    std::chrono::high_resolution_clock::now();
+
+public:
+  ~MeasureTime()
+  {
+    auto finish = std::chrono::high_resolution_clock::now();
+    auto diff = finish - start;
+    std::cout << str() << ": " << std::setw(12) << diff.count() << " ns"
+              << std::endl;
   }
+};

src/test/perf/contention/contention.cc

@@ -1,4 +1,3 @@
-#include "test/measuretime.h"
 #include "test/opt.h"
 #include "test/setup.h"
 #include "test/usage.h"

src/test/perf/external_pointer/externalpointer.cc

@@ -64,7 +64,9 @@ namespace test
 #endif
     setup(r, alloc);
 
-    DO_TIME("External pointer queries ", {
+    {
+      MeasureTime m;
+      m << "External pointer queries ";
       for (size_t i = 0; i < iterations; i++)
       {
         size_t rand = (size_t)r.next();
@@ -77,7 +79,7 @@ namespace test
         if (calced_external != external_ptr)
           abort();
       }
-    });
+    }
 
     teardown(alloc);
   }

src/test/perf/low_memory/low-memory.cc

@@ -1,6 +1,5 @@
 #include <iostream>
 #include <snmalloc.h>
-#include <test/measuretime.h>
 #include <test/opt.h>
 #include <test/setup.h>
 #include <unordered_set>

src/test/perf/singlethread/singlethread.cc

@@ -10,54 +10,55 @@ void test_alloc_dealloc(size_t count, size_t size, bool write)
 {
   auto* alloc = ThreadAlloc::get();
 
-  DO_TIME(
-    "Count: " << std::setw(6) << count << ", Size: " << std::setw(6) << size
-              << ", ZeroMem: " << (zero_mem == YesZero) << ", Write: " << write,
+  {
+    MeasureTime m;
+    m << "Count: " << std::setw(6) << count << ", Size: " << std::setw(6)
+      << size << ", ZeroMem: " << (zero_mem == YesZero) << ", Write: " << write;
+
+    std::unordered_set<void*> set;
+
+    // alloc 1.5x objects
+    for (size_t i = 0; i < ((count * 3) / 2); i++)
     {
-      std::unordered_set<void*> set;
+      void* p = alloc->alloc<zero_mem>(size);
+      SNMALLOC_CHECK(set.find(p) == set.end());
 
-      // alloc 1.5x objects
-      for (size_t i = 0; i < ((count * 3) / 2); i++)
-      {
-        void* p = alloc->alloc<zero_mem>(size);
-        SNMALLOC_CHECK(set.find(p) == set.end());
+      if (write)
+        *(int*)p = 4;
 
-        if (write)
-          *(int*)p = 4;
+      set.insert(p);
+    }
 
-        set.insert(p);
-      }
+    // free 0.25x of the objects
+    for (size_t i = 0; i < (count / 4); i++)
+    {
+      auto it = set.begin();
+      void* p = *it;
+      alloc->dealloc(p, size);
+      set.erase(it);
+      SNMALLOC_CHECK(set.find(p) == set.end());
+    }
 
-      // free 0.25x of the objects
-      for (size_t i = 0; i < (count / 4); i++)
-      {
-        auto it = set.begin();
-        void* p = *it;
-        alloc->dealloc(p, size);
-        set.erase(it);
-        SNMALLOC_CHECK(set.find(p) == set.end());
-      }
+    // alloc 1x objects
+    for (size_t i = 0; i < count; i++)
+    {
+      void* p = alloc->alloc<zero_mem>(size);
+      SNMALLOC_CHECK(set.find(p) == set.end());
 
-      // alloc 1x objects
-      for (size_t i = 0; i < count; i++)
-      {
-        void* p = alloc->alloc<zero_mem>(size);
-        SNMALLOC_CHECK(set.find(p) == set.end());
+      if (write)
+        *(int*)p = 4;
 
-        if (write)
-          *(int*)p = 4;
+      set.insert(p);
+    }
 
-        set.insert(p);
-      }
-
-      // free everything
-      while (!set.empty())
-      {
-        auto it = set.begin();
-        alloc->dealloc(*it, size);
-        set.erase(it);
-      }
-    });
+    // free everything
+    while (!set.empty())
+    {
+      auto it = set.begin();
+      alloc->dealloc(*it, size);
+      set.erase(it);
+    }
+  }
 
   current_alloc_pool()->debug_check_empty();
 }