Merge pull request #102 from microsoft/cq-free

Code quality for `free`

azure-pipelines.yml

@@ -203,6 +203,7 @@ jobs:
 
   - script: |
       make clangformat
+      git diff
       if [ "$(git diff $(Build.SourceVersion))" != "" ]; then exit 1; fi
 
     workingDirectory: build

src/mem/alloc.h

@@ -484,7 +484,7 @@ namespace snmalloc
       if (likely(size == PMSuperslab))
       {
         RemoteAllocator* target = super->get_allocator();
-        Slab* slab = Slab::get(p);
+        Slab* slab = Metaslab::get_slab(p);
         Metaslab& meta = super->get_meta(slab);
 
         // Reading a remote sizeclass won't fail, since the other allocator
@@ -552,7 +552,7 @@ namespace snmalloc
       Superslab* super = Superslab::get(p);
       if (size == PMSuperslab)
       {
-        Slab* slab = Slab::get(p);
+        Slab* slab = Metaslab::get_slab(p);
         Metaslab& meta = super->get_meta(slab);
 
         sizeclass_t sc = meta.sizeclass;
@@ -621,7 +621,7 @@ namespace snmalloc
 
         // Reading a remote sizeclass won't fail, since the other allocator
         // can't reuse the slab, as we have no yet deallocated this pointer.
-        Slab* slab = Slab::get(p);
+        Slab* slab = Metaslab::get_slab(p);
         Metaslab& meta = super->get_meta(slab);
 
         return sizeclass_to_size(meta.sizeclass);
@@ -968,7 +968,7 @@ namespace snmalloc
 #endif
       if (likely(super->get_kind() == Super))
       {
-        Slab* slab = Slab::get(p);
+        Slab* slab = Metaslab::get_slab(p);
         Metaslab& meta = super->get_meta(slab);
         if (likely(p->target_id() == id()))
         {
@@ -1000,7 +1000,7 @@ namespace snmalloc
       else
       {
         assert(likely(p->target_id() != id()));
-        Slab* slab = Slab::get(p);
+        Slab* slab = Metaslab::get_slab(p);
         Metaslab& meta = super->get_meta(slab);
         // Queue for remote dealloc elsewhere.
         remote.dealloc(p->target_id(), p, meta.sizeclass);
@@ -1210,7 +1210,7 @@ namespace snmalloc
     small_dealloc(Superslab* super, void* p, sizeclass_t sizeclass)
     {
 #ifdef CHECK_CLIENT
-      Slab* slab = Slab::get(p);
+      Slab* slab = Metaslab::get_slab(p);
       if (!slab->is_start_of_object(super, p))
       {
         error("Not deallocating start of an object");
@@ -1233,7 +1233,7 @@ namespace snmalloc
     SNMALLOC_FAST_PATH void small_dealloc_offseted_inner(
       Superslab* super, void* p, sizeclass_t sizeclass)
     {
-      Slab* slab = Slab::get(p);
+      Slab* slab = Metaslab::get_slab(p);
       if (likely(slab->dealloc_fast(super, p)))
         return;
 
@@ -1245,7 +1245,7 @@ namespace snmalloc
     {
       bool was_full = super->is_full();
       SlabList* sl = &small_classes[sizeclass];
-      Slab* slab = Slab::get(p);
+      Slab* slab = Metaslab::get_slab(p);
       Superslab::Action a =
         slab->dealloc_slow(sl, super, p, large_allocator.memory_provider);
       if (likely(a == Superslab::NoSlabReturn))

src/mem/metaslab.h

@@ -30,23 +30,29 @@ namespace snmalloc
   class Metaslab
   {
   public:
-    // How many entries are not in the free list of  slab.
+    /**
-    uint16_t used = 0;
+     *  Pointer to first free entry in this slab
+     *
+     *  The list will be (allocated - needed - 1) long. The -1 is
+     *  for the `link` element which is not in the free list.
+     */
+    void* head;
 
-    // How many entries have been allocated from this slab.
+    /**
+     *  How many entries are not in the free list of slab, i.e.
+     *  how many entries are needed to fully free this slab.
+     *
+     *  In the case of a fully allocated slab, where link==1 needed
+     *  will be 1. This enables 'return_object' to detect the slow path
+     *  case with a single operation subtract and test.
+     */
+    uint16_t needed = 0;
+
+    /**
+     *  How many entries have been allocated from this slab.
+     */
     uint16_t allocated;
 
-    // Index of first entry in this slab that forms the free
-    // list.  The list entries are stored as the first pointer
-    // in each unused object. The terminator is a pointer or
-    // offset into the block with the bottom bit set.  This means
-    //  I.e.
-    //    * an empty list has a head of 1.
-    //    * a one element list has an head contains an offset to this
-    //       this block, and then contains a pointer with the bottom
-    //       bit set.
-    Mod<SLAB_SIZE, uint16_t> head;
-
     // When a slab has free space it will be on the has space list for
     // that size class.  We use an empty block in this slab to be the
     // doubly linked node into that size class's free list.
@@ -56,36 +62,38 @@ namespace snmalloc
     // Initially zero to encode the superslabs relative list of slabs.
     uint8_t next = 0;
 
-    void add_use()
+    /**
+     * Updates statistics for adding an entry to the free list, if the
+     * slab is either
+     *  - empty adding the entry to the free list, or
+     *  - was full before the subtraction
+     * this returns true, otherwise returns false.
+     **/
+    bool return_object()
     {
-      used++;
+      return (--needed) == 0;
-    }
-
-    void sub_use()
-    {
-      used--;
-    }
-
-    void set_unused()
-    {
-      used = 0;
     }
 
     bool is_unused()
     {
-      return used == 0;
+      return needed == 0;
     }
 
     bool is_full()
     {
-      return link == 1;
+      auto result = link == 1;
+      assert(!result || head == nullptr);
+      return result;
     }
 
     void set_full()
     {
-      assert(head == 1);
+      assert(head == nullptr);
       assert(link != 1);
       link = 1;
+      // Set needed to 1, so that "return_object" will return true after calling
+      // set_full
+      needed = 1;
     }
 
     SlabLink* get_link(Slab* slab)
@@ -122,16 +130,24 @@ namespace snmalloc
       return *static_cast<void**>(node);
     }
 
-    bool valid_head(bool is_short)
+    bool valid_head()
     {
       size_t size = sizeclass_to_size(sizeclass);
-      size_t slab_start = get_initial_offset(sizeclass, is_short);
+      size_t slab_end = (address_cast(head) | ~SLAB_MASK) + 1;
-      size_t all_high_bits = ~static_cast<size_t>(1);
+      uintptr_t allocation_start =
+        remove_cache_friendly_offset(address_cast(head), sizeclass);
 
-      size_t head_start =
+      return (slab_end - allocation_start) % size == 0;
-        remove_cache_friendly_offset(head & all_high_bits, sizeclass);
+    }
 
-      return ((head_start - slab_start) % size) == 0;
+    static Slab* get_slab(void* p)
+    {
+      return pointer_cast<Slab>(address_cast(p) & SLAB_MASK);
+    }
+
+    static bool is_short(Slab* p)
+    {
+      return (address_cast(p) & SUPERSLAB_MASK) == address_cast(p);
     }
 
     /**
@@ -146,11 +162,15 @@ namespace snmalloc
     {
 #ifndef NDEBUG
       size_t length = 0;
-      void* curr = (head == 1) ? nullptr : pointer_offset(slab, head);
+      void* curr = head;
-      void* curr_slow = (head == 1) ? nullptr : pointer_offset(slab, head);
+      void* curr_slow = head;
       bool both = false;
       while (curr != nullptr)
       {
+        if (get_slab(curr) != slab)
+        {
+          error("Free list corruption, not correct slab.");
+        }
         curr = follow_next(curr);
         if (both)
         {
@@ -172,26 +192,25 @@ namespace snmalloc
 #endif
     }
 
-    void debug_slab_invariant(bool is_short, Slab* slab)
+    void debug_slab_invariant(Slab* slab)
     {
 #if !defined(NDEBUG) && !defined(SNMALLOC_CHEAP_CHECKS)
-      size_t size = sizeclass_to_size(sizeclass);
+      bool is_short = Metaslab::is_short(slab);
-      size_t offset = get_initial_offset(sizeclass, is_short);
-
-      if (is_unused())
-        return;
-
-      size_t accounted_for = used * size + offset;
 
       if (is_full())
       {
-        // All the blocks must be used.
-        assert(SLAB_SIZE == accounted_for);
         // There is no free list to validate
         // 'link' value is not important if full.
         return;
       }
 
+      if (is_unused())
+        return;
+
+      size_t size = sizeclass_to_size(sizeclass);
+      size_t offset = get_initial_offset(sizeclass, is_short);
+      size_t accounted_for = needed * size + offset;
+
       // Block is not full
       assert(SLAB_SIZE > accounted_for);
 
@@ -200,7 +219,7 @@ namespace snmalloc
       UNUSED(length);
 
       // Walk bump-free-list-segment accounting for unused space
-      void* curr = (head == 1) ? nullptr : pointer_offset(slab, head);
+      void* curr = head;
       while (curr != nullptr)
       {
         // Check we are looking at a correctly aligned block
@@ -240,7 +259,6 @@ namespace snmalloc
       assert(SLAB_SIZE == accounted_for);
 #else
       UNUSED(slab);
-      UNUSED(is_short);
 #endif
     }
   };

src/mem/sizeclass.h

@@ -8,6 +8,7 @@ namespace snmalloc
   // We use size_t as it generates better code.
   using sizeclass_t = size_t;
   //  using sizeclass_t = uint8_t;
+  using sizeclass_compress_t = uint8_t;
 
   constexpr static uint16_t get_initial_offset(sizeclass_t sc, bool is_short);
   constexpr static size_t sizeclass_to_size(sizeclass_t sizeclass);
@@ -152,8 +153,8 @@ namespace snmalloc
     return p = (void*)((uintptr_t)p & mask);
   }
 
-  SNMALLOC_FAST_PATH static uint16_t
+  SNMALLOC_FAST_PATH static uintptr_t
-  remove_cache_friendly_offset(uint16_t relative, sizeclass_t sizeclass)
+  remove_cache_friendly_offset(uintptr_t relative, sizeclass_t sizeclass)
   {
     size_t mask = sizeclass_to_inverse_cache_friendly_mask(sizeclass);
     return relative & mask;
@@ -166,8 +167,8 @@ namespace snmalloc
     return p;
   }
 
-  SNMALLOC_FAST_PATH static uint16_t
+  SNMALLOC_FAST_PATH static uintptr_t
-  remove_cache_friendly_offset(uint16_t relative, sizeclass_t sizeclass)
+  remove_cache_friendly_offset(uintptr_t relative, sizeclass_t sizeclass)
   {
     UNUSED(sizeclass);
     return relative;

src/mem/slab.h

@@ -20,11 +20,6 @@ namespace snmalloc
     }
 
   public:
-    static Slab* get(void* p)
-    {
-      return pointer_cast<Slab>(address_cast(p) & SLAB_MASK);
-    }
-
     Metaslab& get_meta()
     {
       Superslab* super = Superslab::get(this);
@@ -45,24 +40,25 @@ namespace snmalloc
     {
       // Read the head from the metadata stored in the superslab.
       Metaslab& meta = get_meta();
-      uint16_t head = meta.head;
+      void* head = meta.head;
 
       assert(rsize == sizeclass_to_size(meta.sizeclass));
-      meta.debug_slab_invariant(is_short(), this);
+      meta.debug_slab_invariant(this);
       assert(sl.get_head() == (SlabLink*)((size_t)this + meta.link));
       assert(!meta.is_full());
 
       void* p = nullptr;
       bool p_has_value = false;
 
-      if (head == 1)
+      if (head == nullptr)
       {
         size_t bumpptr = get_initial_offset(meta.sizeclass, is_short());
         bumpptr += meta.allocated * rsize;
         if (bumpptr == SLAB_SIZE)
         {
-          meta.add_use();
+          // Everything is in use, so we need all entries to be
-          assert(meta.used == meta.allocated);
+          // return before we can reclaim this slab.
+          meta.needed = meta.allocated;
 
           void* link = pointer_offset(this, meta.link);
           p = remove_cache_friendly_offset(link, meta.sizeclass);
@@ -96,7 +92,7 @@ namespace snmalloc
 
             if (curr == nullptr)
             {
-              meta.head = static_cast<uint16_t>(bumpptr);
+              meta.head = pointer_offset(this, bumpptr);
             }
             else
             {
@@ -115,23 +111,23 @@ namespace snmalloc
 
       if (!p_has_value)
       {
-        p = pointer_offset(this, meta.head);
+        p = meta.head;
 
         // Read the next slot from the memory that's about to be allocated.
         void* next = Metaslab::follow_next(p);
         // Put everything in allocators small_class free list.
-        meta.head = 1;
+        meta.head = nullptr;
         fast_free_list.value = next;
         // Treat stealing the free list as allocating it all.
         // Link is not in use, i.e. - 1 is required.
-        meta.used = meta.allocated - 1;
+        meta.needed = meta.allocated - 1;
 
         p = remove_cache_friendly_offset(p, meta.sizeclass);
       }
 
       assert(is_start_of_object(Superslab::get(p), p));
 
-      meta.debug_slab_invariant(is_short(), this);
+      meta.debug_slab_invariant(this);
 
       if constexpr (zero_mem == YesZero)
       {
@@ -162,29 +158,21 @@ namespace snmalloc
       if (meta.is_unused())
         error("Detected potential double free.");
 #endif
-      meta.debug_slab_invariant(is_short(), this);
+      meta.debug_slab_invariant(this);
-      meta.sub_use();
 
-      bool was_full = meta.is_full();
+      if (unlikely(meta.return_object()))
-      if (unlikely(was_full))
-        return false;
-
-      bool is_unused = meta.is_unused();
-      if (unlikely(is_unused))
         return false;
 
       // Update the head and the next pointer in the free list.
-      uint16_t head = meta.head;
+      void* head = meta.head;
-      uint16_t current = pointer_to_index(p);
 
       // Set the head to the memory being deallocated.
-      meta.head = current;
+      meta.head = p;
-      assert(meta.valid_head(is_short()));
+      assert(meta.valid_head());
 
       // Set the next pointer to the previous head.
-      Metaslab::store_next(
+      Metaslab::store_next(p, head);
-        p, (head == 1) ? nullptr : pointer_offset(this, head));
+      meta.debug_slab_invariant(this);
-      meta.debug_slab_invariant(is_short(), this);
       return true;
     }
 
@@ -198,13 +186,10 @@ namespace snmalloc
     {
       Metaslab& meta = super->get_meta(this);
 
-      bool was_full = meta.is_full();
+      if (meta.is_full())
-      bool is_unused = meta.is_unused();
-
-      if (was_full)
       {
         // We are not on the sizeclass list.
-        if (is_unused)
+        if (meta.allocated == 1)
         {
           // Dealloc on the superslab.
           if (is_short())
@@ -214,33 +199,28 @@ namespace snmalloc
         }
         // Update the head and the sizeclass link.
         uint16_t index = pointer_to_index(p);
-        assert(meta.head == 1);
+        assert(meta.head == nullptr);
         //        assert(meta.fully_allocated(is_short()));
         meta.link = index;
+        meta.needed = meta.allocated - 1;
 
         // Push on the list of slabs for this sizeclass.
         sl->insert_back(meta.get_link(this));
-        meta.debug_slab_invariant(is_short(), this);
+        meta.debug_slab_invariant(this);
         return Superslab::NoSlabReturn;
       }
 
-      if (is_unused)
+      // Remove from the sizeclass list and dealloc on the superslab.
-      {
+      sl->remove(meta.get_link(this));
-        // Remove from the sizeclass list and dealloc on the superslab.
-        sl->remove(meta.get_link(this));
 
-        if (is_short())
+      if (is_short())
-          return super->dealloc_short_slab(memory_provider);
+        return super->dealloc_short_slab(memory_provider);
 
-        return super->dealloc_slab(this, memory_provider);
+      return super->dealloc_slab(this, memory_provider);
-      }
-
-      abort();
     }
-
     bool is_short()
     {
-      return (address_cast(this) & SUPERSLAB_MASK) == address_cast(this);
+      return Metaslab::is_short(this);
     }
   };
 } // namespace snmalloc

src/mem/superslab.h

@@ -161,7 +161,7 @@ namespace snmalloc
         return alloc_slab(sizeclass, memory_provider);
 
       meta[0].allocated = 1;
-      meta[0].head = 1;
+      meta[0].head = nullptr;
       meta[0].sizeclass = static_cast<uint8_t>(sizeclass);
       meta[0].link = get_initial_offset(sizeclass, true);
 
@@ -183,7 +183,7 @@ namespace snmalloc
 
       uint8_t n = meta[h].next;
 
-      meta[h].head = 1;
+      meta[h].head = nullptr;
       meta[h].allocated = 1;
       meta[h].sizeclass = static_cast<uint8_t>(sizeclass);
       meta[h].link = get_initial_offset(sizeclass, false);