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..4ef6197579c4092d3f19df52ffbce38cecd1551f 100644
--- a/Source/wtf/PartitionAlloc.cpp
+++ b/Source/wtf/PartitionAlloc.cpp
@@ -296,8 +296,16 @@ bool partitionAllocGenericShutdown(PartitionRootGeneric* root)
     return noLeaks;
 }
 
-static NEVER_INLINE void partitionOutOfMemory()
+static NEVER_INLINE void partitionOutOfMemory(const PartitionRootBase* root)
 {
+    if ((uint64_t)root->totalSizeOfSuperPages - (uint64_t)root->totalSizeOfCommittedPages > kReasonableSizeOfUnusedPages) {

[Tom Sepez, 2014/11/06 17:32:51]

why this promotion to unit64_t? Also, are we sure that
root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages

[Chris Evans, 2014/11/06 20:43:22]

I don't think this condition will ever give more accurate results on 64-bit. I
propose #if CPU(32_BIT) for this entire if condition.

[Chris Evans, 2014/11/06 20:43:22]

I also don't like the look of these casts. Hopefully we can get rid of them. If
not, the coding style is wrong: we always use static_cast.

I do believe root->totalSizeOfSuperPages >= root->totalSizeOfCommittedPages
should be an invariant that holds. Maybe add some ASSERT() at the code locations
where we increase root->totalSizeOfCommittedPages to ASSERT it is still <=
root->totalSizeOfSuperPages ?

[Jens Widell, 2014/11/07 08:31:21]

Since we now include direct mapped allocations in totalSizeOfCommittedPages, it
can theoretically be bigger than totalSizeOfSuperPages. In theory, we could have
*only* direct mapped allocations, in which case totalSizeOfSuperPages==0 and
totalSizeOfCommittedPages>0.

[hiroshige, 2014/11/07 08:57:00]

I expect there are no out-of-virtual-address-space crash detected here, but I'd
like to enable this check in 64-bit to confirm that.

[hiroshige, 2014/11/07 08:57:00]

Added assertions, and removed casts.

[hiroshige, 2014/11/07 08:57:00]

Yes, and I added totalSizeOfDirectMappedPages in Patch Set 5.

+        // OOMs where a lot of super pages are allocated but not committed,
+        // probably due to http://crbug.com/421387.
+        // Crash at a special address (0x9b)
+        // to be easily distinguished on crash reports.
+        ((void(*)())0x0000009b)();

[Tom Sepez, 2014/11/06 17:32:51]

Maybe we need IMMEDIATE_CRASH_WITH_FLAG(0x9b) macro along the lines of
immediate_crash since there may be platforms where we want to use a builtin.

[Chris Evans, 2014/11/06 20:43:22]

I don't like this, it's a bit arbitrary and doesn't lead to an obviously visible
distinction in crash logs.

I'd prefer we did it this way:

partitionOutOfMemoryWithLotsOfUncommitedPages();

With:

static NEVER_INLINE void partitionOutOfMemoryWithLotsOfUncommitedPages()
{
    IMMEDIATE_CRASH();
}

[hiroshige, 2014/11/07 08:56:59]

cevans@, Agree, I also prefer what you suggested, but in Windows  many functions
(partitionOutOfMemoryWithLotsOfUncommitedPages in this case) won't appear in the
crash stack traces, even with the NEVER_INLINE directive.
I'm not sure why (frame pointer omission?), but in the crash reports, there are
no partitionOutOfMemory (or partitionFull) reported on Windows.

Does anyone knows how to work around this?

[hiroshige, 2014/11/07 10:28:17]

Added IMMEDIATE_CRASH_WITH_FLAG and
partitionOutOfMemoryWithLotsOfUncommitedPages() in Patch Set 6.

+    }
+    // Ordinary OOMs (where super pages are consumed and mostly committed).
     IMMEDIATE_CRASH();
 }
 
@@ -701,7 +709,7 @@ void* partitionAllocSlowPath(PartitionRootBase* root, int flags, size_t size, Pa
 partitionAllocSlowPathFailed:
     if (returnNull)
         return nullptr;
-    partitionOutOfMemory();
+    partitionOutOfMemory(root);
     return nullptr;
 }