PartitionAlloc: improve clarity of page state checks.

Source/wtf/PartitionAlloc.cpp

Index: Source/wtf/PartitionAlloc.cpp
diff --git a/Source/wtf/PartitionAlloc.cpp b/Source/wtf/PartitionAlloc.cpp
index ee0a1c0b341751d1220aa6ce402d3e9f403fba06..7017ac0e7bd237c6d7f71877ab3bbe1f51ed2210 100644
--- a/Source/wtf/PartitionAlloc.cpp
+++ b/Source/wtf/PartitionAlloc.cpp
@@ -321,20 +321,40 @@ static NEVER_INLINE void partitionBucketFull()
     IMMEDIATE_CRASH();
 }
 
-static bool partitionPageStateIsEmpty(const PartitionPage* page)
+static bool ALWAYS_INLINE partitionPageStateIsActive(const PartitionPage* page)
 {
     ASSERT(page != &PartitionRootGeneric::gSeedPage);
-    return !page->numAllocatedSlots;
+    ASSERT(!page->pageOffset);
+    return (page->numAllocatedSlots && (page->freelistHead || page->numUnprovisionedSlots));

[haraken, 2015/06/24 00:51:52]

page->numAllocatedSlots => page->numAllocatedSlots > 0

Just to confirm: If page->numAllocatedSlots > 0 but 'page->freelistHead ||
page->numUnprovisionedSlots' is false, it is a full page, right?

 }
 
-static bool partitionPageStateIsDecommitted(const PartitionPage* page)
+static bool ALWAYS_INLINE partitionPageStateIsFull(const PartitionPage* page)

[haraken, 2015/06/24 00:51:51]

This method is a bit confusing, since it returns false for a full page that has
already been removed from the active page list.

I'm OK in this CL, but it would be great if we can clean up the page state
transitions a bit more in the future. For example, it looks hacky to represent a
full page by storing a negative value in numAllocatedSlots.

struct PartitionPage {
  ...;
  uint16_t pageOffset;  // We won't need 16 bit for this.
  int16_t emptyCacheIndex;  // Ditto.
};

By saving the bits, we'll be able to have space to explicitly represent the
PageState.

 {
     ASSERT(page != &PartitionRootGeneric::gSeedPage);
-    bool ret = !page->numAllocatedSlots && !page->freelistHead;
+    ASSERT(!page->pageOffset);
+    bool ret = (page->numAllocatedSlots == partitionBucketSlots(page->bucket));
+    if (ret) {
+        ASSERT(!page->freelistHead);
+        ASSERT(!page->numUnprovisionedSlots);
+    }
+    return ret;
+}
+
+static bool ALWAYS_INLINE partitionPageStateIsEmpty(const PartitionPage* page)
+{
+    ASSERT(page != &PartitionRootGeneric::gSeedPage);
+    ASSERT(!page->pageOffset);
+    return (!page->numAllocatedSlots && page->freelistHead);
+}
+
+static bool ALWAYS_INLINE partitionPageStateIsDecommitted(const PartitionPage* page)
+{
+    ASSERT(page != &PartitionRootGeneric::gSeedPage);
+    ASSERT(!page->pageOffset);
+    bool ret = (!page->numAllocatedSlots && !page->freelistHead);
     if (ret) {
         ASSERT(!page->numUnprovisionedSlots);
         ASSERT(page->emptyCacheIndex == -1);
-        ASSERT(!page->pageOffset);
     }
     return ret;
 }
@@ -452,12 +472,11 @@ static ALWAYS_INLINE uint16_t partitionBucketPartitionPages(const PartitionBucke
     return (bucket->numSystemPagesPerSlotSpan + (kNumSystemPagesPerPartitionPage - 1)) / kNumSystemPagesPerPartitionPage;
 }
 
-static ALWAYS_INLINE void partitionPageReset(PartitionPage* page, PartitionBucket* bucket)
+static ALWAYS_INLINE void partitionPageReset(PartitionPage* page)
 {
-    ASSERT(page->bucket == bucket);
     ASSERT(partitionPageStateIsDecommitted(page));
 
-    page->numUnprovisionedSlots = partitionBucketSlots(bucket);
+    page->numUnprovisionedSlots = partitionBucketSlots(page->bucket);
     ASSERT(page->numUnprovisionedSlots);
 
     page->nextPage = nullptr;
@@ -469,7 +488,7 @@ static ALWAYS_INLINE void partitionPageSetup(PartitionPage* page, PartitionBucke
     page->bucket = bucket;
     page->emptyCacheIndex = -1;
 
-    partitionPageReset(page, bucket);
+    partitionPageReset(page);
 
     // If this page has just a single slot, do not set up page offsets for any
     // page metadata other than the first one. This ensures that attempts to
@@ -581,26 +600,23 @@ static bool partitionSetNewActivePage(PartitionBucket* bucket)
         ASSERT(page != bucket->decommittedPagesHead);
 
         // Deal with empty and decommitted pages.
-        if (UNLIKELY(!page->numAllocatedSlots)) {
-            ASSERT(partitionPageStateIsEmpty(page));
-            if (UNLIKELY(!page->freelistHead)) {
-                ASSERT(partitionPageStateIsDecommitted(page));
-                page->nextPage = bucket->decommittedPagesHead;
-                bucket->decommittedPagesHead = page;
-            } else {
-                page->nextPage = bucket->emptyPagesHead;
-                bucket->emptyPagesHead = page;
-            }
-        } else if (LIKELY(page->freelistHead != 0) || LIKELY(page->numUnprovisionedSlots)) {
+        if (LIKELY(partitionPageStateIsActive(page))) {
             // This page is usable because it has freelist entries, or has
             // unprovisioned slots we can create freelist entries from.
             bucket->activePagesHead = page;
             return true;
+        }
+        if (LIKELY(partitionPageStateIsEmpty(page))) {
+            page->nextPage = bucket->emptyPagesHead;
+            bucket->emptyPagesHead = page;
+        } else if (LIKELY(partitionPageStateIsDecommitted(page))) {
+            page->nextPage = bucket->decommittedPagesHead;
+            bucket->decommittedPagesHead = page;
         } else {
+            ASSERT(partitionPageStateIsFull(page));
             // If we get here, we found a full page. Skip over it too, and also
             // tag it as full (via a negative value). We need it tagged so that
             // free'ing can tell, and move it back into the active page list.
-            ASSERT(page->numAllocatedSlots == partitionBucketSlots(bucket));
             page->numAllocatedSlots = -page->numAllocatedSlots;
             ++bucket->numFullPages;
             // numFullPages is a uint16_t for efficient packing so guard against
@@ -767,13 +783,14 @@ void* partitionAllocSlowPath(PartitionRootBase* root, int flags, size_t size, Pa
     } else if (LIKELY(partitionSetNewActivePage(bucket))) {
         // First, did we find an active page in the active pages list?
         newPage = bucket->activePagesHead;
-        ASSERT(newPage != &PartitionRootGeneric::gSeedPage);
+        ASSERT(partitionPageStateIsActive(newPage));
     } else if (LIKELY(bucket->emptyPagesHead != nullptr) || LIKELY(bucket->decommittedPagesHead != nullptr)) {
         // Second, look in our lists of empty and decommitted pages.
         // Check empty pages first, which are preferred, but beware that an
         // empty page might have been decommitted.
         while (LIKELY((newPage = bucket->emptyPagesHead) != nullptr)) {
-            ASSERT(partitionPageStateIsEmpty(newPage));
+            ASSERT(newPage->bucket == bucket);
+            ASSERT(partitionPageStateIsEmpty(newPage) || partitionPageStateIsDecommitted(newPage));
             bucket->emptyPagesHead = newPage->nextPage;
             // Accept the empty page unless it got decommitted.
             if (newPage->freelistHead) {
@@ -786,11 +803,12 @@ void* partitionAllocSlowPath(PartitionRootBase* root, int flags, size_t size, Pa
         }
         if (UNLIKELY(!newPage) && LIKELY(bucket->decommittedPagesHead != nullptr)) {
             newPage = bucket->decommittedPagesHead;
-            bucket->decommittedPagesHead = newPage->nextPage;
+            ASSERT(newPage->bucket == bucket);
             ASSERT(partitionPageStateIsDecommitted(newPage));
+            bucket->decommittedPagesHead = newPage->nextPage;
             void* addr = partitionPageToPointer(newPage);
             partitionRecommitSystemPages(root, addr, partitionBucketBytes(newPage->bucket));
-            partitionPageReset(newPage, bucket);
+            partitionPageReset(newPage);
         }
         ASSERT(newPage);
     } else {
@@ -833,7 +851,6 @@ void* partitionAllocSlowPath(PartitionRootBase* root, int flags, size_t size, Pa
 static ALWAYS_INLINE void partitionDecommitPage(PartitionRootBase* root, PartitionPage* page)
 {
     ASSERT(partitionPageStateIsEmpty(page));
-    ASSERT(page->freelistHead);
     ASSERT(!partitionBucketIsDirectMapped(page->bucket));
     void* addr = partitionPageToPointer(page);
     partitionDecommitSystemPages(root, addr, partitionBucketBytes(page->bucket));
@@ -853,11 +870,9 @@ static void partitionDecommitPageIfPossible(PartitionRootBase* root, PartitionPa
     ASSERT(page->emptyCacheIndex >= 0);
     ASSERT(static_cast<unsigned>(page->emptyCacheIndex) < kMaxFreeableSpans);
     ASSERT(page == root->globalEmptyPageRing[page->emptyCacheIndex]);
-    if (!page->numAllocatedSlots && page->freelistHead) {
-        // The page is still empty, and not decommitted, so _really_ decommit
-        // it.
+    if (partitionPageStateIsEmpty(page))
         partitionDecommitPage(root, page);
-    }
+
     page->emptyCacheIndex = -1;
 }
 
@@ -1190,10 +1205,10 @@ static void partitionDumpPageStats(PartitionBucketMemoryStats* statsOut, const P
     if (partitionPageStateIsEmpty(page)) {
         statsOut->decommittableBytes += pageBytesResident;
         ++statsOut->numEmptyPages;
-    } else if (page->numAllocatedSlots == bucketNumSlots) {
+    } else if (partitionPageStateIsFull(page)) {
         ++statsOut->numFullPages;
     } else {
-        ASSERT(page->numAllocatedSlots > 0);
+        ASSERT(partitionPageStateIsActive(page));
         ++statsOut->numActivePages;
     }
 }
@@ -1220,7 +1235,7 @@ static void partitionDumpBucketStats(PartitionBucketMemoryStats* statsOut, const
     statsOut->residentBytes = bucket->numFullPages * statsOut->allocatedPageSize;
 
     for (const PartitionPage* page = bucket->emptyPagesHead; page; page = page->nextPage) {
-        ASSERT(partitionPageStateIsEmpty(page));
+        ASSERT(partitionPageStateIsEmpty(page) || partitionPageStateIsDecommitted(page));
         partitionDumpPageStats(statsOut, page);
     }
     for (const PartitionPage* page = bucket->decommittedPagesHead; page; page = page->nextPage) {