update SkSmallAllocator to force internal allocations to be 16-byte aligned

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1755293002

Review URL: https://codereview.chromium.org/1755293002

diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h
index 5831ba3..b9e5191 100644 (file)
--- a/include/core/SkTypes.h
+++ b/include/core/SkTypes.h
@@ -337,6 +337,9 @@ template <typename T, size_t N> char (&SkArrayCountHelper(T (&array)[N]))[N];
 #define SkAlign8(x)     (((x) + 7) >> 3 << 3)
 #define SkIsAlign8(x)   (0 == ((x) & 7))
 
+#define SkAlign16(x)     (((x) + 15) >> 4 << 4)
+#define SkIsAlign16(x)   (0 == ((x) & 15))
+
 #define SkAlignPtr(x)   (sizeof(void*) == 8 ?   SkAlign8(x) :   SkAlign4(x))
 #define SkIsAlignPtr(x) (sizeof(void*) == 8 ? SkIsAlign8(x) : SkIsAlign4(x))
 

diff --git a/src/core/SkSmallAllocator.h b/src/core/SkSmallAllocator.h
index 79b1d29..63c02ec 100644 (file)
--- a/src/core/SkSmallAllocator.h
+++ b/src/core/SkSmallAllocator.h
@@ -18,11 +18,15 @@
  *  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<uint32_t kMaxObjects, size_t kTotalBytes>
 class SkSmallAllocator : SkNoncopyable {
@@ -67,14 +71,14 @@ public:
      *  allocation if necessary.
      *  Unlike createT(), this method will not call the constructor of T.
      */
-    template<typename T> void* reserveT(size_t storageRequired = sizeof(T)) {
+    template<typename T> void* reserveT(size_t storageRequested = sizeof(T)) {
         SkASSERT(fNumObjects < kMaxObjects);
-        SkASSERT(storageRequired >= sizeof(T));
+        SkASSERT(storageRequested >= sizeof(T));
         if (kMaxObjects == fNumObjects) {
             return nullptr;
         }
-        const size_t storageRemaining = SkAlign4(kTotalBytes) - fStorageUsed;
-        storageRequired = SkAlign4(storageRequired);
+        const size_t storageRemaining = sizeof(fStorage) - fStorageUsed;
+        const size_t storageRequired = SkAlign16(storageRequested);
         Rec* rec = &fRecs[fNumObjects];
         if (storageRequired > storageRemaining) {
             // Allocate on the heap. Ideally we want to avoid this situation,
@@ -88,8 +92,8 @@ public:
             // There is space in fStorage.
             rec->fStorageSize = storageRequired;
             rec->fHeapStorage = nullptr;
-            SkASSERT(SkIsAlign4(fStorageUsed));
-            rec->fObj = static_cast<void*>(fStorage + (fStorageUsed / 4));
+            SkASSERT(SkIsAlign16(fStorageUsed));
+            rec->fObj = static_cast<void*>(fStorage.fBytes + fStorageUsed);
             fStorageUsed += storageRequired;
         }
         rec->fKillProc = DestroyT<T>;
@@ -125,12 +129,17 @@ private:
         static_cast<T*>(ptr)->~T();
     }
 
+    struct SK_STRUCT_ALIGN(16) Storage {
+        // we add kMaxObjects * 15 to account for the worst-case slop, where each allocation wasted
+        // 15 bytes (due to forcing each to be 16-byte aligned)
+        char    fBytes[kTotalBytes + kMaxObjects * 15];
+    };
+
+    Storage     fStorage;
     // Number of bytes used so far.
-    size_t              fStorageUsed;
-    // Pad the storage size to be 4-byte aligned.
-    uint32_t            fStorage[SkAlign4(kTotalBytes) >> 2];
-    uint32_t            fNumObjects;
-    Rec                 fRecs[kMaxObjects];
+    size_t      fStorageUsed;
+    uint32_t    fNumObjects;
+    Rec         fRecs[kMaxObjects];
 };
 
 #endif // SkSmallAllocator_DEFINED

diff --git a/tests/SmallAllocatorTest.cpp b/tests/SmallAllocatorTest.cpp
index 774e0c9..7c9172d 100644 (file)
--- a/tests/SmallAllocatorTest.cpp
+++ b/tests/SmallAllocatorTest.cpp
@@ -81,3 +81,16 @@ DEF_TEST(SmallAllocator_pointer, reporter) {
     REPORTER_ASSERT(reporter, container != nullptr);
     REPORTER_ASSERT(reporter, container->getDummy() == &d);
 }
+
+#define check_alignment(reporter, ptr)  \
+    REPORTER_ASSERT(reporter, SkIsAlign16((intptr_t)ptr))
+
+DEF_TEST(SmallAllocator_alignment, reporter) {
+    const size_t totalBytes = 1 + 2 + 4 + 8;
+    SkSmallAllocator<4, totalBytes> alloc;
+
+    check_alignment(reporter, alloc.reserveT<uint8_t>());
+    check_alignment(reporter, alloc.reserveT<uint16_t>());
+    check_alignment(reporter, alloc.reserveT<uint32_t>());
+    check_alignment(reporter, alloc.reserveT<uint64_t>());
+}