Summary: | [JSC] Introduce IsoHeapCellType | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Yusuke Suzuki
2019-11-24 04:21:30 PST
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> 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. |