Bug 188328 – Shrink size of PropertyCondition by packing UniquedStringImpl* and Kind

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Shrink size of PropertyCondition by packing UniquedStringImpl* and Kind

[Yusuke Suzuki]

Created attachment 346580 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 346580 [details]
Patch

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

> Source/WTF/wtf/PointerWithType.h:33
> +// The goal of this class is folding a pointer and 1 byte value into 8 bytes in both 32bit and 64bit architectures.
> +// 32bit architecture just has a pair of byte and pointer, which should be 8 bytes. In 64bit, we use upper 16 bits.

Currently we do not consider about environments using 5-level page tables.
Even it is used, we can still use this strategy if we fold 8bits into upper 4bits (unused part of virtual address) and lower 4bits (alignment bits).

[Saam Barati]

Comment on attachment 346580 [details]
Patch

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

> Source/WTF/ChangeLog:7
> +

You should explain the type you’re adding here

> Source/WTF/wtf/PointerWithType.h:35
> +template<typename PointerType, typename Type>

Do we want to static assert that Type is trivially constructable or maybe even that it’s integral?

> Source/WTF/wtf/PointerWithType.h:36
> +class PointerWithType {

The “WithType” feels like the wrong name here. It reads to me as if you’re describing the type of the pointer. Maybe we can come up with a more descriptive name, some ideas:
- PointerTuple
- CompactPointerTuple

> Source/WTF/wtf/PointerWithType.h:38
> +    static_assert(sizeof(Type) == 1, "");

Seems like you’d allow up to 2 bytes.

> Source/WTF/wtf/PointerWithType.h:42
> +#if USE(JSVALUE64)

I think more accurately you want this conditionalized on sizeof(void*) == 8

There are 64_32 architectures where pointers are 4 bytes

> Source/WTF/wtf/PointerWithType.h:48
> +    PointerType pointer() const { return bitwise_cast<PointerType>(m_data & 0x0000FFFFFFFFFFFFULL); }

This could be where things go wrong on 64_32 archs

[Yusuke Suzuki]

Comment on attachment 346580 [details]
Patch

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

>> Source/WTF/ChangeLog:7
>> +
> 
> You should explain the type you’re adding here

Added.

>> Source/WTF/wtf/PointerWithType.h:35
>> +template<typename PointerType, typename Type>
> 
> Do we want to static assert that Type is trivially constructable or maybe even that it’s integral?

Sounds nice. I'll use `std::is_integral`.

>> Source/WTF/wtf/PointerWithType.h:36
>> +class PointerWithType {
> 
> The “WithType” feels like the wrong name here. It reads to me as if you’re describing the type of the pointer. Maybe we can come up with a more descriptive name, some ideas:
> - PointerTuple
> - CompactPointerTuple

CompactPointerTuple sounds good. Fixed.

>> Source/WTF/wtf/PointerWithType.h:38
>> +    static_assert(sizeof(Type) == 1, "");
> 
> Seems like you’d allow up to 2 bytes.

While JSValue typically only takes JSCells which are allocated from JS heap, this type can take arbitrary pointer.
If our architecture starts using 5-level page tables, we may see 57bit effective addresses in this type.
I would like to keep this size since I want to support x86_64 5-level page table too where the effective pointer bits are 57bits.
I've reworked this patch to support it.

>> Source/WTF/wtf/PointerWithType.h:42
>> +#if USE(JSVALUE64)
> 
> I think more accurately you want this conditionalized on sizeof(void*) == 8
> 
> There are 64_32 architectures where pointers are 4 bytes

I've added a new `CPU(ADDRESS64)` and `CPU(ADDRESS32)` in Platform.h to switch the implementation.

>> Source/WTF/wtf/PointerWithType.h:48
>> +    PointerType pointer() const { return bitwise_cast<PointerType>(m_data & 0x0000FFFFFFFFFFFFULL); }
> 
> This could be where things go wrong on 64_32 archs

OK, I've switched the implementation based on actual address width.

[Yusuke Suzuki]

Created attachment 346586 [details]
Patch

[EWS Watchlist]

Attachment 346586 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CompactPointerTuple.h:42:  The parameter name ">" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 346587 [details]
Patch

[EWS Watchlist]

Attachment 346587 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CompactPointerTuple.h:42:  The parameter name ">" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 346588 [details]
Patch

[EWS Watchlist]

Attachment 346588 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/CompactPointerTuple.h:42:  The parameter name ">" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 346580 [details]
Patch

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

>>> Source/WTF/wtf/PointerWithType.h:38
>>> +    static_assert(sizeof(Type) == 1, "");
>> 
>> Seems like you’d allow up to 2 bytes.
> 
> While JSValue typically only takes JSCells which are allocated from JS heap, this type can take arbitrary pointer.
> If our architecture starts using 5-level page tables, we may see 57bit effective addresses in this type.
> I would like to keep this size since I want to support x86_64 5-level page table too where the effective pointer bits are 57bits.
> I've reworked this patch to support it.

That's fine, but a lot of things would probably break on such architecture. Probably at least:
- JSCells
- DFG::Edge

[Saam Barati]

Comment on attachment 346588 [details]
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

[Yusuke Suzuki]

Comment on attachment 346588 [details]
Patch

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

Thank you!

>> 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"

Fixed.

>> Source/JavaScriptCore/ChangeLog:11
>> +        in their member.
> 
> in their member => as a member field

Fixed.

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

Oops, nice, fixed.

>> Source/WTF/wtf/CompactPointerTuple.h:36
>> +// 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.

Currently, we only use a type with 8bytes alignment. Note that we can extend this class to support 2bytes alignment class by storing 7bits in upper side and 1bit in lower side.
I think we can relax that when we need to support the other types having smaller alignment requirement.

>> 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?

Yes, encoding a type into a data that can be OR-ed to a pointer as much as efficient.

>> 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

Added.

>> 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

Nice, fixed.

>> 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

Nice, fixed.

[Yusuke Suzuki]

Committed r234677: <https://trac.webkit.org/changeset/234677>

[Radar WebKit Bug Importer]

<rdar://problem/43025966>