RESOLVED FIXED 233918
[JSC] Introduce WriteBarrierStructureID 
https://bugs.webkit.org/show_bug.cgi?id=233918
Summary [JSC] Introduce WriteBarrierStructureID
Yusuke Suzuki
Reported 2021-12-07 01:39:29 PST
[JSC] Introduce WriteBarrierStructureID
Attachments
Patch (35.57 KB, patch)
2021-12-07 08:52 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (55.74 KB, patch)
2021-12-07 17:11 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (61.17 KB, patch)
2021-12-07 17:28 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (62.89 KB, patch)
2021-12-07 17:42 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (73.74 KB, patch)
2021-12-07 18:20 PST, Yusuke Suzuki 		mark.lam: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2021-12-07 08:52:18 PST
Created attachment 446178 [details] Patch
Yusuke Suzuki
Comment 2 2021-12-07 17:11:49 PST
Created attachment 446259 [details] Patch
Yusuke Suzuki
Comment 3 2021-12-07 17:28:10 PST
Created attachment 446262 [details] Patch
Yusuke Suzuki
Comment 4 2021-12-07 17:42:59 PST
Created attachment 446270 [details] Patch
Yusuke Suzuki
Comment 5 2021-12-07 18:20:35 PST
Created attachment 446273 [details] Patch
Mark Lam
Comment 6 2021-12-08 01:08:41 PST
Comment on attachment 446273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446273&action=review r=me > Source/JavaScriptCore/runtime/WriteBarrier.h:278 > + // Should only be used by JSCell during early initialisation > + // when some basic types aren't yet completely instantiated Why this comment? In this patch, `setEarlyValue` looks more like a `setInternal` and can be made private. Is this needed in code that is yet to come? > Source/JavaScriptCore/runtime/WriteBarrier.h:283 > + // Copy m_cell to a local to avoid multiple-read issues. (See <http://webkit.org/b/110854>) /m_cell/m_structureID/ > Source/JavaScriptCore/runtime/WriteBarrier.h:313 > + m_structureID = StructureID { }; Do we need the `StructureID` here? Wouldn't it be implied by the type of m_structureID? > Source/JavaScriptCore/runtime/WriteBarrier.h:332 > + m_structureID = StructureID { }; Ditto. > Source/JavaScriptCore/runtime/WriteBarrierInlines.h:82 > + m_structureID = StructureID { }; Ditto.
Yusuke Suzuki
Comment 7 2021-12-08 11:04:58 PST
Comment on attachment 446273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446273&action=review >> Source/JavaScriptCore/runtime/WriteBarrier.h:278 >> + // when some basic types aren't yet completely instantiated > > Why this comment? In this patch, `setEarlyValue` looks more like a `setInternal` and can be made private. Is this needed in code that is yet to come? Because it is aligned to WriteBarrier<JSCell>, so same methods and same comments are prepared. So, we should use setEarlyValue to align it to WriteBarrier<>. >> Source/JavaScriptCore/runtime/WriteBarrier.h:283 >> + // Copy m_cell to a local to avoid multiple-read issues. (See <http://webkit.org/b/110854>) > > /m_cell/m_structureID/ Changed. >> Source/JavaScriptCore/runtime/WriteBarrier.h:313 >> + m_structureID = StructureID { }; > > Do we need the `StructureID` here? Wouldn't it be implied by the type of m_structureID? Dropped. >> Source/JavaScriptCore/runtime/WriteBarrier.h:332 >> + m_structureID = StructureID { }; > > Ditto. Ditto. >> Source/JavaScriptCore/runtime/WriteBarrierInlines.h:82 >> + m_structureID = StructureID { }; > > Ditto. Ditto.
Yusuke Suzuki
Comment 8 2021-12-08 11:11:57 PST
Committed r286667 (244975@trunk): <https://commits.webkit.org/244975@trunk>
Radar WebKit Bug Importer
Comment 9 2021-12-08 11:12:22 PST
<rdar://problem/86220662>
Note You need to log in before you can comment on or make changes to this bug.