1488086 - Improve scrolling performance when recording

Status: RESOLVED WORKSFORME

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Scrolling feels pretty bad when recording an execution.  While the scroll is initially responsive, performance lags when handling long scrolls, and (for example) when rapidly attempting to scroll a page performance falls further and further behind, and can continue updating the page several seconds after input has ceased.

TabChild already has logic for coalescing closely spaced wheel events, but this logic never allows wheel events to be coalesced while recording.  Events will only be coalesced if their timestamps (the times they were generated in the UI process) are closer together than twice the time the child process took to dispatch them.  In a recording/replaying process, dispatching the events isn't any slower than in a normal content process, but there is additional overhead that is not present in a content process: each wheel event will trigger a synchronous paint in the in-process compositor.  I think that the extra overhead of this painting is causing the wheel events to be handled slower than they would be in a normal content process, and since they aren't coalesced the events just pile up.

The attached patch fixes this by changing the heuristics used to coalesce wheel events.  The main concept here seems to be that we should be coalescing events if the parent is generating them faster than the child is consuming them.  To that end, with this patch we compare the difference in time between two adjacent wheel events with the difference in time between the corresponding calls to TabChild::RecvMouseWheelEvent, and coalesce wheel events when the former is smaller than the latter.

Comment 1

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

Comment on attachment 9005906 [details] [diff] [review]
patch

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
>@@ -1779,17 +1779,17 @@ TabChild::MaybeCoalesceWheelEvent(const 
>     // 1. It's eWheel (we don't coalesce eOperationStart and eWheelOperationEnd)
>     // 2. It's not the first wheel event.
>     // 3. It's not the last wheel event.
>     // 4. It's dispatched before the last wheel event was processed +
>-    //    the processing time of the last event.
>+    //    the elapsed time since then.
>     //    This way pages spending lots of time in wheel listeners get wheel
>     //    events coalesced more aggressively.
>     // 5. It has same attributes as the coalesced wheel event which is not yet
>     //    fired.
>     if (!mLastWheelProcessedTimeFromParent.IsNull() &&
>         *aIsNextWheelEvent &&
>         aEvent.mTimeStamp < (mLastWheelProcessedTimeFromParent +
>-                             mLastWheelProcessingDuration) &&
>+                             (TimeStamp::Now() - mLastWheelProcessed)) &&
I'm having great trouble to understand this. The existing model has the nice feature that it coalesces the more the slower wheel event handlers are, but doesn't punish if other code outside event listeners is slow.
aEvent.mTimeStamp < (mLastWheelProcessedTimeFromParent + (TimeStamp::Now() - mLastWheelProcessed)) is true almost always.
IPC layer itself is enough to make mTimeStamp <.
This feels way too aggressive to me. Web pages which use wheel event for non-scrolling purposes could get way too few events.
And then, on the other hand, if there is long running wheel event listener, we would process a new event right after that listener has been executed.



Do you know how much other browsers coalesce?


>+
>+  // The time (from this process) of the last repeated wheel event we handled.
>+  mozilla::TimeStamp mLastWheelProcessed;
Ok, so this point to time _before_ event handling.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Comment on attachment 9005906 [details] [diff] [review]
> patch
> 
> >diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> >--- a/dom/ipc/TabChild.cpp
> >+++ b/dom/ipc/TabChild.cpp
> >@@ -1779,17 +1779,17 @@ TabChild::MaybeCoalesceWheelEvent(const 
> >     // 1. It's eWheel (we don't coalesce eOperationStart and eWheelOperationEnd)
> >     // 2. It's not the first wheel event.
> >     // 3. It's not the last wheel event.
> >     // 4. It's dispatched before the last wheel event was processed +
> >-    //    the processing time of the last event.
> >+    //    the elapsed time since then.
> >     //    This way pages spending lots of time in wheel listeners get wheel
> >     //    events coalesced more aggressively.
> >     // 5. It has same attributes as the coalesced wheel event which is not yet
> >     //    fired.
> >     if (!mLastWheelProcessedTimeFromParent.IsNull() &&
> >         *aIsNextWheelEvent &&
> >         aEvent.mTimeStamp < (mLastWheelProcessedTimeFromParent +
> >-                             mLastWheelProcessingDuration) &&
> >+                             (TimeStamp::Now() - mLastWheelProcessed)) &&
>
> I'm having great trouble to understand this. The existing model has the nice
> feature that it coalesces the more the slower wheel event handlers are, but
> doesn't punish if other code outside event listeners is slow.

This is precisely the problem this bug is addressing: if code outside the event listener is consistently slow, events are never coalesced and the web page basically breaks.

> aEvent.mTimeStamp < (mLastWheelProcessedTimeFromParent + (TimeStamp::Now() -
> mLastWheelProcessed)) is true almost always.
> IPC layer itself is enough to make mTimeStamp <.
> This feels way too aggressive to me. Web pages which use wheel event for
> non-scrolling purposes could get way too few events.
> And then, on the other hand, if there is long running wheel event listener,
> we would process a new event right after that listener has been executed.

I agree this may be too aggressive, but I think the goal of this new heuristic is important to preserve.  No matter what, we should not be handling scrolling events for actual user scrolling activity that occurred a long time ago.

I could make this less aggressive by putting a threshold for how long ago the scrolling activity occurred before we start coalescing, e.g. "coalesce scrolling events that originated more than 20ms ago", which would require a little more state here.  Would that be alright?

Comment 3

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

(In reply to Brian Hackett (:bhackett) from comment #2)

> This is precisely the problem this bug is addressing: if code outside the
> event listener is consistently slow, events are never coalesced and the web
> page basically breaks.

But the patch breaks the case for which coalescing was initially added for - slow wheel event listeners taking too much time.

> I could make this less aggressive by putting a threshold for how long ago
> the scrolling activity occurred before we start coalescing, e.g. "coalesce
> scrolling events that originated more than 20ms ago", which would require a
> little more state here.  Would that be alright?
20ms is rather magical.
Need to keep the old behavior, so start time + (2* handling duration), 
and then possibly some other condition, like if event's time < (currentime - 16ms), coalesce.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> 
> > This is precisely the problem this bug is addressing: if code outside the
> > event listener is consistently slow, events are never coalesced and the web
> > page basically breaks.
> 
> But the patch breaks the case for which coalescing was initially added for -
> slow wheel event listeners taking too much time.

The patch compares the time we started processing the last wheel event with the time we started processing the current wheel event.  That span includes all time spent in wheel event listeners.

That said, it's probably best to table this until after I've had time to work on bug 1488808.  If that bug pans out then we will be painting asynchronously while recording/replaying and might not need any changes to the heuristics here.

Comment 5

[Brian Hackett [Laid off!]]

Now that bug 1488808 is done, scrolling performance is pretty good when recording a tab, and the lag described in comment 0 is gone.  Being able to paint asynchronously helps, but I think a bigger factor is that we aren't forcing paints at every turn of the event loop anymore, and are using the usual mechanisms for managing incoming vsyncs.