Bug 204555 – [JSC] Introduce IsoHeapCellType

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

[Yusuke Suzuki]

[JSC] Introduce IsoHeapCellType

[Yusuke Suzuki]

Created attachment 384251 [details]
Patch

[Yusuke Suzuki]

Created attachment 384252 [details]
Patch

[Mark Lam]

Comment on attachment 384252 [details]
Patch

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

Nice work.  r=me with suggestions.

> Source/JavaScriptCore/heap/IsoHeapCellType.h:35
> +class IsoHeapCellType final : public HeapCellType {
> +public:
> +    IsoHeapCellType()
> +        : HeapCellType(CellAttributes(CellType::needsDestruction ? NeedsDestruction : DoesNotNeedDestruction, HeapCell::JSCell))

If I understand you correctly, you intend to only use IsoHeapCellType for destructible types, yes?  Why not:
1. name it IsoHeapDestructibleCellType?
2. Always pass NeedsDestruction down to HeapCellType constructor here?
3. static_assert(CellType::needsDestruction)?

> Source/JavaScriptCore/heap/IsoHeapCellType.h:53
> +        DestroyFunc()(vm, cell);

Why not just go direct to CellType::destroy(cell) here?

[Yusuke Suzuki]

Comment on attachment 384252 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/heap/IsoHeapCellType.h:35
>> +        : HeapCellType(CellAttributes(CellType::needsDestruction ? NeedsDestruction : DoesNotNeedDestruction, HeapCell::JSCell))
> 
> If I understand you correctly, you intend to only use IsoHeapCellType for destructible types, yes?  Why not:
> 1. name it IsoHeapDestructibleCellType?
> 2. Always pass NeedsDestruction down to HeapCellType constructor here?
> 3. static_assert(CellType::needsDestruction)?

Currently, I'm considering using this for all IsoSubspace types (or, merging it into IsoSubspace if possible). So for now, I would like to make it usable for non destructible cell type too.

>> Source/JavaScriptCore/heap/IsoHeapCellType.h:53
>> +        DestroyFunc()(vm, cell);
> 
> Why not just go direct to CellType::destroy(cell) here?

It is OK too. Changed.

[Yusuke Suzuki]

Committed r252843: <https://trac.webkit.org/changeset/252843>

[Radar WebKit Bug Importer]

<rdar://problem/57461757>

[Mark Lam]

Comment on attachment 384252 [details]
Patch

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

>>> Source/JavaScriptCore/heap/IsoHeapCellType.h:35
>>> +        : HeapCellType(CellAttributes(CellType::needsDestruction ? NeedsDestruction : DoesNotNeedDestruction, HeapCell::JSCell))
>> 
>> If I understand you correctly, you intend to only use IsoHeapCellType for destructible types, yes?  Why not:
>> 1. name it IsoHeapDestructibleCellType?
>> 2. Always pass NeedsDestruction down to HeapCellType constructor here?
>> 3. static_assert(CellType::needsDestruction)?
> 
> Currently, I'm considering using this for all IsoSubspace types (or, merging it into IsoSubspace if possible). So for now, I would like to make it usable for non destructible cell type too.

How is it possible to use this class for non-destructible cell types too given that instantiation of this class requires that the underlying CellType has a destroy() function?  Or are you proposing to add no-op destroy() functions to all non-destructible cell types?

[Yusuke Suzuki]

Comment on attachment 384252 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/IsoHeapCellType.h:35
>>>> +        : HeapCellType(CellAttributes(CellType::needsDestruction ? NeedsDestruction : DoesNotNeedDestruction, HeapCell::JSCell))
>>> 
>>> If I understand you correctly, you intend to only use IsoHeapCellType for destructible types, yes?  Why not:
>>> 1. name it IsoHeapDestructibleCellType?
>>> 2. Always pass NeedsDestruction down to HeapCellType constructor here?
>>> 3. static_assert(CellType::needsDestruction)?
>> 
>> Currently, I'm considering using this for all IsoSubspace types (or, merging it into IsoSubspace if possible). So for now, I would like to make it usable for non destructible cell type too.
> 
> How is it possible to use this class for non-destructible cell types too given that instantiation of this class requires that the underlying CellType has a destroy() function?  Or are you proposing to add no-op destroy() functions to all non-destructible cell types?

JSCell class has destroy() function, which has RELEASE_ASSERT. So instantiating this works. And it is not called since DoesNotNeedDestruction is specified. So, IsoHeapCellType<NonDestructibleCell> just works.