[JSC] Compact StructureTransitionTable
Created attachment 390495 [details] Patch
Created attachment 390496 [details] Patch
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 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 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 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.
Committed r256468: <https://trac.webkit.org/changeset/256468>
<rdar://problem/59396999>
*** Bug 186728 has been marked as a duplicate of this bug. ***