[webkit-reviews] review denied: [Bug 192389] bmalloc uses more memory on iOS compared to macOS due to physical page size differences : [Attachment 361362] Patch for Landing

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 192389: bmalloc uses more memory on iOS compared to macOS due to physical
page size differences
https://bugs.webkit.org/show_bug.cgi?id=192389

Attachment 361362: Patch for Landing

https://bugs.webkit.org/attachment.cgi?id=361362&action=review




--- Comment #17 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 361362
  --> https://bugs.webkit.org/attachment.cgi?id=361362
Patch for Landing

View in context: https://bugs.webkit.org/attachment.cgi?id=361362&action=review

> Source/bmalloc/bmalloc/Heap.cpp:136
> +	   // We only want power of 2 pageSizes. Given that smallPageSize is a
power of 2, we just double it when we want a larger size.
> +	   for (size_t pageSize = smallPageSize; pageSize < pageSizeMax;
pageSize *= 2) {

This isn't quite right. At page sizes above { 4kB on Mac, 16kB on iOS }, you
have a wasteful increase in page size.

We don't actually want powers of two. We want either even divisors of
m_vmPageSizePhysical or even multiples of m_vmPageSizePhysical. That's what
guarantees that we can madvise each physical page without waste.

You can get that effect by doing *= 2 up to m_vmPageSizePhysical and +=
m_vmPageSizePhysical above m_vmPageSizePhysical. I would write that as two
loops.

> Source/bmalloc/bmalloc/Heap.cpp:294
> +    for (auto* page : chunk->freePages()) {
> +	   if (page >= beginPageRange && page < endPageRange) {
> +	       smallPagesReadyToDecommit++;
> +	       if (smallPagesReadyToDecommit == smallPagePerPhysicalPage)
> +		   break;
> +	   }
> +    }

We don't need a linear search of the free page list to find { 2 or 4 } pages
thet are aligned neighbors.

Chunk needs this API:

    size_t pageNumber(SmallPage*);

You can make a mask like this:

    auto mask = smallPagePerPhysicalPage - 1;

Then you can iterate the pages you need like this:

auto pages = chunk->pages()[chunk->pageNumber(smallPage) & mask];
for (size_t i = 0; i < smallPagePerPhysicalPage; ++i) {
    auto page = pages[i];
    ...
}

Also, there's no need to count smallPagesReadyToDecommit. You can just return
if any page says it has a non-zero refCount().