[webkit-reviews] review granted: [Bug 188328] Shrink size of PropertyCondition by packing UniquedStringImpl* and Kind : [Attachment 346588] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 188328: Shrink size of PropertyCondition by packing UniquedStringImpl* and
Kind
https://bugs.webkit.org/show_bug.cgi?id=188328

Attachment 346588: Patch

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




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 346588
  --> https://bugs.webkit.org/attachment.cgi?id=346588
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +	   Shrinking the size of PropertyCondition affects so much on memory
consumption.

"affects so much on memory consumption" => "can improve memory consumption by a
lot"

> Source/JavaScriptCore/ChangeLog:11
> +	   in their member.

in their member => as a member field

> Source/WTF/ChangeLog:8
> +	   This patch adds CompactPointerTuple, which can pack a pointer and
8bit value into 16bytes.

16bytes => 8bytes

> Source/WTF/wtf/CompactPointerTuple.h:36
> +// In 64bit, we use the upper 5 bits and lower 3 bits (zero due to
alignment) since these bits are safe to use even
> +// with 5-level page tables where the effective pointer width is 57bits.

It seems more likely that we'd want to use this with something where alignof <
sizeof(void*) (e.g, char*) than worrying about 57bit pointers.

> Source/WTF/wtf/CompactPointerTuple.h:49
> +    static constexpr uint64_t encodeType(uint8_t type)

Is the goal for this encoding to make retrieving the pointer a single
instruction?

> Source/WTF/wtf/CompactPointerTuple.h:65
> +	   : m_data(bitwise_cast<uint64_t>(pointer) |
encodeType(static_cast<uint8_t>(type)))

Let's debug assert bottom 3 bits are zero if we stick with the alignof
assertion

> Source/WTF/wtf/CompactPointerTuple.h:72
> +	   m_data = bitwise_cast<uint64_t>(pointer) | (m_data & typeMask);

ditto with asserting bottom 3 bits.

Style nit: This also may be more straight forward as an abstraction:

m_data = CompactPointerTuple(pointer, type()).m_data

> Source/WTF/wtf/CompactPointerTuple.h:78
> +	   m_data = (m_data & pointerMask) |
encodeType(static_cast<uint8_t>(type));

Style nit: This may be more straight forward as an abstraction:
m_data = CompactPointerTuple(pointer(), type).m_data