635145 - Throttle Content:SetCacheViewport calls to not flood the platform

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Content:SetCacheViewport messages trigger the child process to set the displayport, which can cause the shadow layer image to be re-allocated (if the size is different) and causes a content repaint in the parent process.

During pageloads, this can slow down the overall feel of the pageload. During panning, it can try to paint new displays faster than we can render.

This patch adds a simple 100ms throttle/queue for the displayport changes. The overall affect on Tp is a small improvement, but since less time was being taken rendering progressively bigger parts of the page, it seemed to load faster.

I don't have any data for panning, but the patch didn't seem to adversely affect panning.

Thoughts on this concept?

Comment 1

[Matt Brubeck (:mbrubeck)]

Comment on attachment 513366 [details] [diff] [review]
patch

Throttling seems like a good idea, but it also seems likely to add some perceptible delays.  I'd like to identify cases where we could pass a flag to override the delay and do an immediate update.  For example, we might want to update immediately on finishFuzzyZoom, and on "browser.active = true".  Or in reverse - we could just the cases where we want throttling, and pass a flag for those.

How many displayport updates during page load are caused by viewport metadata changes?  Perhaps we should add a timer for those instead (or additionally) - at least for the ones not caused by DOMMetaAdded events.

You could move the timer from the frame script into the binding, to avoid sending the ignored messages completely.

feedback+ but I think this needs a fair bit more investigation before I'd check anything in.

Comment 2

[Benjamin Stover (:stechz)]

Great idea. :)

I'm with Matt. For throttling panning and page load, I suggest doing this in the chrome process. We could add a function called throttle(N), which would make sure updateCacheDisplayport is never called more than once every N msecs. Then we can apply some heuristics:

* Throttle during the first five seconds of page load? Something like 500 msecs might work fine, but then we probably want to disable throttling after some time for pages that load forever due to high latency. Alternatively, we could one-time throttle every time we saw a size change event.

* For panning, we may want to consider throttling every 100 or so milliseconds. Ideally we could combine this patch with the kinetic prediction patch (which already has a similar throttle method) so that we are predicting where we will be in 100 msecs (I'd be happy to do this as a followup!).

* Other times to throttle? Perhaps after a mousedown event so that it can invalidate the touched area quickly without worrying about new displayport stuff!

I think this is a useful idea, though eventually I think we can get displayport fast enough to where we can simplify a lot of stuff.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Updated patch

Fixed some comments, also I keep timer for repeated messages, otherwise we don't update until all messages come (no updates until kinetic scroll ends)

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 533870 [details] [diff] [review]
Updated patch

Sorry I forgot to give feedback on this.  Please re-request feedback or review if this patch is still useful and you want to get it into XUL Fennec.

Comment 5

[BMO Automation]

Closing all opened bug in a graveyard component