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?
Looks like that metric can get cached and reset when we receive a settings change events.
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 ? 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?  http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/widget/windows/nsNativeThemeWin.cpp#2276,2288
Assignee: nobody → agashlin
Created attachment 8904791 [details] [diff] [review] cache GetGutterSize 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.
Hard to predict if sharing that cached value will regress, I think landing what you have with the single cached value is worth testing.
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)
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
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc17394d140 Cache the result of GetGutterSize to avoid expensive calls to GetThemeMargins(). r=jimm
Sorry about that, thanks again for filling it in for me.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.