From f982cb37e3092e7a69c13a12ec09806ceff5af45 Mon Sep 17 00:00:00 2001 From: mtklein Date: Sun, 13 Nov 2016 09:55:05 -0800 Subject: Revert of Make SkSmallAllocator obey the RAII invariants and be expandable (patchset #15 id:280001 of https://codereview.chromium.org/2488523003/ ) Reason for revert: bots crashing / asserting Original issue's description: > Make SkSmallAllocator obey the RAII invariants and move to heap structures when needed. > > The biggest change is to the API which allowed code to bypass the > destruction invariants. This destruction bypass feature was needed in > only one use, and is totally encapsulated using createWithIniterT. > > BUG=skia: > GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2488523003 > > Committed: https://skia.googlesource.com/skia/+/d5dc657b8c3ac916f98005dafdedafe02f023449 > Committed: https://skia.googlesource.com/skia/+/c18b5f8f57a4efc5d5d1e399ed8bd3bd02c592ab TBR=bungeman@google.com,herb@google.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia: Review-Url: https://codereview.chromium.org/2494353002 --- src/core/SkSmallAllocator.h | 166 ++++++++++++++++++++------------------------ 1 file changed, 77 insertions(+), 89 deletions(-) (limited to 'src/core/SkSmallAllocator.h') diff --git a/src/core/SkSmallAllocator.h b/src/core/SkSmallAllocator.h index cc50323c3a..13b1505821 100644 --- a/src/core/SkSmallAllocator.h +++ b/src/core/SkSmallAllocator.h @@ -8,144 +8,132 @@ #ifndef SkSmallAllocator_DEFINED #define SkSmallAllocator_DEFINED -#include "SkTArray.h" +#include "SkTDArray.h" #include "SkTypes.h" -#include +#include #include - -// max_align_t is needed to calculate the alignment for createWithIniterT when the T used is an -// abstract type. The complication with max_align_t is that it is defined differently for -// different builds. -namespace { -#if defined(SK_BUILD_FOR_WIN32) || defined(SK_BUILD_FOR_MAC) - // Use std::max_align_t for compiles that follow the standard. - #include - using SystemAlignment = std::max_align_t; -#else - // Ubuntu compiles don't have std::max_align_t defined, but MSVC does not define max_align_t. - #include - using SystemAlignment = max_align_t; -#endif -} - /* * Template class for allocating small objects without additional heap memory - * allocations. + * 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. * * 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 +template 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 (fRecs.count() > 0) { - this->deleteLast(); + 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); } } /* * 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 T* createT(Args&&... args) { - void* buf = this->reserve(sizeof(T), DefaultDestructor); + void* buf = this->reserveT(); + if (nullptr == buf) { + return nullptr; + } return new (buf) T(std::forward(args)...); } /* - * 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. + * 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. */ - template - auto createWithIniter(size_t size, Initer initer) -> decltype(initer(nullptr)) { - using ReturnType = decltype(initer(nullptr)); - SkASSERT(size >= sizeof(ReturnType)); - - void* storage = this->reserve(size, DefaultDestructor); - auto candidate = initer(storage); - if (!candidate) { - // Initializing didn't workout so free the memory. - this->freeLast(); + template 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. - return candidate; + // 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->fHeapStorage = sk_malloc_throw(storageRequired); + rec->fObj = static_cast(rec->fHeapStorage); + } else { + // There is space in fStorage. + rec->fStorageSize = storageRequired; + rec->fHeapStorage = nullptr; + rec->fObj = static_cast(fStorage + fStorageUsed); + fStorageUsed += storageRequired; + } + rec->fKillProc = DestroyT; + fNumObjects++; + return rec->fObj; } /* - * Free the last object allocated and call its destructor. This can be called multiple times - * removing objects from the pool in reverse order. + * 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. */ - void deleteLast() { - SkASSERT(fRecs.count() > 0); - Rec& rec = fRecs.back(); - rec.fDestructor(rec.fObj); - this->freeLast(); + void freeLast() { + SkASSERT(fNumObjects > 0); + Rec* rec = &fRecs[fNumObjects - 1]; + sk_free(rec->fHeapStorage); + fStorageUsed -= rec->fStorageSize; + + fNumObjects--; } private: - using Destructor = void(*)(void*); struct Rec { - char* fObj; - Destructor fDestructor; + size_t fStorageSize; // 0 if allocated on heap + void* fObj; + void* fHeapStorage; + void (*fKillProc)(void*); }; // Used to call the destructor for allocated objects. template - static void DefaultDestructor(void* ptr) { + static void DestroyT(void* ptr) { static_cast(ptr)->~T(); } - static constexpr size_t kAlignment = alignof(SystemAlignment); - - static constexpr size_t AlignSize(size_t size) { - return (size + kAlignment - 1) & ~(kAlignment - 1); - } - - // Reserve storageRequired from fStorage if possible otherwise allocate on the heap. - void* reserve(size_t storageRequired, Destructor destructor) { - // Make sure that all allocations stay aligned by rounding the storageRequired up to the - // aligned value. - char* objectStart = fStorageEnd; - char* objectEnd = objectStart + AlignSize(storageRequired); - Rec& rec = fRecs.push_back(); - if (objectEnd > &fStorage[kTotalBytes]) { - // Allocate on the heap. Ideally we want to avoid this situation. - rec.fObj = new char [storageRequired]; - } else { - // There is space in fStorage. - rec.fObj = objectStart; - fStorageEnd = objectEnd; - } - rec.fDestructor = destructor; - return rec.fObj; - } - - void freeLast() { - Rec& rec = fRecs.back(); - if (std::less()(rec.fObj, fStorage) - || !std::less()(rec.fObj, &fStorage[kTotalBytes])) { - delete [] rec.fObj; - } else { - fStorageEnd = rec.fObj; - } - fRecs.pop_back(); - } - - SkSTArray fRecs; - char* fStorageEnd {fStorage}; - // Since char have an alignment of 1, it should be forced onto an alignment the compiler - // expects which is the alignment of std::max_align_t. - alignas (kAlignment) char fStorage[kTotalBytes]; + alignas(16) char fStorage[kTotalBytes]; + size_t fStorageUsed; // Number of bytes used so far. + uint32_t fNumObjects; + Rec fRecs[kMaxObjects]; }; #endif // SkSmallAllocator_DEFINED -- cgit v1.2.3