Bug 193612 – [JSC] Shrink data structure size in JSC/heap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Shrink data structure size in JSC/heap

[Yusuke Suzuki]

Created attachment 359603 [details]
Patch

[Saam Barati]

Comment on attachment 359603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359603&action=review

> Source/JavaScriptCore/heap/Heap.h:581
> +    Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_collectionScope;

I really hate the name of this "Markable" class. Why don't we just encode this stuff in Optional via some extra type parameter?

[Yusuke Suzuki]

Comment on attachment 359603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359603&action=review

Thanks!

>> Source/JavaScriptCore/heap/Heap.h:581
>> +    Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_collectionScope;
> 
> I really hate the name of this "Markable" class. Why don't we just encode this stuff in Optional via some extra type parameter?

This is because `WTF::Optional` was `std::optional` when `WTF::Markable` is introduced. We do not have the way to modify `std::optional` at that point.
Anyway, now we have a chance to do that. Opened the bug for that. https://bugs.webkit.org/show_bug.cgi?id=193625

[Yusuke Suzuki]

Committed r240216: <https://trac.webkit.org/changeset/240216>

[Radar WebKit Bug Importer]

<rdar://problem/47416829>

[Saam Barati]

Comment on attachment 359603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359603&action=review

>>> Source/JavaScriptCore/heap/Heap.h:581
>>> +    Markable<CollectionScope, EnumMarkableTraits<CollectionScope>> m_collectionScope;
>> 
>> I really hate the name of this "Markable" class. Why don't we just encode this stuff in Optional via some extra type parameter?
> 
> This is because `WTF::Optional` was `std::optional` when `WTF::Markable` is introduced. We do not have the way to modify `std::optional` at that point.
> Anyway, now we have a chance to do that. Opened the bug for that. https://bugs.webkit.org/show_bug.cgi?id=193625

Thanks. I also meant to say that this is completely unrelated to this patch. This patch LGTM