Bug 207616 - [JSC] Compact StructureTransitionTable
Summary: [JSC] Compact StructureTransitionTable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-12 00:29 PST by Yusuke Suzuki
Modified: 2020-02-12 12:33 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.96 KB, patch)
2020-02-12 00:31 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2020-02-12 00:33 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-02-12 00:29:58 PST
[JSC] Compact StructureTransitionTable
Comment 1 Yusuke Suzuki 2020-02-12 00:31:10 PST
Created attachment 390495 [details]
Patch
Comment 2 Yusuke Suzuki 2020-02-12 00:33:39 PST
Created attachment 390496 [details]
Patch
Comment 3 Tuomas Karkkainen 2020-02-12 02:27:48 PST
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.)
Comment 4 Mark Lam 2020-02-12 11:09:33 PST
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.
Comment 5 Mark Lam 2020-02-12 11:12:18 PST
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.
Comment 6 Yusuke Suzuki 2020-02-12 12:24:28 PST
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.
Comment 7 Yusuke Suzuki 2020-02-12 12:32:37 PST
Committed r256468: <https://trac.webkit.org/changeset/256468>
Comment 8 Radar WebKit Bug Importer 2020-02-12 12:33:16 PST
<rdar://problem/59396999>