1301859 - A few minor restyling optimizations

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

I was looking at [1], and noticed a few performance issues that could be affecting this.

These patches improve the situation a bit, even though the end result is still not great, and dominated by CaptureChange time.

I got stuck at some point (not really stuck, but the changes I have to make are bigger than what I initially thought, I'll fill bugs about it).

Anyway, I thought these were still worth landing.

[1]: https://mozdevs.github.io/servo-experiments/experiments/tiles/

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Review commit: https://reviewboard.mozilla.org/r/78004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78004/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1301859: Don't bother initiating the bloom filter in RestyleElement if we know we're not doing selector matching.

Review commit: https://reviewboard.mozilla.org/r/78006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78006/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1301859: Don't do the style context and restyle dance if we know nobody can trigger new transitions.

Review commit: https://reviewboard.mozilla.org/r/78008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78008/

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regarding the third patch -- what makes us actually *apply* the transition style to the element when we skip this?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #4)

What do you mean exactly with "apply the transition style"? This is called from ElementRestyler during the restyle process, so it applies newStyleContext, if that's the question. I've also verified that transitions keep working as expected with this patch applied, could you point to a test case that would fail with that?

Comment 6

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> (In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from
> comment #4)
> 
> What do you mean exactly with "apply the transition style"? This is called
> from ElementRestyler during the restyle process, so it applies
> newStyleContext, if that's the question. I've also verified that transitions
> keep working as expected with this patch applied, could you point to a test
> case that would fail with that?

Are you sure the style doesn't just get applied on the next tick? (I think you'll need a reftest to confirm. For scripted tests we have DOMWindowUtils.advanceTimeAndRefresh to put the refresh driver under test control but I'm not sure it helps, since calling getComputedStyle etc. would trigger a style flush making the test invalid.)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> > (In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from
> > comment #4)
> > 
> > What do you mean exactly with "apply the transition style"? This is called
> > from ElementRestyler during the restyle process, so it applies
> > newStyleContext, if that's the question. I've also verified that transitions
> > keep working as expected with this patch applied, could you point to a test
> > case that would fail with that?
> 
> Are you sure the style doesn't just get applied on the next tick? (I think
> you'll need a reftest to confirm. For scripted tests we have
> DOMWindowUtils.advanceTimeAndRefresh to put the refresh driver under test
> control but I'm not sure it helps, since calling getComputedStyle etc. would
> trigger a style flush making the test invalid.)

Hmm... I think David was right (as usual, btw :-)).

I think this works, but I think only accidentally, since the CSSTransition::PlayFromStyle call ends up posting a restyle anyway (from UpdateEffects). I'll attach the callstack shortly.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Call stack of the new restyle being posted from PlayFromStyle

I think this is what ends up making my third patch work... This is pretty much a bug, since the point of PlayFromStyle is (I think) not posting style updates, right?

Comment 9

[Brian Birtles (:birtles)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Created attachment 8790137 [details]
> Call stack of the new restyle being posted from PlayFromStyle
> 
> I think this is what ends up making my third patch work... This is pretty
> much a bug, since the point of PlayFromStyle is (I think) not posting style
> updates, right?

Looks like https://hg.mozilla.org/mozilla-central/rev/86b2881f552737f3723563fc88707b69d8e6e5f8 is the culprit.

Boris tried to fix this in bug 1049975 by adding a stack class that would toggle a flag to block unnecessary restyles but it turned out to be better addressed in a separate bug (which has yet to happen).

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8789989 [details]
Bug 1301859: Don't do the style context and restyle dance if we know nobody can trigger new transitions.

https://reviewboard.mozilla.org/r/78008/#review76856

Clearing review request since I think we agree this isn't right.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

https://reviewboard.mozilla.org/r/78004/#review78292

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8789988 [details]
Bug 1301859: Don't bother initiating the bloom filter in RestyleElement if we know we're not doing selector matching.

https://reviewboard.mozilla.org/r/78006/#review78296

::: layout/base/RestyleManager.cpp:1790
(Diff revision 1)
> +  // We might need to init the bloom filter if there are undisplayed contents to
> +  // restyle.
> +  if (!mTreeMatchContext.mAncestorFilter.HasFilter()) {
> +    mTreeMatchContext.InitAncestors(aRestyleRoot);
> +  }

If we get into DoConditionallyRestyleUndisplayedDescendants, it means we had an eRestyle_SomeDescendants hint.  In that case we must already have initialized the ancestor filter, right?  (I guess that is a reason for it to diverge from DoRestyleUndisplayedDescendants, despite the comment there.)

::: layout/base/RestyleManager.cpp:3364
(Diff revision 1)
> +  if (aRestyleHint & ~eRestyle_AllHintsWithoutSelectorMatching) {
> -  treeMatchContext.InitAncestors(parent);
> +    treeMatchContext.InitAncestors(parent);
> +  }

Can we check the inverse of this, i.e. define an eRestyle_AllHintsWithSelectorMatching and then check here whether aRestyleHint contains any of those?  I think that might be clearer.

(Note also the current condition will initialize the ancestor filter for |aRestyleHint == eRestyle_ForceDescendants|, which isn't required.)

::: layout/base/RestyleManager.cpp:3434
(Diff revision 1)
> +  // We might need to init the bloom filter if there are undisplayed contents to
> +  // restyle.
> +  if (!mTreeMatchContext.mAncestorFilter.HasFilter()) {
> +    mTreeMatchContext.InitAncestors(aParent->AsElement());
> +  }

Should we be deciding based on aChildRestyleHint whether to initialize the ancestor filter here?  Otherwise it seems like we'll end up initializing the ancestor filter whenever we encounter a display:none element.

There is another problem with deciding to initialize the ancestor filter mid-way through a restyle: on ancestors we decided to skip the AutoAncestorPusher::PushAncestorAndStyleScope call, but now, after initializing the ancestor filter, we really should be popping elements off it as we go back up the tree.  But the AutoAncestorPusher doesn't need to do that.

If we only make the decision once, at the top of the restyle, whether we'll need the ancestor filter or not, we can avoid this problem.

::: layout/base/RestyleManager.cpp:3687
(Diff revision 1)
> -  if (!lists.IsDone()) {
> +  if (!lists.IsDone() && mTreeMatchContext.mAncestorFilter.HasFilter()) {
>      ancestorPusher.PushAncestorAndStyleScope(mContent);
>    }

Don't we need to add the HasFilter() check around the other PushAncestorAndStyleScope() call later in this function?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78004/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Sorry for dropping the ball on this bug Cam, I completely forgot about it.

I was benchmarking the first motionmark test for servo (see https://github.com/servo/servo/issues/15163), and when I did the same for Gecko I saw a nontrivial amount of time spent in InitAncestors (and then suddenly remembered about this patch).

It's not the same approach as before at all, should be way simpler and easier to understand/verify. This makes about 20ms of 300 that I saw in the profile go away.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c8368ab03c94cc8fe005992358cdea1e7bbf58a

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

https://reviewboard.mozilla.org/r/78004/#review108116

::: layout/base/RestyleManager.cpp:2114
(Diff revision 2)
>  
> +  // If we didn't have a filter, but we've suddenly discovered that we need to
> +  // run selector matching on our children, init the ancestor filter and scope
> +  // chain.
> +  if (!mTreeMatchContext.mAncestorFilter.HasFilter() &&
> +      !mSelectorsForDescendants.IsEmpty()) {

I don't understand why we look at mSelectorsForDescendants.  Why don't we need to check aRestyleHint to see if the current element or descendants needs selector matching, like you do in ComputeStyleChangeFor?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78004/diff/2-3/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

https://reviewboard.mozilla.org/r/78004/#review108116

> I don't understand why we look at mSelectorsForDescendants.  Why don't we need to check aRestyleHint to see if the current element or descendants needs selector matching, like you do in ComputeStyleChangeFor?

I thought that SomeDescendants was the only hint that we could possibly get at that point, but I'm presumably wrong, so I've reworked how it works.

Also this way we don't leave stale state in our walk up (which is sort of tricky because we can't guarantee to not trying push the same element twice :/).

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78004/diff/3-4/

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8789987 [details]
Bug 1301859: Don't bother initiating the bloom filter if we know we're not doing selector matching.

Per discussion a few weeks ago, I'm cancelling review on this for now, while there are other higher priority things to look at.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Let's close this, I don't think it'd be worth to rebase the patch.