Make SkSmallAllocator obey the RAII invariants and be expandable

src/core/SkSmallAllocator.h

Index: src/core/SkSmallAllocator.h
diff --git a/src/core/SkSmallAllocator.h b/src/core/SkSmallAllocator.h
index 13b1505821ab588f99a8a28fe21fc0ddd9a16cb9..7562cabbccb6ff17a7f40593d73ffd2405da15d1 100644
--- a/src/core/SkSmallAllocator.h
+++ b/src/core/SkSmallAllocator.h
@@ -8,120 +8,78 @@
 #ifndef SkSmallAllocator_DEFINED
 #define SkSmallAllocator_DEFINED
 
-#include "SkTDArray.h"
+#include "SkTArray.h"
 #include "SkTypes.h"
 
-#include <new>
 #include <utility>
 
 /*
  *  Template class for allocating small objects without additional heap memory
- *  allocations. kMaxObjects is a hard limit on the number of objects that can
- *  be allocated using this class. After that, attempts to create more objects
- *  with this class will assert and return nullptr.
+ *  allocations.
  *
  *  kTotalBytes is the total number of bytes provided for storage for all
  *  objects created by this allocator. If an object to be created is larger
  *  than the storage (minus storage already used), it will be allocated on the
  *  heap. This class's destructor will handle calling the destructor for each
  *  object it allocated and freeing its memory.
- *
- *  Current the class always aligns each allocation to 16-bytes to be safe, but future
- *  may reduce this to only the alignment that is required per alloc.
  */
-template<uint32_t kMaxObjects, size_t kTotalBytes>
+template<uint32_t kExpectedObjects, size_t kTotalBytes>
 class SkSmallAllocator : SkNoncopyable {
 public:
-    SkSmallAllocator()
-    : fStorageUsed(0)
-    , fNumObjects(0)
-    {}
-
     ~SkSmallAllocator() {
         // Destruct in reverse order, in case an earlier object points to a
         // later object.
-        while (fNumObjects > 0) {
-            fNumObjects--;
-            Rec* rec = &fRecs[fNumObjects];
-            rec->fKillProc(rec->fObj);
-            // Safe to do if fObj is in fStorage, since fHeapStorage will
-            // point to nullptr.
-            sk_free(rec->fHeapStorage);
+        while (fRecs.count() > 0) {
+            this->deleteLast();
         }
     }
 
     /*
      *  Create a new object of type T. Its lifetime will be handled by this
      *  SkSmallAllocator.
-     *  Note: If kMaxObjects have been created by this SkSmallAllocator, nullptr
-     *  will be returned.
      */
     template<typename T, typename... Args>
     T* createT(Args&&... args) {
-        void* buf = this->reserveT<T>();
-        if (nullptr == buf) {
-            return nullptr;
-        }
+        void* buf = this->reserve(sizeof(T), DestroyT<T>);
         return new (buf) T(std::forward<Args>(args)...);
     }
 
     /*
-     *  Reserve a specified amount of space (must be enough space for one T).
-     *  The space will be in fStorage if there is room, or on the heap otherwise.
-     *  Either way, this class will call ~T() in its destructor and free the heap
-     *  allocation if necessary.
-     *  Unlike createT(), this method will not call the constructor of T.
+     * Create a new object of size using initer to initialize the memory. The initer function has
+     * the signature T* initer(void* storage). If initer is unable to initialize the memory it
+     * should return nullptr where SkSmallAllocator will free the memory.
      */
-    template<typename T> void* reserveT(size_t storageRequired = sizeof(T)) {
-        SkASSERT(fNumObjects < kMaxObjects);
-        SkASSERT(storageRequired >= sizeof(T));
-        if (kMaxObjects == fNumObjects) {
-            return nullptr;
-        }
-        const size_t storageRemaining = sizeof(fStorage) - fStorageUsed;
-        Rec* rec = &fRecs[fNumObjects];
-        if (storageRequired > storageRemaining) {
-            // Allocate on the heap. Ideally we want to avoid this situation.
-
-            // With the gm composeshader_bitmap2, storage required is 4476
-            // and storage remaining is 3392. Increasing the base storage
-            // causes google 3 tests to fail.
+    template <typename T, typename Initer>
+    T* createWithIniterT(size_t size, Initer initer) {
+        SkASSERT(size >= sizeof(T));
 
-            rec->fStorageSize = 0;
-            rec->fHeapStorage = sk_malloc_throw(storageRequired);
-            rec->fObj = static_cast<void*>(rec->fHeapStorage);
-        } else {
-            // There is space in fStorage.
-            rec->fStorageSize = storageRequired;
-            rec->fHeapStorage = nullptr;
-            rec->fObj = static_cast<void*>(fStorage + fStorageUsed);
-            fStorageUsed += storageRequired;
+        void* storage = this->reserve(size, DestroyT<T>);
+        T* candidate = initer(storage);
+        if (!candidate) {
+            // Initializing didn't workout so free the memory.
+            this->freeLast();
         }
-        rec->fKillProc = DestroyT<T>;
-        fNumObjects++;
-        return rec->fObj;
+
+        return candidate;
     }
 
     /*
-     *  Free the memory reserved last without calling the destructor.
-     *  Can be used in a nested way, i.e. after reserving A and B, calling
-     *  freeLast once will free B and calling it again will free A.
+     * Free the last object allocated and call its destructor. This can be called multiple times
+     * removing objects from the pool in reverse order.
      */
-    void freeLast() {
-        SkASSERT(fNumObjects > 0);
-        Rec* rec = &fRecs[fNumObjects - 1];
-        sk_free(rec->fHeapStorage);
-        fStorageUsed -= rec->fStorageSize;
-
-        fNumObjects--;
+    void deleteLast() {
+        SkASSERT(fRecs.count() > 0);
+        Rec& rec = fRecs.back();
+        rec.fDestructor(rec.fObj);
+        this->freeLast();
     }
 
 private:
+    using Destructor = void(*)(void*);
     struct Rec {

[bungeman-skia, 2016/11/09 19:30:56]

If this Rec were to have a destructor which did the deleting / calling of
fDestructor (the Rec owned the allocation) then ~SkSmallAllocator becomes
trivial and freeLast becomes simple. On the other hand, this also means needing
to split up the allocation and the setting of the fDestructor (the same way
throwing from a class constructor doesn't call the destructor).

[herb_g, 2016/11/09 20:20:05]

I will work on this in a follow up CL.

-        size_t fStorageSize;  // 0 if allocated on heap
-        void*  fObj;
-        void*  fHeapStorage;
-        void   (*fKillProc)(void*);
+        size_t     fStorageSize;  // 0 if allocated on heap
+        char*      fObj;
+        Destructor fDestructor;
     };
 
     // Used to call the destructor for allocated objects.
@@ -130,10 +88,41 @@ private:
         static_cast<T*>(ptr)->~T();
     }
 
-    alignas(16) char fStorage[kTotalBytes];
-    size_t           fStorageUsed;  // Number of bytes used so far.
-    uint32_t         fNumObjects;
-    Rec              fRecs[kMaxObjects];
+    // Reserve storageRequired from fStorage if possible otherwise allocate on the heap.
+    void* reserve(size_t storageRequired, Destructor destructor) {
+        const size_t storageRemaining = sizeof(fStorage) - fStorageUsed;
+        Rec& rec = fRecs.push_back();
+        if (storageRequired > storageRemaining) {
+            // Allocate on the heap. Ideally we want to avoid this situation.
+
+            // With the gm composeshader_bitmap2, storage required is 4476
+            // and storage remaining is 3392. Increasing the base storage
+            // causes google 3 tests to fail.
+
+            rec.fStorageSize = 0;
+            rec.fObj = new char [storageRequired];
+        } else {
+            // There is space in fStorage.
+            rec.fStorageSize = storageRequired;
+            rec.fObj = &fStorage[fStorageUsed];
+            fStorageUsed += storageRequired;
+        }
+        rec.fDestructor = destructor;
+        return rec.fObj;
+    }
+
+    void freeLast() {
+        Rec& rec = fRecs.back();
+        if (0 == rec.fStorageSize) {
+            delete [] rec.fObj;
+        }
+        fStorageUsed -= rec.fStorageSize;
+        fRecs.pop_back();
+    }
+
+    size_t                                 fStorageUsed {0};  // Number of bytes used so far.
+    SkSTArray<kExpectedObjects, Rec, true> fRecs;
+    char                                   fStorage[kTotalBytes];

[bungeman-skia, 2016/11/09 19:30:56]

Did we lose alignment here? Do we care about alignment in general (reserve
doesn't seem to care).

[herb_g, 2016/11/09 20:20:05]

Yes, and this is the same behavior is the original code. I will fix this in a
follow up CL.

 };
 
 #endif // SkSmallAllocator_DEFINED