1690687 - Investigate if we can avoid -[NSApplication postEvent:atStart:] to wake up the native event loop

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, task, P2)

Description

[Markus Stange [:mstange]]

I think this might help a lot with bug 1690669.

Profile: https://share.firefox.dev/2YFEghw

Code: https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/widget/cocoa/nsAppShell.mm#419-440

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1690687. Remove unneeded postEvent.

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.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/125b55180fd7
Remove unneeded postEvent. r=mstange

Comment 3

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/125b55180fd7

Comment 4

[Andra Esanu (needinfo me)]

== 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

Comment 5

[Jeff Muizelaar [:jrmuizel]]

It looks like this patch is causing very high cpu usage in xpcshell when running the browser/components/doh/test/browser/ tests

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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

Comment 7

[Markus Stange [:mstange]]

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

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

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

[Mike Conca [:mconca]]

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

[Ryan VanderMeulen [:RyanVM]]

Backed out for 94.0b8.
https://hg.mozilla.org/releases/mozilla-beta/rev/cf5517eb6e77