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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: dbaron, Assigned: stone)

Tracking

({perf})

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p2])

Attachments

(3 attachments, 2 obsolete attachments)

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.
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.
Depends on: 1351148
Priority: -- → P1
FWIW, https://bugzilla.mozilla.org/show_bug.cgi?id=1351148&mark=38-39#c38
Would be good to test using mouse, not just touchpad.
When Stone finishes work on bug 1351148, maybe he can take a look here.
Flags: needinfo?(sshih)
Tracking under Quantum Flow.
Whiteboard: [qf]

Updated

2 years ago
Whiteboard: [qf] → [qf:p3]
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.
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
Marking for triage as I added some more data.
Whiteboard: [qf:p3] → [qf]
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
Whiteboard: [qf] → [qf:p2]
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih
Flags: needinfo?(sshih)
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 12

2 years ago
Patches to coalesce mouse move events in TabChild
(Assignee)

Updated

2 years ago
Attachment #8896885 - Flags: feedback?(bugs)
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.
Attachment #8896885 - Flags: feedback?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8896884 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8897369 - Flags: review?(bugs)
Attachment #8896884 - Flags: review?(bugs) → review+
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
Attachment #8897369 - Flags: review?(bugs) → review+
(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.
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)
But yeah, initially we should perhaps have the pref off anyhow.
Even if there isn't a "proper" proposal -- did Google implement something?
I'm not aware.
There is discussion for raw events in https://github.com/w3c/pointerevents/issues/214
(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.
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.
(Assignee)

Updated

2 years ago
Attachment #8898161 - Flags: review?(bugmail)
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
Attachment #8898161 - Flags: review?(bugmail) → review-
(Assignee)

Comment 27

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Updated

2 years ago
Attachment #8898537 - Flags: review?(bugmail)
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!
Attachment #8898537 - Flags: review?(bugmail) → review+

Comment 30

2 years ago
Pushed by sshih@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1edccf6d5a8a
https://hg.mozilla.org/mozilla-central/rev/64edaaccec97
https://hg.mozilla.org/mozilla-central/rev/2186f168ab69
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there a bug for enabling this at some point?  Thanks!
(Assignee)

Updated

2 years ago
Blocks: 1392876
(Assignee)

Comment 33

2 years ago
(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.
(Tweaking summary to make it clearer what the distinction is between this bug and bug 1392876.)
Summary: coalesce mouse events to be once per refresh cycle (requestAnimationFrame, rAF) → coalesce mouse events to be once per refresh cycle (requestAnimationFrame, rAF) (preffed off)
Depends on: 1397461
(Assignee)

Updated

2 years ago
Blocks: 1401450
Depends on: 1408157
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.