Bug 103212

Summary:

[Shadow] ShadowRoot should cache InsertionPoint list

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

DOM

Assignee:

Shinya Kawanaka <shinyak>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, morrita, ojan, philn, webcomponents-bugzilla, webkit-ews, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

103016

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Shinya Kawanaka

Reported 2012-11-25 20:11:29 PST

When distributing, we're currently traversing all descendant nodes in ShadowDOM. If we cache InsertionPoint List, we can skip a lot of traversal.

Attachments
Patch (8.78 KB, patch) 2012-11-25 21:29 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (8.37 KB, patch) 2012-11-26 02:50 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (8.35 KB, patch) 2012-11-26 04:02 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (9.60 KB, patch) 2012-11-28 19:56 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (9.16 KB, patch) 2012-11-28 21:10 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch for landing (9.39 KB, patch) 2012-11-28 21:43 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-11-25 21:29:18 PST

Created attachment 175920 [details] Patch

Hajime Morrita

Comment 2 2012-11-26 00:44:30 PST

Comment on attachment 175920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175920&action=review > Source/WebCore/dom/ShadowRoot.h:106 > + bool hasValidDescendentInsertionPointList() const; s/descendent/descendant/ I think it can be simply called insertionPoints or insertionPointList. > Source/WebCore/dom/ShadowRoot.h:191 > + ASSERT(hasValidDescendentInsertionPointList()); We can ensure() the list here considering that all call sites calls ensure() before calling this.

Shinya Kawanaka

Comment 3 2012-11-26 02:50:58 PST

Created attachment 175952 [details] Patch

Shinya Kawanaka

Comment 4 2012-11-26 04:02:44 PST

Created attachment 175964 [details] Patch

Shinya Kawanaka

Comment 5 2012-11-28 00:49:24 PST

Comment on attachment 175964 [details] Patch This patch cannot be applied to ToT. We have to resolve Bug 103481 at first.

Dimitri Glazkov (Google)

Comment 6 2012-11-28 09:01:15 PST

Comment on attachment 175964 [details] Patch This is great!

Shinya Kawanaka

Comment 7 2012-11-28 19:56:42 PST

Created attachment 176630 [details] Patch

Hajime Morrita

Comment 8 2012-11-28 20:25:08 PST

Comment on attachment 176630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176630&action=review If you have a microbenchmark for this change, let's commit them first and mention its improvement on the ChangeLog. It would be good for the record. > Source/WebCore/dom/ShadowRoot.cpp:320 > + distributionData()->setInsertionPointListValidity(true); Let's move the implementation to DistributionData. > Source/WebCore/dom/ShadowRoot.cpp:350 > + distributionData()->clearInsertionPointList(); Ditto.

Shinya Kawanaka

Comment 9 2012-11-28 21:10:29 PST

Created attachment 176637 [details] Patch

Shinya Kawanaka

Comment 10 2012-11-28 21:10:50 PST

(In reply to comment #8) > (From update of attachment 176630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176630&action=review > > If you have a microbenchmark for this change, let's commit them first and mention its improvement on the ChangeLog. > It would be good for the record. > > > Source/WebCore/dom/ShadowRoot.cpp:320 > > + distributionData()->setInsertionPointListValidity(true); > > Let's move the implementation to DistributionData. > > > Source/WebCore/dom/ShadowRoot.cpp:350 > > + distributionData()->clearInsertionPointList(); > > Ditto. Done.

Hajime Morrita

Comment 11 2012-11-28 21:14:46 PST

Comment on attachment 176637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176637&action=review > Source/WebCore/dom/ShadowRoot.cpp:354 > + invalidateInsertionPointList(); These invalidateInsretionPointList() calls also can be in DistributionData > Source/WebCore/dom/ShadowRoot.h:133 > + void ensureValidInsertionPointList(); Then we can kill this.

WebKit Review Bot

Comment 12 2012-11-28 21:15:22 PST

Comment on attachment 176637 [details] Patch Attachment 176637 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15018708

Early Warning System Bot

Comment 13 2012-11-28 21:16:41 PST

Comment on attachment 176637 [details] Patch Attachment 176637 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15015715

Early Warning System Bot

Comment 14 2012-11-28 21:18:09 PST

Comment on attachment 176637 [details] Patch Attachment 176637 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15027676

Build Bot

Comment 15 2012-11-28 21:20:38 PST

Comment on attachment 176637 [details] Patch Attachment 176637 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15015719