1130051 - [e10s] Path Intersections of Paper.js demo is slow/laggy with e10s

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Alice0775 White]

Steps to reproduce:
1. Open http://paperjs.org/examples/path-intersections/
2. Move around mouse pointer so that intersection points display

Actual Results:
It is slow/laggy with e10s

Expected Results:
It should not be slower than non-e10s

Comment 1

[David Parks [:handyman]]

Attachment: Video of current (not compressed) behavior

Comment 2

[David Parks [:handyman]]

Attachment: Video of compressed behavior

Comment 3

[David Parks [:handyman]]

Attachment: Apply IPDL compression to mouse events

NI billm and kats to get opinions on this strategy (since you helped out in bug 1076820).

----------

The situation in this bug is that we are getting "spammed" by a lot of mouse events, which are queueing up in the content process and causing a traffic jam.  This is because the main process isn't very busy and the OS figures that the main process's responsiveness is more important than the content process (a background process), which is working like a dog.

I ran into this with mousemove events in bug 1076820.  There, we were already compressing the messages but it wasn't working well.  Here, we aren't compressing but I'm suggesting we should.  Redundant mousemove events are, I believe, similarly unnecessary.

It should be mentioned that this could create a bad situation where there would have been none in the following case:
1. User moves mouse.  Performance is bad so the mousemove goes into content proc queue.
2. User quickly clicks.  Goes into the queue after the move.  The content proc is still busy with other stuff.
3. User quickly moves mouse away.  The old mousemove is dumped by ‘compress’.
4. The click is processed in the wrong position.

The key thing to note is that this only ever works by coincidence.  e10s makes it *more* likely that this will work because the OS generates mouse events frequently but, in normal uniprocess, the OS would have filtered the mousemove by sending less frequent messages.  So it doesn’t create any new problems — its the same interactive response behavior (vs the batch-processing of a job queue we have now).

For the patch I decided it was unnecessary but we might also want to consider compressing the mousewheel events.  I don’t know what the deal is with touch (Android probably doesn’t have such intense OS balancing).  I’d leave that alone.

Comment 4

[David Parks [:handyman]]

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d27050d954b

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Wouldn't this mean that multiple mouse clicks could be compressed together? It seems like we would need a separate message just for mouse move events.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Given the changes you made in bug 1076820 I think bill is right; this would compress all mouse events instead of just moves, so you'd lose mouse down and up events which is bad. I'm not so worried about the scenario you specified because up/down events have their own coordinates and shouldn't get relocated if move events are dropped. But I do still worry that this might impact other scenarios; maybe dragging use cases where content only checks for move events and doesn't account for movement between the last move and the mouseup? It might be worth exploring botond's proposal at [1] as it would provide a more granular control over the compression mechanism.

[1] https://lists.mozilla.org/pipermail/dev-platform/2014-March/003708.html

Comment 7

[David Parks [:handyman]]

Mouse clicks are definitely mishandled in the patch -- I wanted to get opinions on the idea before working out the kinks.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> But I do still worry that this
> might impact other scenarios; maybe dragging use cases where content only
> checks for move events and doesn't account for movement between the last
> move and the mouseup? It might be worth exploring botond's proposal at [1]
> as it would provide a more granular control over the compression mechanism.

I think that dragging would work similar to clicking -- as you said, the mouse-up would have a position as well.  What's more, the compression always dumps the oldest mouse move, so the 'movement between last move and mouseup' would never be compressed out.   

The proposal for adding predicates to the compress flag is interesting.  For this, tho, I'd handle mouse clicks as Bill said -- simply break moves/clicks into two IPDL messages.  Its much simpler.  If dragging is ok with compression then I can't think of any other conditions under which I would want the predicate not to compress.

I'll put together a patch breaking apart the IPDL messages unless there are still concerns.

(NI is just so that you see this.)

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Parks [:handyman] from comment #7)
> I think that dragging would work similar to clicking -- as you said, the
> mouse-up would have a position as well.  What's more, the compression always
> dumps the oldest mouse move, so the 'movement between last move and mouseup'
> would never be compressed out.   

I'm not sure I follow this. The situation I was referring to is as follows:

mousedown @y=10
mousemove @y=15
mousemove @y=20
mousemove @y=25
mouseup   @y=25
mousemove @y=30

In this case the mousemoves between the down and up would get compressed away. Technically mouse listeners in the page could still detect that it's a "drag" because the y-coordinate of the down and up are different, but in practice they might not do that. If they don't then they wouldn't detect this as a drag. For the same reason whenever we synthesize a click event we make sure to send a move event just before the down/up (e.g. [1][2]), because there are lots of webpages out there that assume mousemove events will get sent when the mouse moves, and they do handling in the mousemove handler that is important.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=20729b28eb1e#2427
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=5321b4fff1ae#5113

Comment 9

[David Parks [:handyman]]

OK, I think I get it.  Because a lot of pages are badly written, we can't just dump unneeded mousemoves indiscriminately.  But I think we are still safe here for a few reasons:

* It looks like the fake mousemoves you mentioned are inserted after the IPC communication ([1] was in TabChild and [2] actually didn't fake a move in-between... its a move before the mousedown).

* If IPDL compressed mousemoves and the result was a mousedown followed by a mouseup with no mousemove in between, we could generate the fake mousemove at the mouseup x,y position in the RecvRealMouseEvent handler itself (this is a simple condition to detect).  Then we don't need the other fake mousemove events.

* This is all happening during a period where the content process is stalled (otherwise there is no issue) so the user isn't getting any cues that the mouse action is being handled.  A big drag could be interpreted as a click if it all takes place during this stall but that can happen in a polling situation like with the mouse.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

What if we take out the changes in bug 1076820? I think bug 1096076 also fixes the problem bug 1076820 was meant to solve, so I don't think we need bug 1076820 anymore.

And that way we'd only be compressing mousemove events that happen without any other intervening events. So the problem kats is worried about wouldn't happen.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to David Parks [:handyman] from comment #9)
> A big drag could be interpreted as a click
> if it all takes place during this stall but that can happen in a polling
> situation like with the mouse.

At the risk of saying something silly due to not reading the context: This could only happen if the mouseup happened at or very near the same coordinates as the mousedown, right? Since a common way to prevent yourself from clicking something at the last moment (after mousedown) is to move the mouse cursor somewhere else before you let go (mouseup), and any big drag would have to end up back near the starting coordinates to be interpreted as a click, it doesn't sound like a problem to me.

Comment 12

[David Parks [:handyman]]

(In reply to Bill McCloskey (:billm) from comment #10)
> What if we take out the changes in bug 1076820? I think bug 1096076 also
> fixes the problem bug 1076820 was meant to solve, so I don't think we need
> bug 1076820 anymore.

Sounds right.  I wasn't crazy about the old semantics of 'compress' (they are not obvious) and I don't think there is a real problem here but restoring the old compress behavior is not much effort and might still fix the bug.  The only way it could fail is if the mousemove events are not contiguous (there could be IPDL messages between them but I have no idea if there usually actually are).


(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #11)
> At the risk of saying something silly due to not reading the context: This
> could only happen if the mouseup happened at or very near the same
> coordinates as the mousedown, right? Since a common way to prevent yourself
> from clicking something at the last moment (after mousedown) is to move the
> mouse cursor somewhere else before you let go (mouseup), and any big drag
> would have to end up back near the starting coordinates to be interpreted as
> a click, it doesn't sound like a problem to me.

Yes, that is the first situation kats mentioned.  Its less worrysome than his second example of busted web sites incorrectly interpreting the events anyway.  For that, we'd need something like either changing the definition of 'compress' or generating fake mousemoves.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to David Parks [:handyman] from comment #9)
> * If IPDL compressed mousemoves and the result was a mousedown followed by a
> mouseup with no mousemove in between, we could generate the fake mousemove
> at the mouseup x,y position in the RecvRealMouseEvent handler itself (this
> is a simple condition to detect).  Then we don't need the other fake
> mousemove events.

If you did this it should resolve the concern I had. There might still be issues we run into but I can't think of any specifically, so I have no further objections.

You can also back out bug 1076820 if you decide to go that way, but if you do, please don't restore the "compress" keyword for touchmove events. We might be ok restoring that but I'd rather defer doing that until we need it.

Comment 14

[David Parks [:handyman]]

Attachment: 1. Restore old compress semantics

We are going to go with the solution of restoring the old compress semantics and compressing the mouse moves.

Comment 15

[David Parks [:handyman]]

Attachment: 2. Compress the mousemove messages

Comment 16

[David Parks [:handyman]]

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f4a36a89b5d

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8563590 [details] [diff] [review]
2. Compress the mousemove messages

This looks fine to me, but kats should probably review it

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8563589 [details] [diff] [review]
1. Restore old compress semantics

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

I wanted to make sure that window resizing didn't regress from this, and it doesn't.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8563590 [details] [diff] [review]
2. Compress the mousemove messages

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

::: dom/ipc/TabChild.cpp
@@ +2160,5 @@
>  
>  bool
> +TabChild::RecvRealMouseMoveEvent(const WidgetMouseEvent& event)
> +{
> +  WidgetMouseEvent localEvent(event);

I'd rather you just called RecvRealMouseButtonEvent directly rather than duplicating the code. We do the same thing for touch-moves (although now that we're not compressing touchmoves we can probably just fold those two methods into one... but I digress).

Comment 20

[David Parks [:handyman]]

Attachment: 2. Compress the mousemove messages (r+ed by kats)

As suggested by kats...

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/416a3c508df2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5931982fa54b

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/416a3c508df2
https://hg.mozilla.org/mozilla-central/rev/5931982fa54b