[JSC] Introduce IsoHeapCellType
Created attachment 384251 [details] Patch
Created attachment 384252 [details] Patch
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?
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.
Committed r252843: <https://trac.webkit.org/changeset/252843>
<rdar://problem/57461757>
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?
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.