Closed Bug 741228 Opened 8 years ago Closed 8 years ago

Gesture:ShowPress handling takes too long

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 2 obsolete files)

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).

Note also that in cases where the user puts their finger down and starts panning right away, the ShowPress is immediately cancelled by a Gesture:CancelTouch, so the time spent processing ShowPress is almost completely wasted. It is only not-wasted when the user puts their finger down on a scrollable subdocument, because it that case we want to execute the part of the ShowPress handler that sends back the Panning:Override event.

Anyway, I'm not sure what we can do here to improve this, but we need to do something as it is a significant cause of checkerboarding when a user initially starts panning or flinging.
I want to switch this to use touch events instead of ShowPress. That will remove the need for some of our elementFromPoint calls, which may help?
Yeah, that should definitely help. Most of the time is being spent in those calls; the handler doesn't do much else.
Cc'ing jrmuizel as well, he was saying that there is slowness before we even start animating a double-tap zoom, which might also be caused by slow point->element code.
Most of the time spent in Gesture:ShowPress handling is actually in the getClosest() function. After looking over this code, I feel like the test case I'm using (timecube.com) is pretty much the worst-case scenario for this code, but taps will almost always land in a spot that is not clickable but has lots of elements within the touch rect area.

Anyway, this patch takes advantage of the fact that we call isElementClickable on a list of elements that are in close proximity to each other, and likely share common ancestors. By caching some elements that have already failed the clickability test, we can cut the runtime of getClosest() by about 40%.
Attachment #611916 - Flags: review?(wjohnston)
Assignee: nobody → bugmail.mozilla
blocking-fennec1.0: --- → ?
Comment on attachment 611916 [details] [diff] [review]
Cache some results from isElementClickable to speed up getClosest() execution

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

I think this is fine. Maybe add a comment short explaining that we're using it to optimize the second call below.
Attachment #611916 - Flags: review?(wjohnston) → review+
blocking-fennec1.0: ? → beta+
Attached patch Patch - Use touchevents (obsolete) — Splinter Review
This triggers the tap highlight drawing on touchstart. That saves us some work in simple cases, but if you didn't exactly tap on an item we still have to do the find-nearby-elements stuff. I wrapped it in a setTimeout as well to try and give a little break to Gecko, but I seriously doubt that helps much.
Attachment #612327 - Flags: review?(bugmail.mozilla)
Comment on attachment 612327 [details] [diff] [review]
Patch - Use touchevents

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

So my main concerns:
1) The ShowPress stuff was cancelable by content JS if it called preventDefault on the down event (because the event would never make it to PanZoomController); the new touchstart hook does not appear to be.
2) Does registering a touch listener force us into a slower code path? i.e. maybe if we have no touch listeners, the mWaitForTouchListeners in Java is false and we can simply not send touch events to begin with. I'd like to understand the full implications of this, as I don't know what the mWaitForTouchListeners flag is actually based on.
3) In the old ShowPress version, the browser.js observe function has a condition where if !BrowserApp.isBrowserContentDocumentDisplayed(), we throw away the ShowPress event. We should preserve that condition in the new touchstart handler.

::: mobile/android/chrome/content/browser.js
@@ +2324,5 @@
> +  handleEvent: function(aEvent) {
> +      let closest = aEvent.target;
> +
> +      setTimeout((function() {
> +        if (aEvent.touches.length > 1)

Is the setTimeout here just so that we run this part after kicking off the draw? If so I'm not sure that will help, I noticed a few NATIVE_POKE events running before the DRAW event, and one of those will probably run this setTimeout. Even if we got rid of those, we'd have to guarantee that it doesn't regress, which I think will be very tricky. Might as well just leave out the setTimeout and have it run directly.

@@ +2332,5 @@
> +          closest = ElementTouchHelper.elementFromPoint(BrowserApp.selectedBrowser.contentWindow,
> +                                                        aEvent.changedTouches[0].screenX,
> +                                                        aEvent.changedTouches[0].screenY);
> +          if (!closest)
> +            closest = aEvent.target;

Either this if condition is mis-indented, or the outer if needs to have braces. I assume the latter.
Attached patch Patch v2 (obsolete) — Splinter Review
I'm not sure this provides a huge benefit either, but if you can compare whatever you were using kats, it would be nice to know.

(In reply to Kartikaya Gupta (:kats) from comment #7)
> So my main concerns:
> 1) The ShowPress stuff was cancelable by content JS if it called
Whoops. I had a check in here for this, but lost it.

> 2) Does registering a touch listener force us into a slower code path? i.e.
Nope. Those are tied to the tab that fired. In this case, there is no tab and we bail out of notifying java about the listener.

> 3) In the old ShowPress version, the browser.js observe function has a
> condition where if !BrowserApp.isBrowserContentDocumentDisplayed(), we throw
Moved this up with the other check. Thanks.

> Is the setTimeout here just so that we run this part after kicking off the
That was the hope. Get out of the way if we can, but I'm not surprised if it doesn't help. Removed it.
Attachment #612327 - Attachment is obsolete: true
Attachment #612327 - Flags: review?(bugmail.mozilla)
Attachment #612417 - Flags: review?(bugmail.mozilla)
Cool, updated patch looks good. I'll run the timing stuff I had before r+'ing though.
So I applied this patch and it's marginally faster than the ShowPress code we had before, so that's good. However, I'm seeing that the touchstart handler doesn't get run some of the time. It happens both on taps and touch-and-drags, but only intermittently. I'm not really sure why this is; I don't believe the page is intercepting the event, I think it's more likely an exit condition in the nsPresShell.cpp code. I tried tracing through with gdb but it's hard to follow.
Comment on attachment 612417 [details] [diff] [review]
Patch v2

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

r+, but we should figure out why the touchstart event handler doesn't always get triggered when a finger goes down before landing this.
Attachment #612417 - Flags: review?(bugmail.mozilla) → review+
Yeah. I'm chasing that in bug 741247. That problem seems related to zoom level based on my testing. I'll try to post stuff I see over there. Any help understanding how zoom works with nsFrame's would be helpful. I'll hold off on landing this until I've solved things there.

Can you point me to what pages you were seeing problems on?
I was mostly testing on timecube.com, nytimes.com, and cnn.com. I'd often see the touchstart not get triggered when flinging the page on timecube.com, and occasionally on nytimes.com as well. Most of the time it would happen while flinging the page upwards (i.e. scrolling down), I don't think I ever saw it happening while flinging downwards. I also saw the touchstart not get triggered when tapping in some places on timecube, usually the spots I was tapping on were to the sides of the main block of text. There were a few spots that seemed to be fairly reproducible, although tapping a few pixels away from those spots would trigger the touchstart fine.
I landed the first patch on inbound since I'm not sure how long it will take to figure out the touch handler problem and I want to keep moving forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a736b04b47f
Whiteboard: [leave open after inbound merge]
(In reply to Kartikaya Gupta (:kats) from comment #14)
> I landed the first patch on inbound since I'm not sure how long it will take
> to figure out the touch handler problem and I want to keep moving forward.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9a736b04b47f

Closing this one, please file another bug for follow up bug
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Spun off bug 744518 for the second half of this.
No longer depends on: 741247
Whiteboard: [leave open after inbound merge]
Comment on attachment 612417 [details] [diff] [review]
Patch v2

Patch moved to bug 744518.
Attachment #612417 - Attachment is obsolete: true
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.