GetThemeMargins() can be extremely expensive

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: agashlin)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
See this profile from bug 1384733: http://bit.ly/2wsAKL8

This is the code at fault: https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/widget/windows/nsNativeThemeWin.cpp#166

Not sure which one of the two calls is the expensive one.  Perhaps it's half and half.

Jim, can we cache the result of this call or something to that effect?
Flags: needinfo?(jmathies)
(Reporter)

Updated

2 years ago
Blocks: 1384733
Looks like that metric can get cached and reset when we receive a settings change events.
Flags: needinfo?(jmathies)
Sounds pretty straightforward and like a perf win; let's call this qf:p1.
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 3

2 years ago
Would it make sense to cache the result of GetGutterSize, at least for these two calls in GetMinimumWidgetSize [1]? That's where all the calls down to GetThemeMargins come from in this profile, and there we know it is just called with the theme and nullptr.

I'm worried about there being different themes for different widget types, though, so it might be necessary to do it once for each of NS_THEME_MENUITEM, CHECKMENUITEM, RADIOMENUITEM, MENUIMAGE, MENUCHECKBOX, and MENURADIO, or can we assume these all use the same theme?

[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/widget/windows/nsNativeThemeWin.cpp#2276,2288
Assignee: nobody → agashlin
Flags: needinfo?(jmathies)
(Assignee)

Comment 4

2 years ago
Here's a first pass. It's a big win with many tabs, I'm seeing a 50% drop in the total time (the red event processing delay bar), time in the second big DOMEvent popupshowing goes from 250+ to around 80ms, the big reflow goes from 200ms to 12ms, measured on a Acer Aspire E 15 reference laptop with 800 tabs or so.

I'll kick off a try run, but in the meantime can someone recommend a reviewer?
(In reply to Adam Gashlin [:agashlin] from comment #4)
> I'll kick off a try run, but in the meantime can someone recommend a
> reviewer?

Default strategy is to look at commit log for the file(s) in question (skipping over trivial/mass changes where the file was only incidentally touched), for who has made and reviewed substantial changes to the file.

That suggests jimm as the primary candidate for review here, probably.  If he can't for some reason, perhaps jfkthame or emk?
I can do the reviews.
Flags: needinfo?(jmathies)
Hard to predict if sharing that cached value will regress, I think landing what you have with the single cached value is worth testing.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8904791 [details] [diff] [review]
cache GetGutterSize

Try looks good but for known intermittents ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab58fd28254c8a7e083d625a65b54838f283525e ).
Attachment #8904791 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
Attachment #8904791 - Flags: review?(jmathies) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
In the future, please make sure your attached patches contain all the necessary commit information before requesting checkin (name, email, commit message).
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages
Flags: needinfo?(agashlin)

Comment 10

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc17394d140
Cache the result of GetGutterSize to avoid expensive calls to GetThemeMargins(). r=jimm
Keywords: checkin-needed
Sorry about that, thanks again for filling it in for me.
Flags: needinfo?(agashlin)
https://hg.mozilla.org/mozilla-central/rev/4fc17394d140
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.