Bug 229748

Summary: Eagerly resolve slot elements to simply the code in SlotAssignment
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, kangil.han, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
Patch
none
Fixed the crash cdumez: review+

Ryosuke Niwa
Reported 2021-08-31 18:23:12 PDT
We have a rather elaborate logic to lazily resolve slot elements when there are multiple ones of the same name. Get rid of this code since that's a very rare case, and it doesn't really benefit perf much since the only case we'd get faster is when scripts inserts & removes slot elements repeatedly.
Attachments
Patch (14.94 KB, patch)
2021-08-31 18:36 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed the crash (16.15 KB, patch)
2021-09-01 02:00 PDT, Ryosuke Niwa 		cdumez: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2021-08-31 18:24:23 PDT
Actually, it's even more stupid. It only benefits details element since whenever slot change may be needed, we'd go through the slow path anyway. This is really stupid.
Ryosuke Niwa
Comment 2 2021-08-31 18:36:18 PDT
Created attachment 436980 [details] Patch
Ryosuke Niwa
Comment 3 2021-08-31 18:39:21 PDT
Comment on attachment 436980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436980&action=review > Source/WebCore/dom/SlotAssignment.cpp:-125 > - if (!m_slotAssignmentsIsValid) > - assignSlots(shadowRoot); FYI, this code was totally unnecessary. hasAssignedNodes calls it internally where it's needed In all other cases hasAssignedNodes is not called, we're just waiting CPU cycle traversing through nodes.
Ryosuke Niwa
Comment 4 2021-09-01 00:28:21 PDT
Ugh... I need to special case when we're in the middle of shadow root teardown.
Ryosuke Niwa
Comment 5 2021-09-01 02:00:25 PDT
Created attachment 437003 [details] Fixed the crash
Chris Dumez
Comment 6 2021-09-01 07:13:38 PDT
Comment on attachment 437003 [details] Fixed the crash View in context: https://bugs.webkit.org/attachment.cgi?id=437003&action=review > Source/WebCore/ChangeLog:8 > + This patch makes the resolution of slot elements eager. Lazily resolution stopping making any sense stopping -> stopped? > Source/WebCore/ChangeLog:11 > + Right now, this lazy optimzation only applies when scripts repeatedly inserts & removes a slot element Typo: optimzation > Source/WebCore/ChangeLog:13 > + and there are assigned nodes to the slot. There is no reason to overcomplicate the slot assignemnt code typo: assignemnt
Ryosuke Niwa
Comment 7 2021-09-01 12:35:30 PDT
Will address these. Thanks! (In reply to Chris Dumez from comment #6) > Comment on attachment 437003 [details] > Fixed the crash > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437003&action=review > > > Source/WebCore/ChangeLog:8 > > + This patch makes the resolution of slot elements eager. Lazily resolution stopping making any sense > > stopping -> stopped? > > > Source/WebCore/ChangeLog:11 > > + Right now, this lazy optimzation only applies when scripts repeatedly inserts & removes a slot element > > Typo: optimzation > > > Source/WebCore/ChangeLog:13 > > + and there are assigned nodes to the slot. There is no reason to overcomplicate the slot assignemnt code > > typo: assignemnt
Ryosuke Niwa
Comment 8 2021-09-01 14:30:59 PDT
Committed r281878 (241208@main): <https://commits.webkit.org/241208@main>
Radar WebKit Bug Importer
Comment 9 2021-09-01 14:31:37 PDT
<rdar://problem/82646116>
Ryosuke Niwa
Comment 10 2021-09-07 18:20:01 PDT
(In reply to Ryosuke Niwa from comment #3) > Comment on attachment 436980 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=436980&action=review > > > Source/WebCore/dom/SlotAssignment.cpp:-125 > > - if (!m_slotAssignmentsIsValid) > > - assignSlots(shadowRoot); > > FYI, this code was totally unnecessary. hasAssignedNodes calls it internally > where it's needed > In all other cases hasAssignedNodes is not called, we're just waiting CPU > cycle traversing through nodes. This was totally wrong LOL.
Ryosuke Niwa
Comment 11 2021-09-07 18:21:08 PDT
Committed r282120 (241417@main): <https://commits.webkit.org/241417@main>
Note You need to log in before you can comment on or make changes to this bug.