[webkit-reviews] review denied: [Bug 171384] bmalloc scavenger should know what page classes are allocating : [Attachment 308418] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 171384: bmalloc scavenger should know what page classes are allocating
https://bugs.webkit.org/show_bug.cgi?id=171384

Attachment 308418: Patch

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




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

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

We need to measure MallocBench with this change.

> Source/bmalloc/bmalloc/Heap.cpp:119
> +    m_isPageClassAllocating.fill(false);

The default constructor does this for us. You can remove this.

> Source/bmalloc/bmalloc/Heap.cpp:-136
> -    waitUntilFalse(lock, sleepDuration, m_isAllocatingPages);

I think we still want this initial wait. Otherwise, we'll reclaim everything
before we have time to learn that we're still allocating pages.

> Source/bmalloc/bmalloc/Heap.cpp:159
> +		   allocationInProgress = true;
> +		   break;

Instead of taking an out parameter, we should just reschedule ourselves here
when we learn that we have more work to do.

> Source/bmalloc/bmalloc/Heap.cpp:163
>	       m_vmHeap.deallocateSmallPage(lock, pageClass, page);

I think it's a pre-existing bug that we don't drop the lock around the system
call, like we do in scavengeLargeObjects. We should probably fix that too.

> Source/bmalloc/bmalloc/Heap.cpp:183
> +	       allocationInProgress = true;
> +	       break;

Ditto.

Also, I would put this check at the top of the loop. Otherwise, you always
deallocate one.

> Source/bmalloc/bmalloc/Heap.h:120
> +    std::array<bool, pageClassCount + 1> m_isPageClassAllocating; // Extra
class is for large objects.

Let's call this m_isAllocatingPages and only use pageClassCount items.

> Source/bmalloc/bmalloc/Heap.h:121
> +#define isAllocatingLargePages m_isPageClassAllocating[pageClassCount]

Let's make this a data member instead of a #define: m_largeIsAllocatingPages.

> Source/bmalloc/bmalloc/bmalloc.h:79
> -    PerProcess<Heap>::get()->scavenge(lock, std::chrono::milliseconds(0));
> +    PerProcess<Heap>::get()->scavenge(lock);

I think we've lost the behavior where this function should synchronously
guarantee (or nearly guarantee) a complete scavenge before any new allocations
took place. That's an important thing to do in memory measurement and also in
our critical memory warning handler.

One option, which is a little gross, is to pass a fake mutex to our callee, so
the lock never actually unlocks. Another option is to pass a bool/enum
parameter that says whether to unlock or not. Or maybe pass some kind of helper
lambda or object for unlocking.