PartitionAlloc: Distinguish OOMs where a lot of super pages are not committed

Source/wtf/PartitionAlloc.cpp

Index: Source/wtf/PartitionAlloc.cpp
diff --git a/Source/wtf/PartitionAlloc.cpp b/Source/wtf/PartitionAlloc.cpp
index ab14a4328f122cd7b2e7b56e31c6fc83aadc98c6..87d3a43265cfc639f715775299ebef63b19848ad 100644
--- a/Source/wtf/PartitionAlloc.cpp
+++ b/Source/wtf/PartitionAlloc.cpp
@@ -114,6 +114,7 @@ static void parititonAllocBaseInit(PartitionRootBase* root)
     root->initialized = true;
     root->totalSizeOfCommittedPages = 0;
     root->totalSizeOfSuperPages = 0;
+    root->totalSizeOfDirectMappedPages = 0;
     root->nextSuperPage = 0;
     root->nextPartitionPage = 0;
     root->nextPartitionPageEnd = 0;
@@ -296,15 +297,30 @@ bool partitionAllocGenericShutdown(PartitionRootGeneric* root)
     return noLeaks;
 }
 
-static NEVER_INLINE void partitionOutOfMemory()
+

[Chris Evans, 2014/11/08 07:04:43]

Nit: Remove extra newline?

[hiroshige, 2014/11/11 09:41:30]

Done.

+static NEVER_INLINE void partitionOutOfMemoryWithLotsOfUncommitedPages()
+{
+    // Crash at a special address (0x9b)
+    // to be easily distinguished on crash reports.
+    IMMEDIATE_CRASH_WITH_FLAG(0x9b);

[Chris Evans, 2014/11/08 07:04:43]

Do we think this strange quirk of Windows (missing frames in stack trace) is
worth adding a brand new macro for?

I'd be happy with
#if OS(WIN)
.. do the magic bad dereference ..
#endif
IMMEDIATE_CRASH()

Also: does the "missing frames" problem affect 64-bit Windows? If not, we can
narrow the special hack to just Windows and just 32-bit which would be nice.

[hiroshige, 2014/11/11 09:41:30]

Done.
I withdraw the macro because I feel its current implementation (crashing at
magic number address) is somehow ugly and not ready for use in general.
I think so, because there are no crashes which includes partitionFull or
partitionOutOfMemory in stack traces on Win64.
(and there are such crashes on amd64 Linux and Mac)

+}
+
+static NEVER_INLINE void partitionOutOfMemory(const PartitionRootBase* root)
 {
+    ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);

[Chris Evans, 2014/11/08 07:04:44]

Nit: for clarity, maybe we only need to ASSERT() directly after modifying one of
these values.

[hiroshige, 2014/11/11 09:41:30]

Done.

+    if (root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages - root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) {

[Chris Evans, 2014/11/08 07:04:44]

I still don't think this test has much value in 64-bit. Even if we do have 1GB
of uncommitted pages on 64-bit, I think this has zero correlation with running
out of virtual address space. A 64-bit address space is enormous, even on ARM64.

[hiroshige, 2014/11/11 09:41:30]

OK, I added #if CPU(32_BIT) to limit this to 32-bit only.

+        // OOMs where a lot of super pages are allocated but not committed,
+        // probably due to http://crbug.com/421387.
+        partitionOutOfMemoryWithLotsOfUncommitedPages();
+    }
+    // Ordinary OOMs (where super pages are consumed and mostly committed).
     IMMEDIATE_CRASH();
 }
 
 static ALWAYS_INLINE void partitionDecommitSystemPages(PartitionRootBase* root, void* addr, size_t len)
 {
     decommitSystemPages(addr, len);
-    ASSERT(root->totalSizeOfCommittedPages > len);
+    ASSERT(root->totalSizeOfCommittedPages >= len);
     root->totalSizeOfCommittedPages -= len;
 }
 
@@ -312,6 +328,7 @@ static ALWAYS_INLINE void partitionRecommitSystemPages(PartitionRootBase* root,
 {
     recommitSystemPages(addr, len);
     root->totalSizeOfCommittedPages += len;
+    ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);
 }
 
 static ALWAYS_INLINE void* partitionAllocPartitionPages(PartitionRootBase* root, int flags, uint16_t numPartitionPages)
@@ -320,22 +337,27 @@ static ALWAYS_INLINE void* partitionAllocPartitionPages(PartitionRootBase* root,
     ASSERT(!(reinterpret_cast<uintptr_t>(root->nextPartitionPageEnd) % kPartitionPageSize));
     RELEASE_ASSERT(numPartitionPages <= kNumPartitionPagesPerSuperPage);
     size_t totalSize = kPartitionPageSize * numPartitionPages;
-    root->totalSizeOfCommittedPages += totalSize;

[Chris Evans, 2014/11/08 07:07:15]

Whoa, the changes in this file look like an important bugfix: the accounting is
wrong if we subsequently fail the allocation.

Feel free to extract out the changes in this function (minus the new ASSERTs)
and TBR it to me.

[hiroshige, 2014/11/11 09:41:30]

Done.

     size_t numPartitionPagesLeft = (root->nextPartitionPageEnd - root->nextPartitionPage) >> kPartitionPageShift;
     if (LIKELY(numPartitionPagesLeft >= numPartitionPages)) {
         // In this case, we can still hand out pages from the current super page
         // allocation.
         char* ret = root->nextPartitionPage;
         root->nextPartitionPage += totalSize;
+        root->totalSizeOfCommittedPages += totalSize;
+        ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);
         return ret;
     }
 
     // Need a new super page.
-    root->totalSizeOfSuperPages += kSuperPageSize;
     char* requestedAddress = root->nextSuperPage;
     char* superPage = reinterpret_cast<char*>(allocPages(requestedAddress, kSuperPageSize, kSuperPageSize));
     if (UNLIKELY(!superPage))
         return 0;
+
+    root->totalSizeOfSuperPages += kSuperPageSize;
+    root->totalSizeOfCommittedPages += totalSize;
+    ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);
+
     root->nextSuperPage = superPage + kSuperPageSize;
     char* ret = superPage + kPartitionPageSize;
     root->nextPartitionPage = ret + totalSize;
@@ -566,6 +588,8 @@ static ALWAYS_INLINE void* partitionDirectMap(PartitionRootBase* root, int flags
     mapSize &= kPageAllocationGranularityBaseMask;
 
     root->totalSizeOfCommittedPages += size + kSystemPageSize;

[Chris Evans, 2014/11/08 07:04:44]

Nit: calculate "size + kSystemPageSize" into a separate variable to avoid
repetition?

[hiroshige, 2014/11/11 09:41:30]

Done.

+    root->totalSizeOfDirectMappedPages += size + kSystemPageSize;
+    ASSERT(root->totalSizeOfCommittedPages <= root->totalSizeOfSuperPages + root->totalSizeOfDirectMappedPages);
 
     // TODO: we may want to let the operating system place these allocations
     // where it pleases. On 32-bit, this might limit address space
@@ -621,7 +645,10 @@ static ALWAYS_INLINE void partitionDirectUnmap(PartitionPage* page)
     unmapSize += kPartitionPageSize + kSystemPageSize;
 
     PartitionRootBase* root = partitionPageToRoot(page);

[Chris Evans, 2014/11/08 07:04:44]

Nit: calculate "page->bucket->slotSize + kSystemPageSize" into a separate
variable to avoid repetition?

[hiroshige, 2014/11/11 09:41:29]

Done.

+    ASSERT(root->totalSizeOfCommittedPages >= page->bucket->slotSize + kSystemPageSize);
     root->totalSizeOfCommittedPages -= page->bucket->slotSize + kSystemPageSize;
+    ASSERT(root->totalSizeOfDirectMappedPages >= page->bucket->slotSize + kSystemPageSize);
+    root->totalSizeOfDirectMappedPages -= page->bucket->slotSize + kSystemPageSize;
 
     ASSERT(!(unmapSize & kPageAllocationGranularityOffsetMask));
 
@@ -701,7 +728,7 @@ void* partitionAllocSlowPath(PartitionRootBase* root, int flags, size_t size, Pa
 partitionAllocSlowPathFailed:
     if (returnNull)
         return nullptr;
-    partitionOutOfMemory();
+    partitionOutOfMemory(root);
     return nullptr;
 }