[webkit-reviews] review granted: [Bug 189558] [JSC] Heap::reportExtraMemoryVisited shows contention if we have many JSString : [Attachment 350197] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 189558: [JSC] Heap::reportExtraMemoryVisited shows contention if we have
many JSString
https://bugs.webkit.org/show_bug.cgi?id=189558

Attachment 350197: Patch

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




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 350197
  --> https://bugs.webkit.org/attachment.cgi?id=350197
Patch

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

r=me

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:162
> +	   // FIXME: Change this to use SaturatedArithmetic when available.
> +	   // https://bugs.webkit.org/show_bug.cgi?id=170411
> +	   Checked<size_t, RecordOverflow> checkedNewSize = m_extraMemorySize;
> +	   checkedNewSize += size;
> +	   m_extraMemorySize = UNLIKELY(checkedNewSize.hasOverflowed()) ?
std::numeric_limits<size_t>::max() : checkedNewSize.unsafeGet();

I suggest just making m_extraMemorySize a Checked<size_t, RecordOverflow>
instead to simplify this.  That way, we won't be checking hasOverflowed()
repeatedly.  Note: the Checked add operator would already have checked
hasOverflowed().  Only need to check overflow in
propagateExternalMemoryVisitedIfNecessary().