Bug 103212 - [Shadow] ShadowRoot should cache InsertionPoint list
Summary: [Shadow] ShadowRoot should cache InsertionPoint list
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 103016
  Show dependency treegraph
 
Reported: 2012-11-25 20:11 PST by Shinya Kawanaka
Modified: 2012-11-28 22:24 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 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.
Comment 1 Shinya Kawanaka 2012-11-25 21:29:18 PST
Created attachment 175920 [details]
Patch
Comment 2 Hajime Morrita 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.
Comment 3 Shinya Kawanaka 2012-11-26 02:50:58 PST
Created attachment 175952 [details]
Patch
Comment 4 Shinya Kawanaka 2012-11-26 04:02:44 PST
Created attachment 175964 [details]
Patch
Comment 5 Shinya Kawanaka 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.
Comment 6 Dimitri Glazkov (Google) 2012-11-28 09:01:15 PST
Comment on attachment 175964 [details]
Patch

This is great!
Comment 7 Shinya Kawanaka 2012-11-28 19:56:42 PST
Created attachment 176630 [details]
Patch
Comment 8 Hajime Morrita 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.
Comment 9 Shinya Kawanaka 2012-11-28 21:10:29 PST
Created attachment 176637 [details]
Patch
Comment 10 Shinya Kawanaka 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.
Comment 11 Hajime Morrita 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.
Comment 12 WebKit Review Bot 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
Comment 13 Early Warning System Bot 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
Comment 14 Early Warning System Bot 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
Comment 15 Build Bot 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
Comment 16 Shinya Kawanaka 2012-11-28 21:43:50 PST
Created attachment 176645 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-11-28 22:24:48 PST
Comment on attachment 176645 [details]
Patch for landing

Clearing flags on attachment: 176645

Committed r136098: <http://trac.webkit.org/changeset/136098>
Comment 18 WebKit Review Bot 2012-11-28 22:24:52 PST
All reviewed patches have been landed.  Closing bug.