[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
bugzilla-daemon at webkit.org
Wed Feb 6 17:39:35 PST 2019
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.
More information about the webkit-reviews
mailing list