Closed Bug 1069037 Opened 5 years ago Closed 5 years ago

Prevent Touch Events From Piling Up If main Thread is Busy

Categories

(Core Graveyard :: Widget: Gonk, defect)

26 Branch
x86
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 5 obsolete files)

:mwu pointed out a bug where if the main thread is very busy, we don't always throw out touch events. We can pile up touch events and dispatch them in rapid succession, which just adds to more load. We have a couple of ideas.

If resampling is enabled, check to make sure that the current runnable is within some configurable number of the last vsync, otherwise return. If resampling is disabled, we'll send only the last touch event.
Blocks: 1062331
No longer depends on: 1062331
See Also: → 1073704
Did a couple of things to prevent touch events from piling up. If touch resampling is disabled, we overwrite the last touch move event, which was what was happening before we introduced GeckoTouchDispatcher. If touch resampling is enabled, we check the vsync time versus the last touch time. If the time between the vsync event occured and when the main thread finally processes the touch is greater than 20ms, we assume the main thread is too busy and just dispatch the last touch event.
Attachment #8502785 - Flags: review?(mwu)
Comment on attachment 8502785 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +151,5 @@
> +      mTouchMoveEvents.push_back(aData);
> +      mTouchTimeDiff = aEventTime - mLastTouchTime;
> +      mLastTouchTime = aEventTime;
> +      return;
> +    } else if (mTouchMoveEvents.empty()) {

Using else if here is a bit confusing. I suggest just breaking this off into its own if statement.

@@ +178,5 @@
>      int touchCount = mTouchMoveEvents.size();
>      // Both aVsynctime and mLastTouchTime are uint64_t
>      // Need to store as a signed int.
>      int64_t vsyncTouchDiff = aVsyncTime - mLastTouchTime;
> +    bool isDelayedTouch = vsyncTouchDiff < mDelayedTouchThreshold;

This is always true when resampling is disabled, which makes the code that checks isDelayedTouch pointless. Please check the logic here again.
Attachment #8502785 - Flags: review?(mwu)
Attached patch Refactor GeckoTouchDispatcher (obsolete) — Splinter Review
Have a couple of changes as talked about on irc.

1) Made code more explicit that it needs to rely on touch resampling being enabled.
2) Modified GeckoTouchDispatcher to always use mozilla::TimeStamp instead of the android event time. Code that uses this is in the VsyncFramework in bug 1048667.
3) Also changed the resampling a bit to be more accurate. Before, we could get touch events that occurred after a vsync. We would then always use the last two touch events to resample, which means we would include a touch event that occurred after the vsync event. Changed it so that we always process touch events that occur before the current vsync event.
4) Also filter heavily based on timings. I've seen 10-20 ms roundtrip timings between posting an event on the main thread and the event actually executing. I've also seen the inverse where a vsync event occurs and no touch events have been added due to scheduling issues. We no longer resample in those cases, send the last touch event, and clear touch events until we catch up again.
Attachment #8504408 - Flags: review?(mwu)
Attachment #8502785 - Attachment is obsolete: true
Attached patch Refactor GeckoTouchDispatcher (obsolete) — Splinter Review
Fixed a build issue.
Attachment #8504408 - Attachment is obsolete: true
Attachment #8504408 - Flags: review?(mwu)
Attachment #8504427 - Flags: review?(mwu)
Comment on attachment 8504427 [details] [diff] [review]
Refactor GeckoTouchDispatcher

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

Way too many changes in here. Please split into a patch that fixes *just* this bug, and we can look at refactoring as a separate thing.

::: widget/InputData.h
@@ +7,5 @@
>  #define InputData_h__
>  
> +#include "Units.h"
> +#include "mozilla/EventForwards.h"
> +#include "mozilla/TimeStamp.h"

Why is this being moved?
Attachment #8504427 - Flags: review?(mwu)
(In reply to Michael Wu [:mwu] from comment #5)
> Comment on attachment 8504427 [details] [diff] [review]
> Refactor GeckoTouchDispatcher
> 
> Review of attachment 8504427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Way too many changes in here. Please split into a patch that fixes *just*
> this bug, and we can look at refactoring as a separate thing.
> 
> ::: widget/InputData.h
> @@ +7,5 @@
> >  #define InputData_h__
> >  
> > +#include "Units.h"
> > +#include "mozilla/EventForwards.h"
> > +#include "mozilla/TimeStamp.h"
> 
> Why is this being moved?

I just noticed that it wasn't alphabetically sorted, so I thought I'd clean that bit up since I was looking at that code.
Simplified a bit. Does two main things:

1) If touch resampling is disabled, we coalesce touch moves when we get a touch event.
2) If touch resampling is enabled, we check for delayed vsync events (e.g. the main thread can't process touch events fast enough). In these cases, we just send the last touch event.
Attachment #8504427 - Attachment is obsolete: true
Attachment #8505798 - Flags: review?(mwu)
Blocks: 1083530
Comment on attachment 8505798 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread

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

::: gfx/thebes/gfxPrefs.h
@@ +208,5 @@
>    // These times should be in nanoseconds
>    DECL_GFX_PREF(Once, "gfx.touch.resample.max-predict",        TouchResampleMaxPredict, int32_t, 8000000);
>    DECL_GFX_PREF(Once, "gfx.touch.resample.vsync-adjust",       TouchVsyncSampleAdjust, int32_t, 5000000);
>    DECL_GFX_PREF(Once, "gfx.touch.resample.min-resample",       TouchResampleMinTime, int32_t, 2000000);
> +  DECL_GFX_PREF(Once, "gfx.touch.resample.delay-threshold",    TouchResampleVsyncDelayThreshold, int32_t, -20000000);

This needs to be negative due to how the math in checking the threshold is done, which seems wrong. Make this take positive numbers and adjust the math accordingly.

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +146,2 @@
>  {
> +  if (aTouch.mType == MultiTouchInput::MULTITOUCH_MOVE ) {

nit: remove the space before )

@@ +146,4 @@
>  {
> +  if (aTouch.mType == MultiTouchInput::MULTITOUCH_MOVE ) {
> +    MutexAutoLock lock(mTouchQueueLock);
> +    if (mResamplingEnabled || mTouchMoveEvents.empty()) {

I think this would be little bit more clear if there were separate blocks for each condition -

if (mResamplingEnabled) {
  ...
}

if (mTouchMoveEvents.empty()) {
  ...
} else {
  ...
}
Attachment #8505798 - Flags: review?(mwu)
Updated with feedback from comment 8.
Attachment #8505798 - Attachment is obsolete: true
Attachment #8506394 - Flags: review?(mwu)
Comment on attachment 8506394 [details] [diff] [review]
Prevent Touch Events From Piling Up On Main Thread

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

Looks good, thanks.
Attachment #8506394 - Flags: review?(mwu) → review+
Properly formatted commit message. Carrying r+. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5990cd5f7a7b
Attachment #8506394 - Attachment is obsolete: true
Attachment #8506404 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d2cbf090111f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.