1130016 - Touchstart listener in lockscreen_notifications.js can slow down APZ responsiveness

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #1130011 +++

With parent-process APZ and (hopefully soon) the input-thread work landing, it's more important than ever to not have spurious touchstart and touchmove listeners covering large parts of the gaia UI. Because gecko cannot know ahead of time if the listeners are going to call preventDefault() or not on the event, it must wait for the listeners to run before it can do any processing on those events, such as panning/clicking. Therefore adding touchstart and touchmove listeners on things like the root window in gaia will directly have a negative performance impact when it comes to responsiveness.

This bug is specifically about the touchstart listener registered in lockscreen_notification.js [1]. As far as I can tell this (and I could be wrong completely) this listener is only used in very specific scenarios dealing with the notification scroller on the lockscreen, and should be removable when the lockscreen is not visible. Doing so would help prevent this listener from impacting performance while in other parts of the Gaia UI.

[1] https://github.com/mozilla-b2g/gaia/blob/ded85e94a325d1546d5c46fbe68153ddd7671181/apps/system/lockscreen/js/lockscreen_notifications.js#L37

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Greg, do you know if it's easy to unregister this touch listener when it's not needed?

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

While it may be OK to do that, I would like to know if you mean that any event listener on window would bring some performance impact, even it's un-triggered with events? Since from the client side there is no difference between the hooked-but-unused listener and the in-heavy-used listener.

And from your comment:

> Because gecko cannot know ahead of time if the listeners are going to call preventDefault() or not on the event, it must wait for the listeners to run before it can do any processing on those events, such as panning/clicking.

I don't know what the 'wait for the listeners to run' is waiting what. Do you mean the main process would need to scan all touching listeners when one fresh touch event happened, so that's what your 'waiting' means? Also, if this is a general patterns impact the performance, does it means other types of event listeners may encounter the same issue?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #2)
> While it may be OK to do that, I would like to know if you mean that any
> event listener on window would bring some performance impact, even it's
> un-triggered with events? Since from the client side there is no difference
> between the hooked-but-unused listener and the in-heavy-used listener.

I'm not sure I understand here. Once you register a touch event listener on the window it will get run for all touch events in that window. What do you mean by "hooked-but-unused"?

> And from your comment:
> 
> > Because gecko cannot know ahead of time if the listeners are going to call preventDefault() or not on the event, it must wait for the listeners to run before it can do any processing on those events, such as panning/clicking.
> 
> I don't know what the 'wait for the listeners to run' is waiting what. Do
> you mean the main process would need to scan all touching listeners when one
> fresh touch event happened, so that's what your 'waiting' means?

If I understand your question correctly, yes. The way it works for touch events is that preventDefault can be called on the touchstart or first touchmove event by listeners. If that happens then the APZ cannot use those events for scrolling. In gecko when we receive the touch event from the hardware we know if there are listeners registered or not, so if they are registered then we need to run those listeners first before APZ can start scrolling. If there are no listeners registered then we can start scrolling right away. It doesn't matter if the listener actually does preventDefault or not - as long as there is a listener, even if it does absolutely nothing - we still need to wait for it to run. Does that clarify the situation?

> Also, if
> this is a general patterns impact the performance, does it means other types
> of event listeners may encounter the same issue?

Touch events are special in this regard because calling preventDefault on them actually has an effect. Calling preventDefault on mouse events for example does nothing. There are other events where calling preventDefault has an effect but the APZ code on B2G really only cares about touch events.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Okay. I would take a while to patch the bug next week (it's Chinese's New Year now). NI myself as a memo

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Thanks, and happy new year!

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] snowmantw:bug1130016 > mozilla-b2g:master

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

:kats:

I've a WIP patch here. Could you test that if the patch really works with your test method?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Unfortunately it doesn't seem to fully work. It looks like the 'visibilitychange' event only fires when the screen goes off/on. So when I unlock the device and go to the homescreen there's no visibilitychange event, and the touchstart event listener remains attached.

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

I've updated the patch and it should work well now. From the log I added it would remove the listener when it's unlocked, and re-added it while it's locked. You may take a look again.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Yup this seems to work great, thanks!

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8568365 [details] [review]
[gaia] snowmantw:bug1130016 > mozilla-b2g:master

Since I've updated the patch and there are tests I set the reviewer.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8568365 [details] [review]
[gaia] snowmantw:bug1130016 > mozilla-b2g:master

Thanks :kats for filing the bug. This is one of these implicit optimization that can be hard to target with DOM API.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

TBPL is green:
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4a001eba7b46

master:

https://github.com/mozilla-b2g/gaia/commit/fb47c645ccd4778040ed51ef3e89d2a004ba9ecc