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

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({perf, regression})

49 Branch
mozilla51
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 wontfix, firefox50 fixed, firefox51 fixed)

Details

Attachments

(2 attachments)

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.
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.
Blocks: 1271579
See Also: → 1301305
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.
Attachment #8789062 - Flags: review?(stransky) → review+
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.
Attachment #8789063 - Flags: review?(stransky) → review+
Pushed by ktomlinson@mozilla.com:
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
See Also: → 1303649
https://hg.mozilla.org/mozilla-central/rev/ba1e87c0e0a6
https://hg.mozilla.org/mozilla-central/rev/ba9818c39954
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Flags: needinfo?(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.
Flags: needinfo?(karlt)
Attachment #8789062 - Flags: approval-mozilla-beta?
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.
Attachment #8789063 - Flags: approval-mozilla-beta?
Comment on attachment 8789062 [details]
bug 1301194 be more careful to only invalidate style context when state changes

Fixes a perf regression, Beta50+
Attachment #8789062 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8789063 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1315668
You need to log in before you can comment on or make changes to this bug.