[webkit-reviews] review denied: [Bug 230143] [bmalloc] Merge large frees after making them eligible : [Attachment 438567] patch

[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 438567: patch

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




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

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

> Source/bmalloc/ChangeLog:22
> +	   Here's a quick result from testmem on MacBook Air M1. The
differences are tiny.

This patch seems to include some subtleties, including a potential throughput
penalty at the end of scavenging. Can we point to a clearer memory improvement
that would justify the subtleties and the potential throughput penalty? We
generally prefer not to make changes for performance unless we can measure the
impact.

> Source/bmalloc/bmalloc/LargeMap.cpp:69
>  void LargeMap::add(const LargeRange& range)

Why does LargeMap::add change at all in this patch? Why isn't the heap always
back in a fully merged, consistent state after LargeMap::mergeNeighbors() has
returned?

> Source/bmalloc/bmalloc/LargeMap.cpp:90
> +	       auto* next = it + 1;
> +	       if (next != m_free.end() && canMerge(*it, *next)) {
> +		   *it = merge(*it, *next);
> +		   m_free.remove(next);
> +	       }

I don't think it's right to check only the immediate neighbor (it + 1) for
merging. m_free is not sorted, so I'm not sure what this would achieve.

> Source/bmalloc/bmalloc/LargeMap.cpp:112
> +	   auto* next = &range + 1;
> +
> +	   while (next < m_free.end() && canMerge(range, *next))
> +	       range = merge(range, *next++);

Same comment about it + 1.

> Source/bmalloc/bmalloc/Vector.h:174
> +    BASSERT(from < to);
> +    size_t moveCount = end() - to;
> +    if (moveCount)
> +	   std::memmove(from, to, moveCount * sizeof(T));

No need to check moveCount here.