Redraw on resize performance for content is pretty bad with e10s

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jimm, Assigned: handyman)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla36
x86_64
Windows 7
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8499125 [details]
screenshot.png

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

Comment 2

3 years ago
Also, we don't use the right background color, which should match the page color. See bing.com for an example.
Assignee: nobody → davidp99
tracking-e10s: ? → m4+
(Reporter)

Comment 3

3 years ago
another good test page - 

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

Comment 4

3 years ago
(Assignee)

Comment 5

3 years ago
Created attachment 8519244 [details] [diff] [review]
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.
Attachment #8519244 - Flags: review?(wmccloskey)
(Reporter)

Comment 6

3 years ago
This patch improves resize perf on win7 substantially.
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.
Attachment #8519244 - Flags: review?(wmccloskey) → review+
Actually, let's hold off landing this. bent wants kats to check it over to make sure the touch event stuff is okay.
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 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.
Attachment #8519244 - Flags: feedback?(bugmail.mozilla)
(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.
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 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
Attachment #8519244 - Flags: feedback?(bugmail.mozilla) → feedback+
(Assignee)

Comment 14

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
'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.

Updated

3 years ago
Blocks: 1096076
(Assignee)

Comment 16

3 years ago
Created attachment 8520098 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages (r+'ed by billm)

Removed compress IPDL option from RealTouchMoveEvent.
Attachment #8519244 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
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...
Flags: needinfo?(davidp99)
(Assignee)

Comment 18

3 years ago
*sigh*.  Wrong patch.
Flags: needinfo?(davidp99)
Keywords: checkin-needed
(Assignee)

Comment 19

3 years ago
Created attachment 8520125 [details] [diff] [review]
Fully consolidate 'compress'ed IPDL messages (r+'ed by billm)

With reverse iterators suggestion.
Attachment #8520098 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 20

3 years ago
Do we have any MDN documentation on this ipdl compress feature? I can't find a thing on it.
Keywords: dev-doc-needed
Can we please run this through Try first? :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a0a30f2ddb2d
https://hg.mozilla.org/mozilla-central/rev/a0a30f2ddb2d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
See Also: → bug 1123762
You need to log in before you can comment on or make changes to this bug.