Closed
Bug 1393685
Opened 7 years ago
Closed 7 years ago
GetThemeMargins() can be extremely expensive
Categories
(Core :: Widget: Win32, enhancement)
Core
Widget: Win32
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: agashlin)
References
Details
Attachments
(1 file)
5.12 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•7 years ago
|
||
Looks like that metric can get cached and reset when we receive a settings change events.
Flags: needinfo?(jmathies)
Comment 2•7 years ago
|
||
Sounds pretty straightforward and like a perf win; let's call this qf:p1.
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 3•7 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•7 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?
Comment 5•7 years ago
|
||
(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?
Comment 7•7 years ago
|
||
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•7 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•7 years ago
|
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8904791 -
Flags: review?(jmathies) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 9•7 years ago
|
||
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•7 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
Assignee | ||
Comment 11•7 years ago
|
||
Sorry about that, thanks again for filling it in for me.
Flags: needinfo?(agashlin)
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4fc17394d140
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•