Investigate if we can avoid -[NSApplication postEvent:atStart:] to wake up the native event loop
Categories
(Core :: Widget: Cocoa, task, P2)
Tracking
()
People
(Reporter: mstange, Assigned: jrmuizel)
References
Details
(Keywords: perf-alert)
Attachments
(1 file)
I think this might help a lot with bug 1690669.
Profile: https://share.firefox.dev/2YFEghw
The profile shows a fair bit of time in NSEvent creation and freeing, and in setWindowsNeedUpdate
and updateWindows
.
The documentation on -[NSWindow update]
says:
An NSWindow object is automatically sent an update message on every pass through the event loop
It would be great if we could avoid this if we no longer post NSEvents during normal operation.
Reporter | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
This avoids always posting a NSEvent to the event loop everytime we run a
non-native event. Servicing native NSEvents appears to have quite a high
overhead and causes us to commit a CoreAnimation transaction and communicate
with the WindowServer for every Gecko event. This change should also reduce the
CPU usage of the WindowServer process.
It appears that we still need to post an NSEvent when we're in a nested event loop
or when we're shutting down, but those situations are uncommon.
Updated•3 years ago
|
Pushed by jmuizelaar@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/125b55180fd7 Remove unneeded postEvent. r=mstange
Comment 3•3 years ago
|
||
bugherder |
Comment 4•3 years ago
|
||
== Change summary for alert #31249 (as of Fri, 10 Sep 2021 10:09:07 GMT) ==
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
3% | tscrollx | macosx1015-64-shippable-qr | e10s fission stylo webrender | 0.64 -> 0.62 |
2% | tscrollx | macosx1015-64-shippable-qr | e10s stylo webrender-sw | 0.65 -> 0.63 |
2% | tscrollx | macosx1015-64-shippable-qr | e10s stylo webrender-sw | 0.65 -> 0.63 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31249
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
It looks like this patch is causing very high cpu usage in xpcshell when running the browser/components/doh/test/browser/ tests
Assignee | ||
Comment 6•3 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: Overall reduction in CPU usage
[Affects Firefox for Android]: None
[Suggested wording]: Reduced event handling overhead on macOS
Reporter | ||
Comment 7•3 years ago
|
||
To get a sense of the impact of this fix on full screen YouTube video playback, I've collected these two profiles.
Before (2021-09-05): https://share.firefox.dev/3BIciUm
After (2021-10-15): https://share.firefox.dev/3lGyE2Z
The "before" profile shows a lot more CPU usage on the main thread whenever new data is fetched from the network.
However, I couldn't see any difference in the power numbers in Intel Power Gadget between the two builds.
Comment 8•3 years ago
|
||
Mike, I could use a second opinion on this one.
We're trying to decide whether or not to leave this fix in 94 or back it out from Beta and let it ride 95 instead. The main issue is bug 1734705, which causes noticeable scrolling lag for Mac users. A fix is ready, but hasn't had much time to bake and has unclear risk to users (see comments 25 and down in that bug). And we only have a week left before 94 goes to RC.
Given comment 7, it appears that the patch from this bug has led to significant CPU usage wins, but it's unclear if those translate to measurable power savings. I'm personally leaning more towards backing this out from 94 and letting this and bug 1734705 ride 95 instead since it doesn't seem to help with the Mac power savings wins we're touting for this release, but I can see the argument for taking the uplift and letting these patches ride 94 too. What do you think from the Product perspective?
Assignee | ||
Comment 9•3 years ago
|
||
Another thing worth considering is that if there are regressions that come from bug 1734705, I wouldn't be surprised if they don't show up until we hit release which means we wouldn't have gained anything from the extra bake time.
I too am interested in product's thoughts.
Comment 10•3 years ago
|
||
I think we should back this out of Beta 94. While there are probably battery life wins here, given the decreased CPU usage, taking on the potential risks for unknown gains doesn't feel like the right approached. We are much more confident in the battery life wins from Bug 1653417, and those are significant enough to carry the battery life story for 94.
Comment 11•3 years ago
|
||
backout |
Backed out for 94.0b8.
https://hg.mozilla.org/releases/mozilla-beta/rev/cf5517eb6e77
Description
•