Shrink size of PropertyCondition by packing UniquedStringImpl* and Kind
Created attachment 346580 [details] Patch
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).
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
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.
Created attachment 346586 [details] Patch
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.
Created attachment 346587 [details] Patch
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.
Created attachment 346588 [details] Patch
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.
Ping?
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
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
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.
Committed r234677: <https://trac.webkit.org/changeset/234677>
<rdar://problem/43025966>