Bug 205834 – [bmalloc] Extract constants from Heap and share it among Heaps.

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Basuke Suzuki]

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.

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 386995 [details]
PATCH

build fix

[Basuke Suzuki]

The style error is false positive and it will be solved by https://bugs.webkit.org/show_bug.cgi?id=205840 .

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 387165 [details]
PATCH

Add ChangeLog description.

[Yusuke Suzuki]

(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?

[Geoffrey Garen]

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.

[Geoffrey Garen]

Side note: I think the word "metadata" is too generic. Can you think of a name that describes the data recorded here?

[Basuke Suzuki]

(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.

[Basuke Suzuki]

(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.

[Basuke Suzuki]

(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?

[Basuke Suzuki]

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

[Yusuke Suzuki]

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).

[Geoffrey Garen]

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?

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 387259 [details]
PATCH

[Geoffrey Garen]

Comment on attachment 387259 [details]
PATCH

r=me

[WebKit Commit Bot]

Comment on attachment 387259 [details]
PATCH

Clearing flags on attachment: 387259

Committed r254303: <https://trac.webkit.org/changeset/254303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58459009>