Closed Bug 1393685 Opened 7 years ago Closed 7 years ago

GetThemeMargins() can be extremely expensive

Categories

(Core :: Widget: Win32, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Performance Impact high
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: agashlin)

References

Details

Attachments

(1 file)

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)
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]
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)
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.
Status: NEW → ASSIGNED
Attachment #8904791 - Flags: review?(jmathies) → review+
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)
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.