Excessive latency between touch events and drawing during hard flings

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 14
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 beta+)

Details

Attachments

(2 attachments, 8 obsolete attachments)

3.56 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.47 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
I stuck some logging in the event handler in nsAppShell.cpp and found that on the Galaxy Nexus (debug build, with the no-margins displayport strategy) we spend at least 150ms (often up to 200 or 250ms) between handling the first touch-related event and the draw event. This seems to be a combination of bad event ordering, bad event coalescing, the refresh driver latency, and general android thread timeslicing. If we want to reduce checkerboarding at the start of flings, when the pan velocity is very high, we should try to tackle some of these problems. I think most of them are solveable; I played around with different event coalescing and killing the refresh driver latency and those gave me different results (not necessary better, since the changes affect other things too). We should also consider bumping up the Gecko thread priority when checkerboard is visible so that android schedules it more often and we draw the right thing faster.
Depends on: 730914
Depends on: 730515
Also, the handling for Gesture:ShowPress, which runs right when the user puts their finger down, takes too long. I'm seeing times in the 60-160ms range. This is causing all of our other events to get backed up, and means we checkerboard more right at the start of the fling because we're busy running that event instead of drawing. Commenting out that event makes a noticeable improvement (but obviously breaks functionality).
No longer depends on: 730515
Depends on: 741228
I filed bug 741228 for the thing in comment #1, since it is not immediately obvious how to fix it.
Created attachment 611303 [details] [diff] [review]
(1/5) Fix correctness for VIEWPORT even handling

These patches need the patch from bug 740718 to apply cleanly. Also I haven't run the checkerboarding tests with these patches applied; will do that Monday when I have more devices. Feedback welcome if anybody gets a chance to look at the them in the meantime.
Created attachment 611304 [details] [diff] [review]
WIP (2/3) reduce DRAW event latency
Created attachment 611305 [details] [diff] [review]
WIP (3/3) reduce refresh driver latency by scheduling draw events ourselves
Attachment #611303 - Attachment description: WIP (1/3) reduce VIEWPORT event latency → (1/5) Fix correctness for VIEWPORT even handling
Attachment #611303 - Flags: review?(chrislord.net)
Comment on attachment 611303 [details] [diff] [review]
(1/5) Fix correctness for VIEWPORT even handling

Actually let me rearrange some of these patches for easier review and re-upload.
Attachment #611303 - Flags: review?(chrislord.net)
Created attachment 611537 [details] [diff] [review]
(1/5) Reduce draw event latency
Attachment #611304 - Attachment is obsolete: true
Attachment #611537 - Flags: review?(chrislord.net)
Created attachment 611539 [details] [diff] [review]
(2/5) Reduce refresh driver latency by scheduling draw events ourselves

I don't really like doing this, and am perfectly fine with leaving this patch out. It's relatively independent of the other patches anyway, and can just be excluded from the series. I'm mostly just including this for completeness.
Attachment #611305 - Attachment is obsolete: true
Attachment #611539 - Flags: review?(chrislord.net)
Created attachment 611540 [details] [diff] [review]
(3/5) Replace if/else block with switch statement
Attachment #611540 - Flags: review?(chrislord.net)
Created attachment 611541 [details] [diff] [review]
(4/5) Fix correctness and improve latency for viewport event handling
Attachment #611303 - Attachment is obsolete: true
Attachment #611541 - Flags: review?(chrislord.net)
Created attachment 611543 [details] [diff] [review]
(5/5) Coalesce viewport events across poke/draw events

This change feels a little brittle, any thoughts on improving the code arrangement are welcome.
Attachment #611543 - Flags: review?(chrislord.net)
Depends on: 741565
blocking-fennec1.0: --- → ?
Comment on attachment 611537 [details] [diff] [review]
(1/5) Reduce draw event latency

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

Took me a while to wrap my head around what this was doing, perhaps because I've had a couple of days off :) I think a comment under the VIEWPORT case in the switch statement, explaining what mViewportChangedSinceLastDraw does would make this nicer. Something along the lines of "Temporarily turn off draw-event coalescing, so that we process a draw event as soon as possible after a viewport change".
Attachment #611537 - Flags: review?(chrislord.net) → review+
Comment on attachment 611539 [details] [diff] [review]
(2/5) Reduce refresh driver latency by scheduling draw events ourselves

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

I'm not too keen on this change, I'd rather we figured out a way to reduce the latency as the result of a viewport event, instead of working around it like this (especially as we lack the context to be able to have a correct update rectangle). How much do we save with this change? I'll r+/r- depending on that.

::: widget/android/nsAppShell.cpp
@@ +650,5 @@
> +        // The refresh driver will trigger one anyway when we process the viewport event,
> +        // but there is added latency in that path which we would like to avoid. The
> +        // DRAW event coalescing behaviour should take care of preventing us from doing
> +        // unneeded draws in these cases.
> +        nsIntRect rect(0, 0, 0, 0);

While we don't currently make use of any partial-update double-buffer extensions, it's worth mentioning here that we're assuming the whole screen changes on a viewport change.

There should be a bug about the latency that this comment references, I don't think this is a long-term solution.
Comment on attachment 611540 [details] [diff] [review]
(3/5) Replace if/else block with switch statement

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

Looks fine to me.
Attachment #611540 - Flags: review?(chrislord.net) → review+
Comment on attachment 611541 [details] [diff] [review]
(4/5) Fix correctness and improve latency for viewport event handling

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

Looks good to me.

::: widget/android/nsAppShell.cpp
@@ +262,5 @@
> +            // of time before we get around to processing that one. also other
> +            // events like the touch events depend on us setting gecko viewport
> +            // (see LayerController.convertViewPointToLayerPoint) when calculating
> +            // offsets relative to gecko's viewport, and throwing out viewport
> +            // events will screw that up.

nit: s/screw/mess/ ? :)
Attachment #611541 - Flags: review?(chrislord.net) → review+
Comment on attachment 611543 [details] [diff] [review]
(5/5) Coalesce viewport events across poke/draw events

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

Like it, have made suggestions in comments, but r+ regardless.

::: widget/android/AndroidJavaWrappers.cpp
@@ +556,5 @@
>      mPoints = aResizeEvent->mPoints; // x,y coordinates
>  }
>  
>  void
> +AndroidGeckoEvent::CoalesceViewportEvent(AndroidGeckoEvent *aViewportEvent)

Maybe rename this OverwriteViewportEvent ?

::: widget/android/nsAppShell.h
@@ +93,5 @@
>          *aBrowserApp = mBrowserApp;
>      }
>  
> +private:
> +    void AppendEventInternal(mozilla::AndroidGeckoEvent *ae);

Perhaps it would be good to add a comment above the definition of mEventQueue that this should only be appended to via this function.

Or for robustness, perhaps this should be the protected method, mEventQueue should be private and any additional accessors should be added if necessary (Insert, Remove, etc.)
Attachment #611543 - Flags: review?(chrislord.net) → review+
blocking-fennec1.0: ? → beta+
Comment on attachment 611539 [details] [diff] [review]
(2/5) Reduce refresh driver latency by scheduling draw events ourselves

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

Minusing this one since it's not worth it.
Attachment #611539 - Flags: review?(chrislord.net) → review-
Created attachment 613235 [details] [diff] [review]
(1/2) Reduce draw event latency

This patch had to be rebased on top of the patch from bug 740718 which resulted in a significant change to the patch (although should be functionally equivalent). will request review once i've tested this.
Attachment #611537 - Attachment is obsolete: true
Created attachment 613236 [details] [diff] [review]
(2/2) Coalesce viewport events across poke/draw events

Same as above.
Attachment #611539 - Attachment is obsolete: true
Attachment #611540 - Attachment is obsolete: true
Attachment #611541 - Attachment is obsolete: true
Attachment #611543 - Attachment is obsolete: true
Attachment #613235 - Flags: review?(chrislord.net)
Comment on attachment 613236 [details] [diff] [review]
(2/2) Coalesce viewport events across poke/draw events

These patches seem to accomplish what I want, requesting review.
Attachment #613236 - Flags: review?(chrislord.net)
Depends on: 743736
Comment on attachment 613235 [details] [diff] [review]
(1/2) Reduce draw event latency

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

Looks good to me.
Attachment #613235 - Flags: review?(chrislord.net) → review+
Comment on attachment 613236 [details] [diff] [review]
(2/2) Coalesce viewport events across poke/draw events

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

Looks good apart from the one comment, which should be addressed.

::: widget/android/nsAppShell.cpp
@@ +519,5 @@
>      {
> +        // set this to true when inserting events that we can coalesce
> +        // viewport events across. this is effectively maintaining a whitelist
> +        // of events that are unaffected by viewport changes.
> +        bool mAllowCoalescingNextViewport = false;

Don't prefix this with m if it isn't a member of the class.
Attachment #613236 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #22)
> Comment on attachment 613236 [details] [diff] [review]
> 
> Don't prefix this with m if it isn't a member of the class.

Oh, good catch, fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7953acc6c72
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e947b367402
status-firefox14: --- → fixed
Target Milestone: --- → Firefox 14

Comment 24

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f7953acc6c72
https://hg.mozilla.org/mozilla-central/rev/1e947b367402
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.