Summary: | [JSC] Compact StructureTransitionTable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, simon.fraser, tuomas.webkit, tzagallo, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Yusuke Suzuki
2020-02-12 00:29:58 PST
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> *** Bug 186728 has been marked as a duplicate of this bug. *** |