Get rid of TreeBoundaryCrossingRules.

Get rid of TreeBoundaryCrossingRules.

*** NOT FOR LANDING - WORK IN PROGRESS ***

- Another attempt at walking the composed tree ancestors for matching
  boundary-crossing rules instead of collecting from all scoped resolvers
  with boundary-crossing rules.

- Prepare for non-global RuleFeatureSets, even for ::content by supporting
  cheaper traversal of ancestor tree-scopes by caching the composed tree
  TreeScope parent on TreeScope.

- Fixes shadow dom cascading issues as a side-effect.

BUG=401359, 452542, 437846

[rune, 2015-05-13 11:09:35]

Ran some performance tests locally.

blink_perf.css:

No noticable differences


blink_perf.shadow_dom:

- Consistenly around ~20% improvement on
blink_perf.shadow_dom:ChangingClassNameShadowDOM
- Several tests have an improvement that varies between ~1.5% to ~4% over the
runs.
- None consistently worse.
- Some tests have way too high variance to tell anything.

blink_perf.dom:

- Pretty much unchanged apart from some wild variance, both positive/negative.

[rune, 2015-05-13 11:16:55]

On 2015/05/13 11:09:35, rune wrote:
> Ran some performance tests locally.

blink_perf.events:

2-3% worse for blink_perf.events:EventsDispatching

[dglazkov, 2015-05-13 16:36:54]

On 2015/05/13 at 11:16:55, rune wrote:
> On 2015/05/13 11:09:35, rune wrote:
> > Ran some performance tests locally.
> 
> blink_perf.events:
> 
> 2-3% worse for blink_perf.events:EventsDispatching

I am curious -- how is that possible? The code doesn't seem to touch event
dispatch parts.

[rune, 2015-05-15 13:08:48]

Patchset #3 (id:40001) has been deleted

[rune, 2015-05-15 22:53:13]

On 2015/05/13 16:36:54, dglazkov wrote:
> On 2015/05/13 at 11:16:55, rune wrote:
> > On 2015/05/13 11:09:35, rune wrote:
> > > Ran some performance tests locally.
> > 
> > blink_perf.events:
> > 
> > 2-3% worse for blink_perf.events:EventsDispatching
> 
> I am curious -- how is that possible? The code doesn't seem to touch event
> dispatch parts.

I haven't done any profiling, but I think creating the event path calls into
TreeScope, and I added some code to TreeScope.cpp. Could be that the test is
sensitive to just moving code around.

[esprehn, 2015-05-19 02:37:55]

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

[esprehn, 2015-05-19 02:37:56]

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

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:581: m_composedParent =
treeScopeInComposedTree(*host);
Can we update these pointers inside the updateDistribution phase instead? I'd
rather that and make the getter inline and do no branching.

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:589: if (composedParent &&
isShadowHost(composedParent))
This null check doesn't make sense, you're in a loop that already checked it

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:590: return
toElement(composedParent)->shadow()->youngestShadowRoot();
there's a getter right on Node for this, just do
composedParent->youngestShadowRoot().

[rune, 2015-05-19 11:08:18]

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

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:581: m_composedParent =
treeScopeInComposedTree(*host);
On 2015/05/19 02:37:56, esprehn wrote:
> Can we update these pointers inside the updateDistribution phase instead? I'd
> rather that and make the getter inline and do no branching.

Maybe, I need to look closer at the distribution code. We probably do update
distribution in tree-of-trees order which possibly makes it more costly, as we
would need to traverse down the composed tree finding the tree-scopes that needs
composedParent updates. Also, we might not want to update composedParent
pointers in display:none subtrees (we don't know the display type at
distribution time), yet we need to find the composedParent if we query for
computed style inside a display:none sub-tree. So, there are various concerns
here, and I don't have the full overview yet.

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:589: if (composedParent &&
isShadowHost(composedParent))
On 2015/05/19 02:37:56, esprehn wrote:
> This null check doesn't make sense, you're in a loop that already checked it

Done.

https://codereview.chromium.org/1134173002/diff/20001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:590: return
toElement(composedParent)->shadow()->youngestShadowRoot();
On 2015/05/19 02:37:56, esprehn wrote:
> there's a getter right on Node for this, just do
> composedParent->youngestShadowRoot().

Done.

[esprehn, 2015-05-20 00:38:18]

The distribution phase is where we compute what ::parent() is going to return,
I'm pretty sure you can just assign these pointers inside there.

[rune, 2015-05-20 07:51:45]

On 2015/05/20 00:38:18, esprehn wrote:
> The distribution phase is where we compute what ::parent() is going to return,
> I'm pretty sure you can just assign these pointers inside there.

No, you'd have to do a lot of traversal to find the scopes who needs a composed
parent update.

Given this tree:

<div id=host>
  <:shadowroot>
    <content select="#distributed" />
  </:shadowroot>
  <div id=distributed>
    <div id=container>
      <div></div>
      ...
      <div></div>
      <div id=innerHost>
        <:shadowroot>
          <content/>
        </:shadowroot>
        <div id=innerDistributed></div>
      </div>
    </div>
  </div>
  <div id="non-distributed">
    <:shadowroot>
      <content/>
    </:shadowroot>
  </div>
</div>

If the shadow for id=host is re-distributed (because we create a shadowroot for
it), the composed tree parent scope for the inner shadow root needs to be
updated. The distribution update/recalc will stop at the shadow of id=host, but
we need to do a composed traversal of the subtree looking for shadow hosts that
contain shadow roots that will have the outer shadow root as its composed
parent.

With the current code in this CL, we do that update lazily traversing the
composed path upwards instead from the inner shadow root instead of traversing
the full id=host subtree looking for descendant hosts.

I've tried doing it as part of the distribution, but doing a composed traversal
for a clean (in terms of distribution) sub-tree causes asserts in
ComposedTreeTraversal which expects the full tree to be clean.

Another side to this is that we do distribute in display:none subtrees, since we
don't know if they will be display:none until styles are applied to the new
distribution. If we lazily query parent tree-scope in the composed tree, we
avoid updating composed parents in display:none sub-trees. We might need the
composed tree parent scopes from display:none sub-trees if we query computed
style on nodes inside such sub-trees, though. Lazily computing the composed
parent scope will handle that.

[esprehn, 2015-05-20 09:58:47]

This patch is not correct though, we need to know what rules could match you
without ever doing a recalc and having only done a distribution. Specifically
you cannot use ShadowRoot::detach to clear pointers like this. We shouldn't be
prematurely optimizing for display: none content.

We need to invalidate these pointers in recalcDistribution, or in a new phase at
the end of that tree walk.

[rune, 2015-05-20 10:54:48]

On 2015/05/20 09:58:47, esprehn wrote:
> This patch is not correct though, we need to know what rules could match you
> without ever doing a recalc and having only done a distribution. Specifically
> you cannot use ShadowRoot::detach to clear pointers like this. We shouldn't be
> prematurely optimizing for display: none content.

If you're talking about style invalidation, I think it'll currently work because
a change in distribution will detach everything that'll have invalid composed
tree parent scopes and everything will have a recalc. I do agree that
invalidation of these parent pointers in detach is not ideal. It also means that
if we detach due to a display:none in an ascendant, we will invalidate the
parent pointer unnecessarily.

> We need to invalidate these pointers in recalcDistribution, or in a new phase
at
> the end of that tree walk.

Doing it as part of recalcDistribution when there are distribution-dirty shadow
trees sounds scary. Afterwards sounds more safe.

[esprehn, 2015-05-20 11:06:43]

On 2015/05/20 at 10:54:48, rune wrote:
> On 2015/05/20 09:58:47, esprehn wrote:
> > This patch is not correct though, we need to know what rules could match you
> > without ever doing a recalc and having only done a distribution.
Specifically
> > you cannot use ShadowRoot::detach to clear pointers like this. We shouldn't
be
> > prematurely optimizing for display: none content.
> 
> If you're talking about style invalidation, I think it'll currently work
because a change in distribution will detach everything that'll have invalid
composed tree parent scopes and everything will have a recalc. I do agree that
invalidation of these parent pointers in detach is not ideal. It also means that
if we detach due to a display:none in an ascendant, we will invalidate the
parent pointer unnecessarily.

It's not just invalidation, also getMatchedCSSRules, the inspector, any other
system that wants look at rules... It's not at all clear from the contract that
composedParent() is only valid after, and maybe in the middle of, a style
recalc.

[esprehn, 2015-05-20 11:36:44]

Let me think about this some more tomorrow. Only ::content rules need the
complex version of this, deep and shadow rules can just walk scopes. I
wonder how common ::content is and if we make that path slower for a
simpler common case.

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

[rune, 2015-05-20 12:03:07]

On 2015/05/20 11:36:44, esprehn wrote:
> Let me think about this some more tomorrow. Only ::content rules need the
> complex version of this, deep and shadow rules can just walk scopes. I
> wonder how common ::content is and if we make that path slower for a
> simpler common case.

Looking at polymer, it looks quite common.

I think a common use case is to style sub-components. <custom-list> below could
set a border on its <list-item>s below by using <style>::content list-item {
border:1px solid black}</style><content></content> in its shadow tree.

<custom-list>
  <list-item></list-item>
  <list-item></list-item>
</custom-list>

[rune, 2015-05-31 23:37:21]

This is still experimental - work in progress.

[rune, 2016-01-17 23:30:39]

Description was changed from

==========
Get rid of TreeBoundaryCrossingRules.

*** NOT FOR LANDING - WORK IN PROGRESS ***

- Another attempt at walking the composed tree ancestors for matching
  boundary-crossing rules instead of collecting from all scoped resolvers
  with boundary-crossing rules.

- Prepare for non-global RuleFeatureSets, even for ::content by supporting
  cheaper traversal of ancestor tree-scopes by caching the composed tree
  TreeScope parent on TreeScope.

- Fixes shadow dom cascading issues as a side-effect.

BUG=401359,452542,437846
==========

to

==========
Get rid of TreeBoundaryCrossingRules.

*** NOT FOR LANDING - WORK IN PROGRESS ***

- Another attempt at walking the composed tree ancestors for matching
  boundary-crossing rules instead of collecting from all scoped resolvers
  with boundary-crossing rules.

- Prepare for non-global RuleFeatureSets, even for ::content by supporting
  cheaper traversal of ancestor tree-scopes by caching the composed tree
  TreeScope parent on TreeScope.

- Fixes shadow dom cascading issues as a side-effect.

BUG=401359,452542,437846
==========