Closed Bug 1130016 Opened 7 years ago Closed 7 years ago

Touchstart listener in lockscreen_notifications.js can slow down APZ responsiveness

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.2 affected, b2g-master fixed)

RESOLVED FIXED
Tracking Status
b2g-v2.2 --- affected
b2g-master --- fixed

People

(Reporter: kats, Assigned: gweng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

+++ 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
Greg, do you know if it's easy to unregister this touch listener when it's not needed?
Flags: needinfo?(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?
Flags: needinfo?(gweng)
(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.
Okay. I would take a while to patch the bug next week (it's Chinese's New Year now). NI myself as a memo
Flags: needinfo?(gweng)
Thanks, and happy new year!
:kats:

I've a WIP patch here. Could you test that if the patch really works with your test method?
Flags: needinfo?(gweng) → needinfo?(bugmail.mozilla)
Assignee: nobody → gweng
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.
Flags: needinfo?(bugmail.mozilla) → needinfo?(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.
Flags: needinfo?(gweng)
Yup this seems to work great, thanks!
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.
Attachment #8568365 - Flags: review?(timdream)
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.
Attachment #8568365 - Flags: review?(timdream) → review+
You need to log in before you can comment on or make changes to this bug.