1361067 - coalesce mouse events to be once per refresh cycle (requestAnimationFrame, rAF) (preffed off)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Chrome appears to be planning to ship changes:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/Y9BrlDeS3x4/discussion
that will coalesce mouse events when there's more than one per refresh driver tick, and only send the last one.  This is to reduce the performance cost of processing unnecessary mouse events that have no effect on what is presented to the user.  Along with this, they're shipping an API to get access to all of the events, for drawing apps that actually do want all of the points.

(I actually was under the impression that Chrome *already* did something like this, at least more than we did, but maybe that impression was wrong.)

This seems like it's likely a good idea, since most mouse event processing doesn't need the extra events and it's just wasted work.

Bug 1149555 may be vaguely related.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Not all mouse events but mouse move and wheel.
We already 'compress' mousemoves in IPC layer and wheel events are coalesced in TabChild.
And once bug 1351148 gets fixed in some way, we hopefully somewhat automatically compress mousemoves and coalesce even more aggressively.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

FWIW, https://bugzilla.mozilla.org/show_bug.cgi?id=1351148&mark=38-39#c38
Would be good to test using mouse, not just touchpad.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Google shipped this in chrome 60:

https://www.chromestatus.com/feature/6192761317359616

Comment 4

[Andrew Overholt [:overholt]]

When Stone finishes work on bug 1351148, maybe he can take a look here.

Comment 5

[:Harald Kirschner :digitarald]

Tracking under Quantum Flow.

Comment 6

[:Harald Kirschner :digitarald]

Getting more data, profiled on OSX.

2 mousemove cycles per frame show up in

- Main. Moving the mouse over the buttons (cheap. 2 handlers, each around 0.05ms): https://perfht.ml/2vmrwQj
- Facebook. Moving the mouse over the page (expensive! 2 handlers, each 0.1-0.7ms): https://perfht.ml/2wfbsfQ
- Amazon. Hovering the product preview (medium. 3 pointermove/mousemove handlers, each 0.2-0.4ms) https://perfht.ml/2vmLdaG
- NYTimes. (cheap, 2 pointermove/mousemove, each 0.2ms): http://bit.ly/2wfafF4

On OSX Facebook and Amazon's mouse/pointermove event overhead on OSX is about 1ms, translating to 1/16th of the frame budget.

Comment 7

[:Harald Kirschner :digitarald]

Continuing prior comment for reference hardware

Facebook: http://bit.ly/2f1STIs - 2 mousemove events each 0.3-0.8ms
Amazon: http://bit.ly/2f1PfOT - 3 mouse/pointeremove each 0.2-0.7ms, worst cases 2ms
Youtube: http://bit.ly/2f2R9Pn - 5 mousemove, each 0.2-1ms consistently. Combined worst case is 4ms

Comment 8

[:Harald Kirschner :digitarald]

Marking for triage as I added some more data.

Comment 9

[Chris Peterson [:cpeterson]]

mrdoob wrote a jsfiddle test to measure mousemove events per frame, saying "Chrome now dispatches 1 mousemove per frame. Firefox dispatches ~2, Safari ~4."

https://jsfiddle.net/b9hs1kqx/

Discussion on Twitter:

https://twitter.com/mrdoob/status/890628257564966914

Comment 10

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Not all mouse events but mouse move and wheel.
> We already 'compress' mousemoves in IPC layer and wheel events are coalesced
> in TabChild.
> And once bug 1351148 gets fixed in some way, we hopefully somewhat
> automatically compress mousemoves and coalesce even more aggressively.

Tested with the patches of bug 1351148 applied and we still got 1~3 mousemove per refresh cycle.

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: Part1: Refactor coalescing mouse wheel events.

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Part2: Coalesce mouse move events to be once per refresh cycle

Patches to coalesce mouse move events in TabChild

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8896885 [details] [diff] [review]
Part2: Coalesce mouse move events to be once per refresh cycle


>diff --git a/dom/base/FlushType.h b/dom/base/FlushType.h
>--- a/dom/base/FlushType.h
>+++ b/dom/base/FlushType.h
>@@ -29,16 +29,17 @@ enum class FlushType : uint8_t {
>   Frames           = Style,
>   EnsurePresShellInitAndFrames = 4, /* As above, plus ensure the pres shell is alive */
>   InterruptibleLayout = 5, /* As above, plus flush reflow,
>                               but allow it to be interrupted (so
>                               an incomplete layout may result) */
>   Layout           = 6, /* As above, but layout must run to
>                            completion */
>   Display          = 7, /* As above, plus flush painting */
>+  Event            = 8, /* Flush pending events before notify other observers */
This looks weird, at least being the last one.

>+CoalescedMouseMoveFlusher::WillRefresh(mozilla::TimeStamp aTime) {
>+  MOZ_ASSERT(mNeedFlush);
>+  mTabChild->MaybeDispatchCoalescedMouseMoveEvents();
>+  RemoveObserver();
>+}
>+
>+void
>+CoalescedMouseMoveFlusher::StartObserver() {
>+  if (mNeedFlush) {
>+    return;
>+  }
>+  bool success = GetRefreshDriver()->AddRefreshObserver(this, FlushType::Event);
Hmm, ok , so we force refresh driver to tick even just to dispatch mousemove events.


>+void
>+CoalescedMouseMoveFlusher::RemoveObserver() {
>+  if (mNeedFlush) {
>+    GetRefreshDriver()->RemoveRefreshObserver(this, FlushType::Event);
So this may crash, since GetRefreshDriver may return null.
Should we possibly keep a strong reference to the RefreshDriver after AddRefreshObserver and remove that reference after RemoveObserver is called or so.

What happens if a new page is loaded while there is an observer added to a refresh driver? GetRefreshDriver() would give the RefreshDriver for the old page when StartObserver is called and then
RemoveObserver would get pointer to the new RefreshDriver I think.


>+CoalescedMouseMoveFlusher::GetRefreshDriver() {
>+  nsCOMPtr<nsIPresShell> presShell = mTabChild->GetPresShell();
>+  MOZ_ASSERT(presShell);
>+  MOZ_ASSERT(presShell->GetPresContext());
>+  MOZ_ASSERT(presShell->GetPresContext()->RefreshDriver());
>+  return presShell->GetPresContext()->RefreshDriver();
>+}
Some null check here please. During shutdown GetPresContext may return null.


>--- a/layout/base/nsRefreshDriver.cpp
>+++ b/layout/base/nsRefreshDriver.cpp
>@@ -1475,16 +1475,18 @@ nsRefreshDriver::ArrayFor(FlushType aFlu
> {
>   switch (aFlushType) {
>     case FlushType::Style:
>       return mObservers[0];
>     case FlushType::Layout:
>       return mObservers[1];
>     case FlushType::Display:
>       return mObservers[2];
>+    case FlushType::Event:
>+      return mObservers[3];
Is this really what we want? Dispatching events after flushing styling and layout?
I would have imagined we'd dispatch events before all that.

In general looks quite reasonable.

Comment 14

[Ming-Chou Shih [:stone]]

Attachment: Part2: Coalesce mouse move events to be once per refresh cycle

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8897369 [details] [diff] [review]
Part2: Coalesce mouse move events to be once per refresh cycle

>+CoalescedMouseMoveFlusher::WillRefresh(mozilla::TimeStamp aTime) {
Nit, { to its own line


>+CoalescedMouseMoveFlusher::StartObserver() {
Ditto

>+  nsRefreshDriver* refreshDriver = GetRefreshDriver();
>+  if (mRefreshDriver && mRefreshDriver == refreshDriver) {
>+    // Nothing to do if we already add a observer and it's same refresh driver.
if we already added an observer

>+CoalescedMouseMoveFlusher::RemoveObserver() {
{ to its own line.


>+CoalescedMouseMoveFlusher::GetRefreshDriver() {
ditto

>+TabChild::MaybeDispatchCoalescedMouseMoveEvents()
>+{
>+  if (!mCoalesceMouseMoveEvents || mCoalescedMouseData.IsEmpty()) {
>+    return;
>+  }
>+  const WidgetMouseEvent* event = mCoalescedMouseData.GetCoalescedEvent();
>+  MOZ_ASSERT(event);
>+  RecvRealMouseButtonEvent(*event,
>+                           mCoalescedWheelData.GetScrollableLayerGuid(),
>+                           mCoalescedWheelData.GetInputBlockId());
Could you add a comment why RecvRealMouseButtonEvent is called here. (to bypass the coalesce handling there is in RecvRealMouseMoveEvent)


> pref("dom.event.contextmenu.enabled",       true);
> pref("dom.event.clipboardevents.enabled",   true);
> pref("dom.event.highrestimestamp.enabled",  true);
>+pref("dom.event.coalesce_mouse_move",       true);
Better to have this enabled first only in nightly.
This is rather risky after all.
Before shipping to release builds we need intent-to-ship in dev.platform

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #15)
> > pref("dom.event.contextmenu.enabled",       true);
> > pref("dom.event.clipboardevents.enabled",   true);
> > pref("dom.event.highrestimestamp.enabled",  true);
> >+pref("dom.event.coalesce_mouse_move",       true);
> Better to have this enabled first only in nightly.
> This is rather risky after all.
> Before shipping to release builds we need intent-to-ship in dev.platform

Has the API that allows pages to get the full sequence of mouse moves been implemented?  I'm not even sure it's a good idea to enable on nightly without that, since it would be asking people to test a configuration that we don't want to ship.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

There is no proper API proposal to let pages to get full sequence of mouse moves.
For pointer events there is getCoalescedEvents.
(and we've never guaranteed full sequence of mousemoves)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

But yeah, initially we should perhaps have the pref off anyhow.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Even if there isn't a "proper" proposal -- did Google implement something?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I'm not aware.
There is discussion for raw events in https://github.com/w3c/pointerevents/issues/214

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #19)
> Even if there isn't a "proper" proposal -- did Google implement something?

See comment 3.  They have a google doc linked from the chrome status page.

Comment 22

[(no longer active)]

The replacement that Google has implemented and shipped is getCoalescedEvents(), and as far as I can tell they are not planning to implement anything else, as per their intent to ship thread: https://groups.google.com/a/chromium.org/d/msg/blink-dev/Y9BrlDeS3x4/zs-onYzwAgAJ

Note that post also mentions that this API is specific to pointer events, and that has been deemed sufficient by the Chrome team.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

FWIW, http://logs.glob.uno/?c=freenode%23whatwg&s=16+Aug+2017&e=16+Aug+2017#c1034893

Comment 24

[Ming-Chou Shih [:stone]]

Attachment: Part3: Ticking refresh driver when it's controlled by the test

Comment 25

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

Comment on attachment 8898161 [details] [diff] [review]
Part3: Ticking refresh driver when it's controlled by the test

Review of attachment 8898161 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really like this change because it means we're adding unexpected side-effects into this mousemove synthesization function. Presumably if the test is manually managing the refresh driver that's for a reason, and if some helper function is ticking the refresh driver as a side-effect that might invalidate what the test is trying to do. It took me a long time to make sure this change doesn't invalidate test_touch_listeners_impacting_wheel.html for example. And while I *think* this change is ok with our current set of tests, I think it makes the tests harder to reason about and I'd like to avoid it.

Do you know which tests were affected by this? As far as I can tell this should only affect the test_touch_listeners_impacting_wheel.html test, and in that case I think we can fix it by moving the line at [1] up a little bit so that it happens at [2] instead. Does that work for you?

[1] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/gfx/layers/apz/test/mochitest/test_touch_listeners_impacting_wheel.html#89
[2] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/gfx/layers/apz/test/mochitest/test_touch_listeners_impacting_wheel.html#81

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Tweak test_touch_listeners_impacting_wheel.html to synthesize mousemove before taking control of refresh driver

Comment 27

[Ming-Chou Shih [:stone]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Comment on attachment 8898161 [details] [diff] [review]
> Part3: Ticking refresh driver when it's controlled by the test
> 
> Review of attachment 8898161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really like this change because it means we're adding unexpected
> side-effects into this mousemove synthesization function. Presumably if the
> test is manually managing the refresh driver that's for a reason, and if
> some helper function is ticking the refresh driver as a side-effect that
> might invalidate what the test is trying to do. It took me a long time to
> make sure this change doesn't invalidate
> test_touch_listeners_impacting_wheel.html for example. And while I *think*
> this change is ok with our current set of tests, I think it makes the tests
> harder to reason about and I'd like to avoid it.
Agree with you. It may cause some troubles to users.

> Do you know which tests were affected by this? As far as I can tell this
> should only affect the test_touch_listeners_impacting_wheel.html test, and
> in that case I think we can fix it by moving the line at [1] up a little bit
> so that it happens at [2] instead. Does that work for you?
Yes. It works. Thanks.

Comment 28

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

Comment on attachment 8898537 [details] [diff] [review]
Tweak test_touch_listeners_impacting_wheel.html to synthesize mousemove before taking control of refresh driver

Review of attachment 8898537 [details] [diff] [review]:
-----------------------------------------------------------------

Please keep the block comment on the line that synthesizes the wheel event. Just move the one line that does the mouse move event. Thanks!

Comment 29

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88bc21cfbd80cde01d2374e2a7efc6ed644e0df0

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1edccf6d5a8a
Part1: Refactor coalescing mouse wheel events. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/64edaaccec97
Part2: Coalesce mouse move events to be once per refresh cycle. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2186f168ab69
Part3: Tweak test_touch_listeners_impacting_wheel.html to synthesize mousemove before taking control of refresh driver. r=kats.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1edccf6d5a8a
https://hg.mozilla.org/mozilla-central/rev/64edaaccec97
https://hg.mozilla.org/mozilla-central/rev/2186f168ab69

Comment 32

[(no longer active)]

Is there a bug for enabling this at some point?  Thanks!

Comment 33

[Ming-Chou Shih [:stone]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #32)
> Is there a bug for enabling this at some point?  Thanks!

I created bug 1392876 for enabling this.

Comment 34

[Daniel Holbert [:dholbert]]

(Tweaking summary to make it clearer what the distinction is between this bug and bug 1392876.)