[webkit-reviews] review granted: [Bug 195895] [BMalloc] Scavenger should react to recent memory activity : [Attachment 365037] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted  review:
Bug 195895: [BMalloc] Scavenger should react to recent memory activity
https://bugs.webkit.org/show_bug.cgi?id=195895

Attachment 365037: Patch

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




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

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

r=me

Please make sure this is unrelated:

** The following JSC stress test failures have been introduced:
       
stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js.default

> Source/bmalloc/bmalloc/Chunk.h:66
> +    unsigned char m_usedSinceLastScavenge: 1;

Might as well just say bool here, since we aren't saving any bits.

> Source/bmalloc/bmalloc/Heap.cpp:532
> +	   range.setUsedSinceLastScavenge();

We don't need this explicit set because 'range' is a temporary.

> Source/bmalloc/bmalloc/LargeRange.h:60
> +	   , m_usedSinceLastScavenge(true)

I think m_used should default to false, and be set to true inside
LargeMap::add. That's a little clearer.

In the future, we can consider whether it's really optimal for any allocation
from a large range to prevent reclamation of the unused portion(s) of the large
range.

> Source/bmalloc/bmalloc/LargeRange.h:101
> +    unsigned m_isEligible: 1;

I think m_isEligible is a leftover from the previous partial strategy. You
should remove it in a follow-up patch.

> Source/bmalloc/bmalloc/LargeRange.h:102
> +    unsigned m_usedSinceLastScavenge: 1;

You should update the merge function to OR together the m_used bits. I don't
think that changes behavior for now, but it's good hygiene.

> Source/bmalloc/bmalloc/Scavenger.cpp:340
> +	   if (m_isInMiniMode)
> +	       m_waitTime =
std::min(std::max(static_cast<long>(timeSpentScavenging) * 50L / 1000L, 25L),
500L);
> +	   else
> +	       m_waitTime =
std::min(std::max(static_cast<long>(timeSpentScavenging) * 150L / 1000L, 100L),
10000L);

Let's do the conversion to milliseconds at the end, so that it's not embedded
inside the min/max logic. Also, can we do the conversion using duration_cast
rather than manual multiplication?

> Source/bmalloc/bmalloc/Scavenger.h:95
> +    long m_waitTime;

It looks like we always interpret m_waitTime as std::chrono::milliseconds. Can
you set the datatype to std::chrono::milliseconds?