The following member variables of Heap are dependent only for vmPageSize and calculating same value for each Heap. m_vmPageSizePhysical, m_smallLineMetadata and m_pageClasses Extract them as a separate class and access them from each Heap reduces sizeof(Heap) for m_pageClasses and Vector for m_smallLineMetadata.
Created attachment 386991 [details] PATCH - Extracted member variables for metadata and initialization code of those. - Merge LineMetadata.h and Metadata.h - Add simple wrapper class to get metadata for specific sizeClass.
Created attachment 386995 [details] PATCH build fix
The style error is false positive and it will be solved by https://bugs.webkit.org/show_bug.cgi?id=205840 .
jsc fails because it uses old version of forwarding header. Heap.h doesn't include LineMetadata.h any more and the line doesn't exist.
Created attachment 387165 [details] PATCH Add ChangeLog description.
(In reply to Basuke Suzuki from comment #4) > jsc fails because it uses old version of forwarding header. Heap.h doesn't > include LineMetadata.h any more and the line doesn't exist. I think the problem is that "bmalloc.h" and "Heap.h" are not copied correctly via CpHeader. Can you make it work?
What's the benefit here? Heap is a per-process singleton, so I don't understand the value of moving its data into another per-process singleton.
Side note: I think the word "metadata" is too generic. Can you think of a name that describes the data recorded here?
(In reply to Geoffrey Garen from comment #7) > What's the benefit here? > > Heap is a per-process singleton, so I don't understand the value of moving > its data into another per-process singleton. Heap is created as PerProcess<PerHeapKind>() singleton. So four copies of pageClass and smallLineMetadata tables are created. Also smallLineMetadata is a Vector, so it allocates another page for each Heap kind.
(In reply to Geoffrey Garen from comment #8) > Side note: I think the word "metadata" is too generic. Can you think of a > name that describes the data recorded here? How about HeapMetadata? Or I can merge them into Heap.(h|cpp) if that is more suitable.
(In reply to Yusuke Suzuki from comment #6) > (In reply to Basuke Suzuki from comment #4) > > jsc fails because it uses old version of forwarding header. Heap.h doesn't > > include LineMetadata.h any more and the line doesn't exist. > > I think the problem is that "bmalloc.h" and "Heap.h" are not copied > correctly via CpHeader. Can you make it work? Does jsc build use different path than other Xcode targets?
Just in case this is the goal of this patch. I'm working on making those values `constxpr`ed for specific VM page sizes. https://github.com/WebKit/webkit/compare/master...basuke:basuke/bmalloc-metadata-constexpr?expand=1#diff-590a267487c8ee077df1036f99e1c38cR75-R98
Yes, Heap exists at least three due to Gigacage IIRC (Primitive, JSValue, FastMalloc). And IIRC, Metadata takes measurable size (I noticed that when doing memory reduction work. If we avoid Heap initialization completely when Malloc=1 is enabled (we use DebugHeap), we get large improvement in RAMification LuaJSFight).
Comment on attachment 387165 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=387165&action=review > Source/bmalloc/bmalloc/Heap.cpp:284 > - size_t pageClass = m_pageClasses[sizeClass]; > + Metadata metadata { sizeClass }; > + size_t pageClass = metadata.pageClass(); This idiom surprised me. There's nothing about the Metadata class name that says to me "I will look up the right page class for a size class upon initialization". What do you think about "m_constants.pageClass(sizeClass)" instead? > Source/bmalloc/bmalloc/Metadata.hSource/bmalloc/bmalloc/LineMetadata.h:42 > +class MetadataStorage : public StaticPerProcess<MetadataStorage> { How about if we call this class "HeapConstants", since it's the factored out set of constants shared by all heaps -- and then keep a reference m_heapConstants in each Heap?
Comment on attachment 387165 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=387165&action=review >> Source/bmalloc/bmalloc/Heap.cpp:284 >> + size_t pageClass = metadata.pageClass(); > > This idiom surprised me. There's nothing about the Metadata class name that says to me "I will look up the right page class for a size class upon initialization". > > What do you think about "m_constants.pageClass(sizeClass)" instead? I agree with that. Actually that was my first implementation. I saw lot of sizeClass passing to get value from metadata, so I decided to make helper class. But yes that idiom is not regular. >> Source/bmalloc/bmalloc/Metadata.hSource/bmalloc/bmalloc/LineMetadata.h:42 >> +class MetadataStorage : public StaticPerProcess<MetadataStorage> { > > How about if we call this class "HeapConstants", since it's the factored out set of constants shared by all heaps -- and then keep a reference m_heapConstants in each Heap? HeapConstants for class name and having a reference to that instance in Heap are good to me. I want to call it m_constants because it's already used in Heap object.
Created attachment 387259 [details] PATCH
Comment on attachment 387259 [details] PATCH r=me
Comment on attachment 387259 [details] PATCH Clearing flags on attachment: 387259 Committed r254303: <https://trac.webkit.org/changeset/254303>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58459009>