[webkit-reviews] review granted: [Bug 226237] [bmalloc] Make adaptive scavenging more precise : [Attachment 429734] Updated patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 226237: [bmalloc] Make adaptive scavenging more precise
https://bugs.webkit.org/show_bug.cgi?id=226237

Attachment 429734: Updated patch

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




--- Comment #8 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 429734
  --> https://bugs.webkit.org/attachment.cgi?id=429734
Updated patch

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

r=me

> Source/bmalloc/ChangeLog:20
> +	   There is the possible future optimization where we also keep track
of the
> +	   first address of physically mapped memory in a LargeRange.  Since
bmalloc
> +	   allocates memory from lower addresses first, it is thought that the
change
> +	   in this patch is sufficient to reduce not only the number of madvise
calls,
> +	   but the time it takes to make those calls.

Might be worth mentioning that you tested this thought, and what the results
were.

> Source/bmalloc/bmalloc/Heap.cpp:-239
> -	   auto metadataSize = Chunk::metadataSize(pageSize);
> -	   vmDeallocatePhysicalPagesSloppy(chunk->address(sizeof(Chunk)),
metadataSize - sizeof(Chunk));
> -
> -	   auto decommitSize = chunkSize - metadataSize - accountedInFreeable;
> -	   if (decommitSize > 0)
> -	       vmDeallocatePhysicalPagesSloppy(chunk->address(chunkSize -
decommitSize), decommitSize);

I thought about this for a while and did not come up with a better idea than
removing these VM deallocations. But I think this change means we need to A/B
test Membuster and PLUM before we land.

> Source/bmalloc/bmalloc/Heap.cpp:231
> +	       if (calculatingStartPhysicalSize) {

I think this would be clearer if we had a local variable 'SmallPage*
firstPageWithoutPhysicalPages = nullptr'.

Then here we can have:

if (!firstPageWithoutPhysicalPages)
    firstPageWithoutPhysicalPages = page;

And below we can have:

size_t startPhysicalSize = firstPageWithoutPhysicalPages ?
firstPageWithoutPhysicalPages->begin()->begin() - chunk->bytes() : size;

That way, it's more explicit what the loop is searching for, and there's never
a time when startPhysicalSize holds an invalid value.

> Source/bmalloc/bmalloc/LargeRange.h:90
> +    // This is the last address of physical memory in this range.

last => past the end