1076820 - Redraw on resize performance for content is pretty bad with e10s

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) enable e10s
2) open bing.com or bugzilla some other darker page
3) starting with a smaller window, expand the size of the window using the lower right window corner.

result: You'll see content repaint very slowly.
expected: try the same in a non-e10s window to compare.

Comment 1

[Jim Mathies [:jimm]]

Attachment: screenshot.png

It's so slow you have time to snap screen shots of content scrunched up into a corner of the window. :)

Comment 2

[Jim Mathies [:jimm]]

Also, we don't use the right background color, which should match the page color. See bing.com for an example.

Comment 3

[Jim Mathies [:jimm]]

another good test page - 

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

Comment 5

[David Parks [:handyman]]

Attachment: Fully consolidate 'compress'ed IPDL messages

A 100% STR:

* Open an e10s window
* Open two tabs, preferably using sites with poor reflow/restyle performance (the pages mentioned above in this bug work).  Make sure you visit both tabs.  Its not enough to 'session-restore' them without viewing them.
* Start resizing by grabbing the bottom-right window-corner and moving in a circular motion.

Expected result: The application will redraw at a reasonable rate as you resize.

Actual result: Redraw is fair while resizing but, when you release the mouse, it takes a long time to stop reflowing.
-------

"compress"ed IPDL messages only matched against the back of the message queue.  We therefore process tons of superfluous window-resize events (ones that the user had already overrode by continuing to resize).  Each resize means a reflow/restyle.  Because of this, performance is terrible if more than one tab was open.

This patch fixes the issue by looking at all IPDL messages in the queue.

Comment 6

[Jim Mathies [:jimm]]

This patch improves resize perf on win7 substantially.

Comment 7

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

Comment on attachment 8519244 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages

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

Thanks, this is a great fix. It does change the meaning of |compress| a little bit, but I don't think it should be a problem in practice. The sorts of messages you would mark as |compress| tend to be "best effort" sorts of things.

::: ipc/glue/MessageChannel.cpp
@@ +584,5 @@
>  }
>  
> +// Predicate that reports messages that should be consolidated if 'compress' is set.
> +class MatchingKinds {
> +  typedef IPC::Message Message;

For some reason this file uses 4-space indent, so let's match that.

@@ +621,4 @@
>      if (compress) {
> +        // Check the message queue for another message of this type.
> +        // If we find it, hold onto the iterator.
> +        MessageQueue::iterator it = std::find_if(mPending.begin(), mPending.end(),

This seems like a good place to use |auto| instead of |MessageQueue::iterator|. I don't think the extra type info is helping anyone.

@@ +621,5 @@
>      if (compress) {
> +        // Check the message queue for another message of this type.
> +        // If we find it, hold onto the iterator.
> +        MessageQueue::iterator it = std::find_if(mPending.begin(), mPending.end(),
> +          MatchingKinds(aMsg.type(), aMsg.routing_id()));

Please indent this to line up with the opening paren on the previous line.

Comment 8

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

Actually, let's hold off landing this. bent wants kats to check it over to make sure the touch event stuff is okay.

Comment 9

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

Comment on attachment 8519244 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages

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

It would be good to get this tested in TestLatency.cpp:
http://mxr.mozilla.org/mozilla-central/source/ipc/ipdl/test/cxx/TestLatency.cpp#149
Maybe add some intervening message (that could also be compressed).

::: ipc/glue/MessageChannel.cpp
@@ +621,4 @@
>      if (compress) {
> +        // Check the message queue for another message of this type.
> +        // If we find it, hold onto the iterator.
> +        MessageQueue::iterator it = std::find_if(mPending.begin(), mPending.end(),

Ben also pointed out that it would be better to use rbegin and rend so that we search from the end of the deque, where the message is more likely to be found.

Comment 10

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

Comment on attachment 8519244 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages

kats, could you please look this over and see if it's okay? It changes how touch events are processed when the content process is running slowly. Currently, a "touch move" event is thrown away if it hasn't been processed before a new one arrives and no other messages have arrived in between.

This patch changes things so that we throw away the old "touch move" message even if some other messages arrived in the middle.

Here's an example. In both designs, if ten touch move events arrive in a row, and we haven't had a chance to process any of them, then we'll throw away all but the last one. In the existing code, if a different kind of message arrived in between each touch move message, then we wouldn't throw anything away. In the new code, we would still throw away all but the last touch move event.

Do you know if this would affect APZC at all? Do you think it's likely it would break anything? One thing we're not clear on is whether it's typical to get lots of touch events in a row or if there are usually other messages in between.

Comment 11

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

(In reply to Bill McCloskey (:billm) from comment #10)
> Do you know if this would affect APZC at all? Do you think it's likely it
> would break anything?

Most of the APZ code itself runs in the parent process and gets the touch events before they go over to the child process, so it would still see the full stream of events. However, I can think of at least one scenario which might be adversely impacted by this. In order to support the touch events spec, the APZ code has to wait for web content listeners on the touchstart and first touchmove event to run to know if they called preventDefault(). This patch means that the "first touchmove" might be severely delayed because of the new compress behaviour. This might increase latency to start of panning, for example, or trigger panning even though otherwise the content might have cancelled it.

> One thing we're not clear on is whether it's typical
> to get lots of touch events in a row or if there are usually other messages
> in between.

I think we get both scenarios happening - touch events in a row as well as interleaved with other stuff. It's not clear to me how much of an impact this change will have in practice, to be honest, but it certainly has the potential for introducing regressions. I would suggest try other lower-impact alternatives like sending back an ack on the resize event and basically implementing the "compress" behaviour in code. If you do want to make this change then I would really like a pref to toggle it so that if people start noticing wonky behaviour they can see if it's a result of this.

Comment 12

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

Actually - I'd be ok with this change if you also remove the compress keyword from the touchmove message. I'm pretty sure that we're going to do compression and interpolation of touchmove events higher up in the stack anyway (at least on B2G, which is all I care about at the moment) so we should be fine not doing it PBrowser.idl.

Comment 13

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

Comment on attachment 8519244 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages

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

f+ if you remove touchmove compression from PBrowser.ipdl

Comment 14

[David Parks [:handyman]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> Comment on attachment 8519244 [details] [diff] [review]
> Fully consolidate 'compress'ed IPDL messages
> 
> Review of attachment 8519244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ if you remove touchmove compression from PBrowser.ipdl

I was going to propose creating a 'fullcompress' option that just supplemented 'compress' but, if you really don't need it, I'll just cut it out.  Thanks

Comment 15

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

'fullcompress' can be plan B if we run into problems with this approach. I considered that too but it seemed like overkill since so few messages use this in the first place.

Comment 16

[David Parks [:handyman]]

Attachment: Fully consolidate 'compress'ed IPDL messages (r+'ed by billm)

Removed compress IPDL option from RealTouchMoveEvent.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8520098 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages (r+'ed by billm)

This still walks through mPending in the wrong direction...

Comment 18

[David Parks [:handyman]]

*sigh*.  Wrong patch.

Comment 19

[David Parks [:handyman]]

Attachment: Fully consolidate 'compress'ed IPDL messages (r+'ed by billm)

With reverse iterators suggestion.

Comment 20

[Jim Mathies [:jimm]]

Do we have any MDN documentation on this ipdl compress feature? I can't find a thing on it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try first? :)

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/a0a30f2ddb2d

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0a30f2ddb2d