[webkit-reviews] review denied: [Bug 171384] bmalloc scavenger should know what page classes are allocating : [Attachment 308418] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 27 12:20:14 PDT 2017
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.
More information about the webkit-reviews
mailing list