Bug 207616 – [JSC] Compact StructureTransitionTable

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

[Yusuke Suzuki]

[JSC] Compact StructureTransitionTable

[Yusuke Suzuki]

Created attachment 390495 [details]
Patch

[Yusuke Suzuki]

Created attachment 390496 [details]
Patch

[Tuomas Karkkainen]

Comment on attachment 390496 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.cpp:130
> +    map()->set(StructureTransitionTable::Hash::Key(structure->m_transitionPropertyName.get(), +structure->transitionPropertyAttributes(), !structure->isPropertyDeletionTransition()), structure);

is the + needed on +structure->transitionPropertyAttributes() and if it isn't, should the comment above be removed? (I was able to build jsc with your patch but with the + removed.)

[Mark Lam]

Comment on attachment 390496 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Some of StructureTransitionTable are shown as very large HashMap and we can compact it by enconding key.

/enconding/encoding/

>> Source/JavaScriptCore/runtime/Structure.cpp:130
>> +    map()->set(StructureTransitionTable::Hash::Key(structure->m_transitionPropertyName.get(), +structure->transitionPropertyAttributes(), !structure->isPropertyDeletionTransition()), structure);
> 
> is the + needed on +structure->transitionPropertyAttributes() and if it isn't, should the comment above be removed? (I was able to build jsc with your patch but with the + removed.)

I think this + is just accidental and can be removed.

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:149
> +        // using Key = std::tuple<UniquedStringImpl*, unsigned, bool>;

Can you make this std::tuple<UniquedStringImpl*, unsigned transitionPropertyAttributes, bool isAddition> to better document the intended purpose of those parameters?

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:153
> +            static constexpr uintptr_t stringMask = ((1ULL << 48) - 1) & (~1ULL);
> +            static constexpr uintptr_t isAdditionMask = 1ULL;

Let's swap these 2 and change stringMask to be:
    static constexpr uintptr_t stringMask = ((1ULL << 48) - 1) & (~isAdditionMask);

This helps document that the lowest bit is where we store the isAddition flag.

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:160
> +            // Higher 16 bits for TransitionPropertyAttributes.
> +            // Lower 1 bit for isAddition flag.

/Higher 16 bits for/Highest 16 bits are for/
/Lower 1 bit for/Lowest 1 bit is for/

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:161
> +            // Remaiing bits for UniquedStringImpl*.

/Remaiing bits for/Remaining bits are for/

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:192
> +            uintptr_t m_encodedData { 0 };

Let's make this private.  If needed, make struct Hash a friend.

[Mark Lam]

Comment on attachment 390496 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.h:681
> +    static constexpr uint32_t s_##lowerName##Width = width;\

Let's make this BitWidth instead of Width.  I think that makes it clearer.

[Yusuke Suzuki]

Comment on attachment 390496 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:8
>> +        Some of StructureTransitionTable are shown as very large HashMap and we can compact it by enconding key.
> 
> /enconding/encoding/

Fixed.

>>> Source/JavaScriptCore/runtime/Structure.cpp:130
>>> +    map()->set(StructureTransitionTable::Hash::Key(structure->m_transitionPropertyName.get(), +structure->transitionPropertyAttributes(), !structure->isPropertyDeletionTransition()), structure);
>> 
>> is the + needed on +structure->transitionPropertyAttributes() and if it isn't, should the comment above be removed? (I was able to build jsc with your patch but with the + removed.)
> 
> I think this + is just accidental and can be removed.

Yeah, we can remove it. Fixed.

>> Source/JavaScriptCore/runtime/Structure.h:681
>> +    static constexpr uint32_t s_##lowerName##Width = width;\
> 
> Let's make this BitWidth instead of Width.  I think that makes it clearer.

I think `bitWidthOfXXX` is better here since this typename can include XXX typename without uncapitalizing it.
Fixed.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:149
>> +        // using Key = std::tuple<UniquedStringImpl*, unsigned, bool>;
> 
> Can you make this std::tuple<UniquedStringImpl*, unsigned transitionPropertyAttributes, bool isAddition> to better document the intended purpose of those parameters?

Added.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:153
>> +            static constexpr uintptr_t isAdditionMask = 1ULL;
> 
> Let's swap these 2 and change stringMask to be:
>     static constexpr uintptr_t stringMask = ((1ULL << 48) - 1) & (~isAdditionMask);
> 
> This helps document that the lowest bit is where we store the isAddition flag.

Fixed.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:160
>> +            // Lower 1 bit for isAddition flag.
> 
> /Higher 16 bits for/Highest 16 bits are for/
> /Lower 1 bit for/Lowest 1 bit is for/

Fixed.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:161
>> +            // Remaiing bits for UniquedStringImpl*.
> 
> /Remaiing bits for/Remaining bits are for/

Fixed.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:192
>> +            uintptr_t m_encodedData { 0 };
> 
> Let's make this private.  If needed, make struct Hash a friend.

Fixed.

[Yusuke Suzuki]

Committed r256468: <https://trac.webkit.org/changeset/256468>

[Radar WebKit Bug Importer]

<rdar://problem/59396999>

[Brent Fulgham]

*** Bug 186728 has been marked as a duplicate of this bug. ***