788943 - Make parent processes consume less CPU time when delivering touch series to child processes

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: PoC patch

See bug 780341 comment 8.  The parent process is spending about 1/3 of its wall-clock time chewing CPU while uselessly going through the full DOM event dispatch machinery, only to forward along the event to the focused child process at the end.

The attached (hacky! demo!) patch introduces the notion of a "capturing TabParent".  During a [ touchstart, ..., touchend ] series, the TabParent consumes all the events, and we completely bypass DOM dispatch.  This patch has a number of problems so don't take it too seriously.

This wins about 10fps on the homescreen.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I meant to add, this is a very heavy and blunt instrument.  We may be able to get the same effect by caching display lists, the construction of which is about half the CPU time the master process consumes.

Comment 2

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

Ah, interesting. Sending event from gonk layer to child. I don't know how hit testing works.
What if there is some UI in gonk, like some in-process app?
Or is it decided that there will never be any UI. If that is the case, then this could be ok.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In a "real" version of this patch, which I'll try to code up tonight, we would do the following
 - go through normal dispatch path for touchstart, including style flush and display list hit-testing
 - if the touchstart targets <iframe remote>, set the TabParent target to capture the remainder of the series
 - directly send subsequent touchmove to the TabParent through fast path
 - on touchend, go through normal dispatch path to make sure event state is coherent
 - unset capturing TabParent

The effect of this approach would be essentially to iframe.setCapture() on behalf of the <iframe remote>, IIUC.

There will be in-process UI for v1, the system app (status bar etc.) and browser UI, but those apps will just have to deal with whatever semantics we choose to have fast event dispatch to all other apps (which will all be OOP).

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This takes the picture<->picture panning fps in the gallery app from ~24fps up to ~48fps.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> The effect of this approach would be essentially to iframe.setCapture() on
> behalf of the <iframe remote>, IIUC.

Thinking about it I believe this is actually the correct behavior per the spec: http://www.w3.org/TR/touch-events/#the-touchmove-event

"The target of this event must be the same Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of the target element."

In addition you could have a touchmove listener in the parent that calls .stopImmediatePropagation() to get it even closer to the behavior of your PoC. Although the cost of running the listener could be higher than if the listener didn't exist and it just bubbled back

Comment 6

[:Felipe Gomes (needinfo for replies!)]

> Thinking about it I believe this is actually the correct behavior per the spec:
> http://www.w3.org/TR/touch-events/#the-touchmove-event

I meant to add: for 1 input point. For more than 1 each touch point captures on its starting element, which might not be the same.. However if the iframe is generally covering the entire screen it might always be the case

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This wins about 15fps in google maps, interestingly enough.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I have a "real" patch for this, and it works great ... except for async pan/zoom :/.  Trying a debug build to see what's going awry.  Might need to phone a friend.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't see any assertions or warnings in a debug build.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Allow TabParents to capture event series for faster dispatch to subprocesses. Implements capturing of touch-event series

This causes intermittent issues with (in-process) apzc, but I'd like to get the meat of this patch going through review asap.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Allow TabParents to capture event series for faster dispatch to subprocesses. Implements capturing of touch-event series, v1.1

Fixes a bug with multiple touch points released out of the order they're pressed.  apzc is happy again.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This patch gets the b2g (main thread) to 99% idle while panning around.

Comment 13

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

Comment on attachment 659345 [details] [diff] [review]
Allow TabParents to capture event series for faster dispatch to subprocesses. Implements capturing of touch-event series, v1.1

>+bool
>+TabParent::TryCapture(const nsGUIEvent& aEvent)
>+{
>+  MOZ_ASSERT(sEventCapturer == this && mEventCaptureDepth > 0);
>+
>+  if (aEvent.eventStructType != NS_TOUCH_EVENT) {
>+    // Only capture of touch events is implemented, for now.
>+    return false;
>+  }
>+
>+  nsTouchEvent event(static_cast<const nsTouchEvent&>(aEvent));
>+
>+  bool isTouchPointUp = (event.message == NS_TOUCH_END ||
>+                         event.message == NS_TOUCH_CANCEL);
>+  if (event.message == NS_TOUCH_START || isTouchPointUp) {
>+    // Let the DOM see touch start/end events so that its touch-point
>+    // state stays consistent.
>+    if (isTouchPointUp && 0 == --mEventCaptureDepth) {
>+      // All event series are un-captured, don't try to catch any
>+      // more.
>+      sEventCapturer = nullptr;
>+    }
>+    return false;
>+  }
>+
>+  // Adjust the widget coordinates to be relative to our frame.
>+  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+  nsEventStateManager::MapEventCoordinatesForChildProcess(frameLoader, &event);
>+
>+  SendRealTouchEvent(event);
>+  return true;
>+}
This all could use some testing in case app is changed while handling touch events.
What if focus is moved to another app?
I'm little worried about mEventCaptureDepth getting out of sync.
Thinking also about cases when after first touchdown/move the underlying app crashes, yet another touchdown/move is initiated before touchup for the first touch.
So... I think we should keep the capturing TabParent alive until its mEventCaptureDepth is back to 0.
Make sEventCapturer StaticRefPtr. Then you wouldn't need to add stuff to TabParent::ActorDestroy, though perhaps there needs to be a check
in SendRealTouchEvent to actually try to send the event to child if there is a child.


(Looks like Gonk doesn't have any consistent coding style.)

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 659345 [details] [diff] [review]
> Allow TabParents to capture event series for faster dispatch to
> subprocesses. Implements capturing of touch-event series, v1.1
> 
> >+bool
> >+TabParent::TryCapture(const nsGUIEvent& aEvent)
> >+{
> >+  MOZ_ASSERT(sEventCapturer == this && mEventCaptureDepth > 0);
> >+
> >+  if (aEvent.eventStructType != NS_TOUCH_EVENT) {
> >+    // Only capture of touch events is implemented, for now.
> >+    return false;
> >+  }
> >+
> >+  nsTouchEvent event(static_cast<const nsTouchEvent&>(aEvent));
> >+
> >+  bool isTouchPointUp = (event.message == NS_TOUCH_END ||
> >+                         event.message == NS_TOUCH_CANCEL);
> >+  if (event.message == NS_TOUCH_START || isTouchPointUp) {
> >+    // Let the DOM see touch start/end events so that its touch-point
> >+    // state stays consistent.
> >+    if (isTouchPointUp && 0 == --mEventCaptureDepth) {
> >+      // All event series are un-captured, don't try to catch any
> >+      // more.
> >+      sEventCapturer = nullptr;
> >+    }
> >+    return false;
> >+  }
> >+
> >+  // Adjust the widget coordinates to be relative to our frame.
> >+  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> >+  nsEventStateManager::MapEventCoordinatesForChildProcess(frameLoader, &event);
> >+
> >+  SendRealTouchEvent(event);
> >+  return true;
> >+}
> This all could use some testing in case app is changed while handling touch
> events.
> What if focus is moved to another app?

Based on felipe's link above, touch events are already supposed to behave like setCapture() event series.  It's certainly possible there are bugs in our code to support that, but since this patch defers touchstart/touchend/touchcancel to normal DOM dispatch, it can't make any such bugs any worse.

> I'm little worried about mEventCaptureDepth getting out of sync.

It's only incremented/decremented by full DOM dispatch.  If it gets out of sync, we have a preexisting bug.

> Thinking also about cases when after first touchdown/move the underlying app
> crashes, yet another touchdown/move is initiated before touchup for the
> first touch.

This patch handles crashed apps: we fall back on normal DOM dispatch if an app crashes.  Again, it's possible that app crashes will result in buggy behavior, but this patch can't make that any worse.

> So... I think we should keep the capturing TabParent alive until its
> mEventCaptureDepth is back to 0.

If we could do that (which I'm not sure about), I don't see how it would ameliorate any potential problems.  Can you explain what you think this would help fix?

> Make sEventCapturer StaticRefPtr. Then you wouldn't need to add stuff to
> TabParent::ActorDestroy, though perhaps there needs to be a check
> in SendRealTouchEvent to actually try to send the event to child if there is
> a child.
> 

We still need to unset the capturing tabparent in ActorDestroy, because a crash means the tabparent can't process any more events.  What problem are you concerned about?

Comment 15

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

I'm worried about the problem that child process A gets 2 touchdowns, 1 touch up and then crashes.
Then process B starts to listen for touch events, and there is 3rd touchdown and two touch ups. Which process gets those 2 touch ups? B should get only 1, because it sees only one touchdown.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I couldn't tell you what would happen in that case, but I would like to point out again that the patch here doesn't affect that behavior at all.  I suggest we work this out in a new bug (if gecko is doing something wrong).

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is green on try, ready to land.

smaug, did we reach an understanding on IRC about this code, and that we should check behavior of crashing processes in a separate bug?  I'm not sure.

Comment 18

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

No we didn't, since I think adding staticrefptr should be done in this bug.

Comment 19

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

But I'm not sure if even that is enough. I need to check still how touchdown is handle in presshell.
If we end up dispatching one touchdown to iframe A (process A') and another one to iframe B (process B'), then touchmoves related to A' may end up to B', I think.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #18)
> No we didn't, since I think adding staticrefptr should be done in this bug.

What problem does that fix?

Comment 21

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

I'm trying force events from one touch event session to go to just one process so that other
processes can't get spurious events.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For event series that don't introduce new touch points after a crashed subprocess, I suspect the current code would Just Work (for what you want) because
 - the behavior in the DOM code should be like setCapture(), per comment 5
 - the DOM dispatch code should hold a strong reference to the <iframe remote> even after its subprocess crashes
 - attempting to dispatch an event to that <iframe remote> will be a no-op, because its TabParent will go away after the crash

For the case where a new touch point in the same series is introduced after a crash, making the change you request probably won't change anything, because we purposely defer to the full DOM dispatch for touchstart/touchcancel/touchend.  There are two possibilities for that new touch point
 - the new touchstart targets the original target (the <iframe remote> that crashed), because it's still the ~setCapture() handler.  In that case, the touchmove events are no-ops per above.
 - the new touchstart targets whatever new iframe is displayed after the crash, and it will be the target for touchmove.  AFAIK we send all touch points with each touchmove, so AFAIK the behavior should be no different with this patch or without it.

So I still don't see why keeping the TabParent around after it's died would make a difference.  If there's a bug to be fixed here, I think it needs to be fixed in the DOM dispatch code: we maybe should keep the original *<iframe remote>* target alive as the setCapture() target after its subprocess dies, but not affect the TabParent's lifetime.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7be12850db1a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7be12850db1a