[webkit-reviews] review granted: [Bug 192389] bmalloc uses more memory on iOS compared to macOS due to physical page size differences : [Attachment 361348] Updated patch responding to review comments

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for 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 361348: Updated patch responding to review comments

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




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 361348
  --> https://bugs.webkit.org/attachment.cgi?id=361348
Updated patch responding to review comments

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

r=me with suggestions.

> Source/bmalloc/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=192389
> +	   bmalloc uses more memory on iOS compared to macOS due to physical
page size differences

This is inverted.  Michael said he'll fix locally.

> Source/bmalloc/bmalloc/Heap.cpp:280
> +    size_t matchingPageCount = 0;

nit: let's use the { 0 } initializer style to be consistent with the lines
above.	Or change the lines above to be consistent with this line.  It just
looks strange.

> Source/bmalloc/bmalloc/Heap.cpp:283
> +    unsigned smallPageCount = m_vmPageSizePhysical / pageSize;

I suggest renaming smallPageCount to smallPagesPerPhysicalPage.  See below for
reason.

> Source/bmalloc/bmalloc/Heap.cpp:296
> +		   firstPageToDecommit = firstPageInRange;
> +		   pagesToDecommit = matchingPageCount;

We don't need these.  All we need to know is that matchingPageCount ==
smallPageCount.  If so, we will recommit the page.

Given that you can remove firstPageToDecommit and pagesToDecommit here, I
suggest renaming matchingPageCount to pagesToDecommit.	I think that reads
better, and focus on the idea that we'll only decommit if pagesToDecommit ==
smallPagesPerPhysicalPage.  You can decide.

> Source/bmalloc/bmalloc/Heap.cpp:303
> +    if (firstPageToDecommit != firstPageInRange || pagesToDecommit !=
smallPageCount)
> +	   return;

This can check for (matchingPageCount < smallPageCount) instead ... or
(pagesToDecommit < smallPagesPerPhysicalPage) if you choose my suggested
renaming.

> Source/bmalloc/bmalloc/Heap.cpp:316
> +    m_physicalPageMap.decommit(firstPageToDecommit, decommitSize);

Just use physicalPagesBegin here instead of firstPageToDecommit.