1483963 - Multi-second browser jank after transition between normal and full-screen video

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Today's Nightly, Build 20180816100035, Windows 10 x64

https://perfht.ml/2OHccU6


STR is pretty easy, just go to YouTube, play a video, switch to fullscreen. Wait for the video to play full screen, then switch back.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

ServoStyleSet::ResolveServoStyleByAddingAnimation took 300ms there.

https://perfht.ml/2MTp5tK

I think this is a recent regression.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Alice, I don't suppose you have a chance to look for a regression window for this recent perf regression?

Comment 3

[Alice0775 White]

what/how browserjank?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yeah, I don't see any jank. Aaron, are you still able to reproduce this?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Yes. Build 20180821100053

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

I am a tab hoarder -- could having a large number of tabs be somehow be factoring into this?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Could be.  I hadn't noticed that the ResolveServoStyleByAddingAnimation happened on chrome window.  And it was from _layOutTitlebar in browser-tabsintitlebar.js.  We might want to stop triggering any transitions in tabbar during fullscreening or un-fullscreening.

CCing some browser folks.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Could be.  I hadn't noticed that the ResolveServoStyleByAddingAnimation
> happened on chrome window.  And it was from _layOutTitlebar in
> browser-tabsintitlebar.js.  We might want to stop triggering any transitions
> in tabbar during fullscreening or un-fullscreening.

Hmm, _layOutTitlebar is called from a callback of fullscreen event, and the fullscreen event is dispatched before styling.  Should we use promiseDocumentFlushed there?  :mconley?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I think that synchronous flush is there to avoid this sort of bug:

https://bugzilla.mozilla.org/attachment.cgi?id=8620954

(from bug 1173768).

I suspect that wrapping that TabsInTitlebar.update inside a pDF would result in momentary glitchiness in the UI.

Coincidentally, I'm looking for ways to move more of the TabsInTitlebar layout calculations out of JS and express them as CSS in bug 1356920, but that bug is in its infancy.

We should probably try to find out why this got worse recently.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #9)
> I think that synchronous flush is there to avoid this sort of bug:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8620954
> 
> (from bug 1173768).
> 
> I suspect that wrapping that TabsInTitlebar.update inside a pDF would result
> in momentary glitchiness in the UI.
> 
> Coincidentally, I'm looking for ways to move more of the TabsInTitlebar
> layout calculations out of JS and express them as CSS in bug 1356920, but
> that bug is in its infancy.
> 
> We should probably try to find out why this got worse recently.

Indeed.  Note that your comment reminded me that we freezes refresh driver (which means that most events are not dispatched, and style/layout/paint triggered by the refresh driver's tick are not processed) during fullscreen process.  So there might be a bunch of pending events/styles/layouts/paints anyway.

Comment 11

[David Durst [:ddurst]]

If you're not going to be taking this, who should we redirect it to?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

It's not yet clear to us whether this is a recent regression or not.
Aaron, can you please take time to make sure whether this is a recent regression or not?  And if it's a regression it would be nice to narrow down it. I think you are the only person to reproduce this.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

I'll try...

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

According to mozregression, the bad commit is either:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca936adbc565
or
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f6cc47313d

But because both commits were part of the same push, that's the best I can give you.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thank you Aaron! That's a great help!

Olli, your change (I guess it's bug 1469521) seems to make the performance of ensure_child() worse.  Can you please take a look?  In the profile in comment 1, we spend 300ms in ensure_child() and I can see a prev_sibling call inside a loop in the function.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

That doesn't really make sense. `ensure_child` is rule tree stuff, the previous sibling there is not related to DOM nodes whatsoever.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Chances are that that change _does_ cause the difference, specially based on comment 6 & co, but it's very unlikely that it's because of those 300ms in ensure_child...

Comment 18

[Emilio Cobos Álvarez (:emilio)]

So bug 1469521 _is_ the right regression range, but not because of the style system stuff mentioned in comment 15. Most of the time spent in the profile in comment 0 is in CompareDocumentPosition, which is an operation which bug 1469521 did obviously regress.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

In particular, most of the "self" time is being consumed by nsINode::ComputeIndexOf.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Thank you, Emilio, as usual. :)  (I was about to do NI to you in comment 15, didn't know you were in CC list in that bug.)

Moving into Core::DOM.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

So this is AnimationEventDispatcher::DispatchEvents taking a long time to sort the events... That can only happen when we have a bunch of transitions on siblings. So I'd consider to keep this as a DOM: Animation bug for now.

Looks like we're firing a transition per tab and we're struggling there, should be pretty easy to construct a test-case that demonstrates the issue.

So, possible fixes:

 * Fix the front-end so that leaving full-screen doesn't trigger a gazillion transitions on each tab. This is the less desirable fix, but also probably easier?

 * Fix it so that we reduce calls to CompareDocumentPosition by walking the sibling list until we see the other sibling instead of computing two indices. That should reduce the work in half, maybe. That may be worth doing.

 * Can we fix the transitions stuff to be smarter as well? Chances are that in most cases the array is already sorted, can we detect that and avoid the work of sorting it and doing all this expensive work?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase.

It indeed was easy to come up with a test-case for this.

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

[Tracking Requested - why for this release]: Perf regression

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)
>  * Fix the front-end so that leaving full-screen doesn't trigger a gazillion
> transitions on each tab. This is the less desirable fix, but also probably
> easier?

Note that entering full-screen is also affected.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

NI to me to figure out a way to make the animation events sort issue better.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)
>  * Can we fix the transitions stuff to be smarter as well? Chances are that
> in most cases the array is already sorted, can we detect that and avoid the
> work of sorting it and doing all this expensive work?

In the test case in comment 22, the events are a bunch of chunks of ordered events. E.g. '1 2 3 4 5', '10 11 12 13', etc.  It is due to we traverse those chunk of elements on different threads, I guess.  I wonder each element can have an index for DOM tree order during traversal, and use the index for event sorting?  The other idea I can think of is similar to what Emilio mentioned in comment 21.  We can check both elements have the same parent in nsINode::CompareDocumentPosition in this particular test case.

Keep NI to me.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Looks like OwningElementRef or some such should definitely cache the index during sorting, and then we could have a version of nsContentUtils::PositionIsBefore which takes nodes and their index in parent as params.
That should reduce ComputeIndexOf call quite a bit.

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Marking this significant perf regression as P1. It sounds like we can fix it on the Animation side (presumably we think the regression to CompareDocumentPosition from bug 1469521 is acceptable).

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

I guess I could add back some kind of cache for ComputeIndexOf

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Let's see what tryserver thinks of these. 
- A cache for ComputeIndexOf
- reorder member variables of Animation a bit to use less memory
- cache child index in Animation when reordering

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/86900f210d1f2eb640397735e6525e8f36622dbd
remote:   https://hg.mozilla.org/try/rev/30f5228c4a0b2a9792ef820edde905a47dea22d5
remote:   https://hg.mozilla.org/try/rev/6768f8c1b1a2318b5606d355a29ef7aeae6c54c7
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6768f8c1b1a2318b5606d355a29ef7aeae6c54c7
remote: recorded changegroup in replication log in 0.013s

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Attachment: cache_index_in_animation.diff

this is perhaps not the most beautiful, but we basically must cache the index here to get good performance.
Other thing to fix is bug 1488278 - I'll upload a patch there in a minute.

ehsan, could you take a look at the dom/base/* and hiro for the rest

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9006089 [details] [diff] [review]
cache_index_in_animation.diff

Review of attachment 9006089 [details] [diff] [review]:
-----------------------------------------------------------------

Animation part looks good to me.
Thanks for fixing this!!

Comment 33

[(no longer active)]

Comment on attachment 9006089 [details] [diff] [review]
cache_index_in_animation.diff

Review of attachment 9006089 [details] [diff] [review]:
-----------------------------------------------------------------

Please nominate for uplift if needed.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a0dbf6d6e9
cache the index of a child node when ordering animations for event dispatch, r=hiro,ehsan

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9006089 [details] [diff] [review]
cache_index_in_animation.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 651120
[User impact if declined]: Jank
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: Landed to m-i yesterday, tested locally
[Needs manual test from QE? If yes, steps to reproduce]: I'd say no
[List of other uplifts needed for the feature/fix]: bug 1488279 and bug 1488278
[Is the change risky?]: The change shouldn't be risky 
[Why is the change risky/not risky?]: just caching a return value of a method call temporarily 
[String changes made/needed]: NA

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a2a0dbf6d6e9

Comment 37

[(No longer employed by Mozilla) Aaron Klotz]

This is significantly better than before, but I am still setting ~400ms jank after leaving full-screen mode.

Is this just something that we have to live with when there are high numbers of open tabs?
https://perfht.ml/2oGKWtU

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

I was using the testcase for profiling, since I can't reproduce the issue using full-screen.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

But I'd say no, we don't have to live with it. If there is some testcase for profiling, we could probably improve things. Not yet sure how though.

Comment 40

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9006089 [details] [diff] [review]
cache_index_in_animation.diff

Review of attachment 9006089 [details] [diff] [review]:
-----------------------------------------------------------------

Noticabmle perf regression, approved for 63 beta

Comment 41

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b236230b856