WIP: Move StyleEngine::m_activeTreeScopes to TreeScope::m_childTreeScopesWithActiveStyleSheets

Move StyleEngine::m_activeTreeScopes to TreeScope::m_childTreeScopesWithActiveStyleSheets

*** SUSPENDED FOR FURTHER INVESTIGATIONS, NOT FOR LANDING ***

StyleEngine used to keep all TreeScopes that have StyleSheetCollections
in the document in the document order. When considering 1k or more
treeScopes in a document, sorting treeScopes in the document order becomes
more expensive and this sorting alone tends to consume 2-3% of total time
to build the DOM tree.

This patch moves the document-global ordered list to a list of direct
children in each TreeScope. Assuming shadow trees are usually nesteed at
some levels, this change helps faster determination of the document order
of TreeScopes.

It should also help to traverse descendant TreeScopes, which will be
needed as we move more to componentised StyleResolver.

Two ways to traverse TreeScopes with active StyleSheetCollections are
provided in this patch.

TreeScopesWithActiveStyleSheetsTraversal(TreeScope&) iterates the TreeScope
and its descendant TreeScopes that have active StyleSheetCollections.

Pass treeScope.childTreeScopesWithActiveStyleSheets() instead to iterate
only its descendants, excluding the treeScope.

BUG=401359, 433225

[kojii, 2015-05-22 07:34:58]

kojii@chromium.org changed reviewers:
+ esprehn@chromium.org, hayato@chromium.org, rune@opera.com, tasak@chromium.org

[kojii, 2015-05-22 07:34:59]

PTAL.

[kojii, 2015-05-22 07:37:19]

kojii@chromium.org changed reviewers:
+ tasak@google.com
- tasak@chromium.org

[kochi, 2015-05-26 01:52:33]

kochi@chromium.org changed reviewers:
+ kochi@chromium.org

[kochi, 2015-05-26 01:52:35]

Sorry for the delay.
This is my first round of comments.  More to follow.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (left):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:486: document().clearScopedStyleResolver();
Where did this go?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:308: if
(treeScope->isInActiveStyleSheetsTraversal() &&
!treeScope->hasChildTreeScopesWithActiveStyleSheets())
What is this addition?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:338:
removed->removeFromActiveStyleSheetsTraversal();
what removed points after returning from the function?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.h (left):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.h:84: const
WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet>>
activeStyleSheetsForInspector() const;
Why unconst'ing is necessary here?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.h:194: TreeScopeStyleSheetCollection*
styleSheetCollectionFor(TreeScope&);
Please append this declaration to public: section and avoid switching
private to public this way.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:724: void
TreeScope::insertToActiveStyleSheetsTraversal()
nit: insertToActiveStyleSheetsTraversal() sounds a bit strange,
as *Traversal means traversing a tree-like structure.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:732: treeScope->m_inActiveStyleSheetsTraversal =
true;
nit: maybe m_hasActiveStyleSheets?  This also sounds a bit weird to call this
"in...Traversal" to me.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:741: void
TreeScope::removeFromActiveStyleSheetsTraversal()
nit: ditto as insertToActiveStyleSheetsTraversal()

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:214: iterator end() { return iterator(*this); }
Why begin() and end() are same?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:289: bool m_inActiveStyleSheetsTraversal : 1;
Not necessary to be a bitfield for one boolean.

[kojii, 2015-05-26 05:21:58]

Thank you for the review. Comments follow.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (left):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:486: document().clearScopedStyleResolver();
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> Where did this go?

The loop below traverses document() and its descendants, so we no longer need to
call document() separately and then m_activeTreeScopes.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:308: if
(treeScope->isInActiveStyleSheetsTraversal() &&
!treeScope->hasChildTreeScopesWithActiveStyleSheets())
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> What is this addition?

Before the patch, removing treeScopes that are not in m_activeTreeScopes is
simply ignored.

With this patch, TreeScope::removeFromActiveStyleSheetsTraversal has
ASSERT(m_inActiveStyleSheetsTraversal). Another change is that treeScopes
without active stylesheet but has children with active stylesheet need to be
included.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:338:
removed->removeFromActiveStyleSheetsTraversal();
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> what removed points after returning from the function?

This does not destruct the pointer, just remove from the traversal list. Along
with another feedback from you, I've renamed the method to make it clearer.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.h (left):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.h:84: const
WillBeHeapVector<RefPtrWillBeMember<CSSStyleSheet>>
activeStyleSheetsForInspector() const;
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> Why unconst'ing is necessary here?

Original code was using const_cast(), and this patch requires the use of
const_cast() more (such as styleSheetCollectionFor()), so I chose to get rid of
the const. We could revise that when styleSheetColectionFor() and its related
staff were cleaned up, but fixing it looked too out of scope for this patch to
me.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.h:194: TreeScopeStyleSheetCollection*
styleSheetCollectionFor(TreeScope&);
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> Please append this declaration to public: section and avoid switching
> private to public this way.

Done.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:724: void
TreeScope::insertToActiveStyleSheetsTraversal()
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> nit: insertToActiveStyleSheetsTraversal() sounds a bit strange,
> as *Traversal means traversing a tree-like structure.

Changed to setSelfOrDescendantsHaveActiveStyleSheets(). Does this sound better?

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:732: treeScope->m_inActiveStyleSheetsTraversal =
true;
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> nit: maybe m_hasActiveStyleSheets?  This also sounds a bit weird to call this
> "in...Traversal" to me.

Changed to m_selfOrDescendantsHaveActiveStyleSheets. This flag is set when
descendants have active stylesheets, even if |this| does not.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:741: void
TreeScope::removeFromActiveStyleSheetsTraversal()
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> nit: ditto as insertToActiveStyleSheetsTraversal()

Done.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:214: iterator end() { return iterator(*this); }
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> Why begin() and end() are same?

end() is not really used, as operator!=() can determine by itself, but I agree
that this is confusing. Fixed.

https://codereview.chromium.org/1131493008/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:289: bool m_inActiveStyleSheetsTraversal : 1;
On 2015/05/26 at 01:52:34, Takayoshi Kochi wrote:
> Not necessary to be a bitfield for one boolean.

Done.

[esprehn, 2015-05-26 05:31:24]

I'll look over this tomorrow.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rune, 2015-05-26 12:23:54]

Looking a bit into this. Comments so far:

We currently need to keep an ordered list of tree-scopes for
TreeBoundaryCrossingRules. Is that the only reason?

If so, and we land a variant of [1], TreeScope parent/child relationship in the
composed tree will give the right order-of-appearance without the need for a
sorted list of active tree-scopes at all.

I'm unsure to what degree this CL actually improves performance for typical web
component sites. If you look at component-heavy sites like polymer-project.org,
there are quite a bit of custom elements which are descendants of other custom
elements in the document scope. There are not necessarily many levels of
tree-scopes, but instead descendant components are distributed into each other's
shadow trees. That will give you a shallow tree of tree-scopes with a lot of
tree-scopes that will need to be ordered within the document scope.

[1] https://codereview.chromium.org/1134173002/

[kojii, 2015-05-27 02:15:27]

Thanks for your comments.

On 2015/05/26 12:23:54, rune wrote:
> Looking a bit into this. Comments so far:
> 
> We currently need to keep an ordered list of tree-scopes for
> TreeBoundaryCrossingRules. Is that the only reason?
> 
> If so, and we land a variant of [1], TreeScope parent/child relationship in
the
> composed tree will give the right order-of-appearance without the need for a
> sorted list of active tree-scopes at all.

Hm, good point.

Inspector needs the sorted list, though I'm not familiar with the Inspector code
if it can get rid of that.

Other cases, we're using the ordered version in several places such as
collectFeatures(), but I haven't really verified that these doesn't need the
list sorted. I'll investigate if we can stop sorting at all.

> I'm unsure to what degree this CL actually improves performance for typical
web
> component sites. If you look at component-heavy sites like
http://polymer-project.org,
> there are quite a bit of custom elements which are descendants of other custom
> elements in the document scope. There are not necessarily many levels of
> tree-scopes, but instead descendant components are distributed into each
other's
> shadow trees. That will give you a shallow tree of tree-scopes with a lot of
> tree-scopes that will need to be ordered within the document scope.
> 
> [1] https://codereview.chromium.org/1134173002/

<core-style>[1] allows sharing styles among components, so if a component uses
<core-style> and its own style, it will have two <style> elements. Even when the
nested level is 1, this CL makes inserting <style>s to shadow trees cheaper
(though if we don't need to sort at all, we're not saving much...)

[1] https://www.polymer-project.org/0.5/docs/elements/core-style.html

[kojii, 2015-05-27 03:40:53]

Briefly talked with tasak@, he agreed that we could get rid of the sorting at
all.

I'll investigate that way, and suspend this CL until then.

[rune, 2015-05-27 11:00:18]

On 2015/05/27 02:15:27, koji wrote:

> Inspector needs the sorted list, though I'm not familiar with the Inspector
code
> if it can get rid of that.
> 
> Other cases, we're using the ordered version in several places such as
> collectFeatures(), but I haven't really verified that these doesn't need the
> list sorted. I'll investigate if we can stop sorting at all.

I doubt we rely on sorted tree-scopes in collectFeatures(). Also, most features
are going to be per scope, so accumulating features will mostly go away.

[kojii, 2015-06-09 03:32:15]

Closing this CL as I reached a consensus to get rid of the sorting with
Inspector team.