[webkit-reviews] review denied: [Bug 230143] [bmalloc] Merge large frees after making them eligible : [Attachment 437992] Patch for landing 2

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 230143: [bmalloc] Merge large frees after making them eligible
https://bugs.webkit.org/show_bug.cgi?id=230143

Attachment 437992: Patch for landing 2

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




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

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

Can you separate the refactoring changes from the behavior changes in this
patch, and post them separately? It's subtle to figure out which is which here.

> Source/bmalloc/ChangeLog:11
> +	   Deallocated larges are stored in Heap's LargeFree. They're merged if
they are neighbors and
> +	   has same states. But chances are only when they added to the
LargeFree map. When they are
> +	   not eligible (it is likely possible) at that time, there's no chance
to be merged except
> +	   other neighbors are inserted.

I think what you're trying to say is that the scavenger may cause temporary
fragmentation when it calls setEligible(false). Is that right?

Did you discover this behavior in some workload? What were the specifics?

> Source/bmalloc/bmalloc/LargeMap.cpp:-76
> -	   merged = merge(merged, m_free.pop(i--));

Why is it OK to remove this merging step? If your plan is to wait for the call
to mergeNeighbors(), that's not OK, because it's not guaranteed to happen soon.