1301194 - high cpu use in moz_gtk_get_scrollbar_metrics()/gtk_style_context_get()/gtk_style_context_get_style()

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Karl Tomlinson (:karlt)]

I received a report of slow performance opening and closing the menu ("Show
menu", top-right) on a trello.com board.

The page needs an account to view, but it includes several scrollbars and the
menu position animates as it slides open and closed.  The animation is about three times slower on Nightly than on 48.0b1, taking 3-4 seconds to open and similar to close.

Two thirds of cpu use is under moz_gtk_get_scrollbar_metrics(), with that
shared between gtk_style_context_get() and gtk_style_context_get_style()

This is most noticeable with GTK 3.20, but is also slow with 3.18.

Comment 1

[Karl Tomlinson (:karlt)]

The performance became significantly worse after
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de4044b57f4

In https://hg.mozilla.org/mozilla-central/rev/53e791a65fa2
I attempted to avoid changing the state unnecessarily, but failed to take into
account the direction flags in the state in newer versions of GTK, and so
oldState was (almost?) always different from aStateFlags.

Comment 2

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dbf2e927a4e

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1301194 be more careful to only invalidate style context when state changes

Previously oldState differed from aStateFlags just because it contained
direction flags.

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

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1301194 don't modify the direction when fetching a cached style context if no direction is explicitly requested

This is important for RTL locales which are drawn with explicit direction but
measured with no direction.

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

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8789062 [details]
bug 1301194 be more careful to only invalidate style context when state changes

https://reviewboard.mozilla.org/r/77322/#review76896

Looks fine.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8789063 [details]
bug 1301194 don't modify the direction when fetching a cached style context if no direction is explicitly requested

https://reviewboard.mozilla.org/r/77324/#review76898

Looks okay.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba1e87c0e0a6
be more careful to only invalidate style context when state changes r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/ba9818c39954
don't modify the direction when fetching a cached style context if no direction is explicitly requested r=stransky+263117

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba1e87c0e0a6
https://hg.mozilla.org/mozilla-central/rev/ba9818c39954

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Karl, do you think it's worthwhile uplifting this patch to Beta50? I noticed this one as it's tagged as a P1 regression.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8789062 [details]
bug 1301194 be more careful to only invalidate style context when state changes

Approval Request Comment

(In reply to Ritu Kothari (:ritu) from comment #9)
> Hello Karl, do you think it's worthwhile uplifting this patch to Beta50? I
> noticed this one as it's tagged as a P1 regression.

Yes, this can be a very noticeable regression in performance, and so I think this worth having on Beta50, thanks.

[Feature/regressing bug #]:
Bug 1271579.
[User impact if declined]:
Impaired performance on sites using animations with native widgets.
The common case is scrollbars, which animate when scrolling.
[Describe test coverage new/current, TreeHerder]:
Tested only manually.
[Risks and why]: 
This is a reasonably simple change to code affecting native widgets on Linux.
The code is complicated slightly by the ABI change in GTK+ 3.8.
Still, it is a lower risk patch.
Reverting the change from bug 1271579 is not a practical option because of the number of subsequent dependencies on that change.
[String/UUID change made/needed]:
None.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8789063 [details]
bug 1301194 don't modify the direction when fetching a cached style context if no direction is explicitly requested

Approval Request Comment
[Feature/regressing bug #]:
This patch is fixing a similar issue, involving the same code, which existed before changes for bug 1271579, though not before the change to GTK3.
[User impact if declined]:
Impaired performance with RTL layout, though to a lesser extent than reported on trello.com.
It is not necessary to uplift this patch because the first patch addresses the part that was a recent regression, which was the worst of the problem.
Reasons to consider uplift are the demonstration that such issues can be significant in performance, and to have similar code on beta as has been tested on m-c.
[Describe test coverage new/current, TreeHerder]:
manual testing only.
[Risks and why]: low.
[String/UUID change made/needed]: none.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8789062 [details]
bug 1301194 be more careful to only invalidate style context when state changes

Fixes a perf regression, Beta50+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5df0eb2f0f02
https://hg.mozilla.org/releases/mozilla-beta/rev/769342995889