1241371 - UpdateImageVisibility eats up ~5ms/frame when scrolling Facebook

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Bobby Holley (:bholley)]

From BenWa's email:

> I just grabbed a fresh profile:
> http://people.mozilla.org/~bgirard/cleopatra/#report=d9220af29d60db5d313235b76718356e1cfec705&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A297026851,%22end%22%3A297028198%7D%5D&selection=0,2212,2213,2214,2215,1932,2216,8,9,10,11,12,13,14,15,16,17,88,89,90,91,107,108,109,167,168,169,170,171,172,2217,2218,2219,2220,355,356,357,358,363

> STR:
> 0) Install FF nightly with the profiler addon. I did this on mac: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler
> 1) Create a bookmark of scroll-test on this page: https://bug894128.bmoattachments.org/attachment.cgi?id=776049
> 2) Visit a FB news feed
> 3) Select the bookmark and use this input: '7000, 200, 120' (200px/sec for 7secs)
> 4) Hit analyze on the profiler

On the profile linked above, click in the "Content" graph, then right-arrow your way down the call graph until the 84.3% breaks into 65.1% and 13.7%. The latter ends up in PresShell::UpdateImageVisibility, and if you click the arrow on that link in the tree, you'll get a view with just that work highlighted.

Comment 1

[Bobby Holley (:bholley)]

Timothy said he'd have a look here.

Comment 2

[Timothy Nikkel (:tnikkel)]

Two problems here, regressions from bug 1169881 and bug 1002992.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Properly update mLastUpdateImagesPos

This is a regrettable mistake that we should have caught sooner.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: Schedule image visibility update much less often when display port changes

I'm aware there are currently some bugs open on the displayport changes slightly when we don't want it to, but I still think we want to make this change as the kind changes in the displayport we want to be sensitive to for this are of the rather large variety.

These heuristics are mostly made up by me because they seem reasonable, happy to tweak them.

Comment 5

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06c7dc28592

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

r=mats

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8710272 [details] [diff] [review]
Schedule image visibility update much less often when display port changes

>+    if ((newDisplayPort.width/((float)oldDisplayPort.width) > 2.f) ||

Nit: add spaces around the / operator please.

Comment 8

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

Comment on attachment 8710272 [details] [diff] [review]
Schedule image visibility update much less often when display port changes

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

Doesn't GetDisplayPortImpl return a displayport that's tile-aligned? I feel like if that changes then it's probably ok to do an update of the image visibility. So maybe a lot of these heuristics can be removed, and we can just compare the displayports that come out of GetDisplayPortImpl before after setting the margins?

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> come out of GetDisplayPortImpl before after setting the margins?

before *and* after

Comment 10

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

Comment on attachment 8710272 [details] [diff] [review]
Schedule image visibility update much less often when display port changes

Flagging myself for review again. Based on discussion with tn we don't want to depend on tiling behaviour because if tiling got disabled this would silently start failing again.

Comment 11

[Pulsebot]

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

Comment 12

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

Comment on attachment 8710272 [details] [diff] [review]
Schedule image visibility update much less often when display port changes

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

So with this patch, if the displayport moves by 1 pixel every time the margins are updated, and the margins are updated 1000 times, the displayport will have moved 1000 pixels without scheduling an update. On the other hand if it moves 1000pixels in a single call, it may very well schedule an update, depending on the base rect. Is that intentional? It seems to me like you would want to schedule an update if the displayport has changed significantly from *the last time the update was done*, rather than what the displayport was just before the margins were updated.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> So with this patch, if the displayport moves by 1 pixel every time the
> margins are updated, and the margins are updated 1000 times, the displayport
> will have moved 1000 pixels without scheduling an update. On the other hand
> if it moves 1000pixels in a single call, it may very well schedule an
> update, depending on the base rect. Is that intentional? It seems to me like
> you would want to schedule an update if the displayport has changed
> significantly from *the last time the update was done*, rather than what the
> displayport was just before the margins were updated.

Yeah, that's a good point. Although since the displayport is relative to the scrolled frame (ie when we scroll it doesn't move) a long series of small changes in the same direction is quite unlikely. And if we get 1000 updates to the displayport, it's likely we've also scrolled 1000 times, and we already have logic to schedule an image visibility update after "enough" scrolling.

I'll change it to "store and compare to the last update displayport".

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: Schedule image visibility update much less often when display port changes v2

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #7)
> Comment on attachment 8710272 [details] [diff] [review]
> Schedule image visibility update much less often when display port changes
> 
> >+    if ((newDisplayPort.width/((float)oldDisplayPort.width) > 2.f) ||
> 
> Nit: add spaces around the / operator please.

Changed this locally after I uploaded the latest patch.

Comment 16

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

Comment on attachment 8710809 [details] [diff] [review]
Schedule image visibility update much less often when display port changes v2

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

Thanks. Note that if you rearrange the division conditions to be multiplications (x/y > 2) can be converted to (x > 2*y) it would let you get rid of the 0 comparison. Up to you if you want to do that.

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> Thanks. Note that if you rearrange the division conditions to be
> multiplications (x/y > 2) can be converted to (x > 2*y) it would let you get
> rid of the 0 comparison. Up to you if you want to do that.

That's much better!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0947272393af

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7a7def04840d
https://hg.mozilla.org/mozilla-central/rev/0947272393af

Comment 20

[William Lachance (:wlach)]

Some nice improvements on tp5o scroll from this: https://treeherder.allizom.org/perf.html#/alerts?id=1865&framework=1&hideImprovements=0

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to William Lachance (:wlach) from comment #20)
> Some nice improvements on tp5o scroll from this:
> https://treeherder.allizom.org/perf.html#/
> alerts?id=1865&framework=1&hideImprovements=0

Great! Hopefully this also means that a similar regression will be caught in the future.

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

Approval Request Comment
[Feature/regressing bug #]: bug 1002992
[User impact if declined]: slower scrolling
[Describe test coverage new/current, TreeHerder]: talos notified of the improvement (talos is our test coverage for perf things)
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8710809 [details] [diff] [review]
Schedule image visibility update much less often when display port changes v2

Approval Request Comment
[Feature/regressing bug #]: bug 1169881
[User impact if declined]: slower scrolling
[Describe test coverage new/current, TreeHerder]: talos has much scroll coverage for perf things
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 24

[Bobby Holley (:bholley)]

To chime in here - this is an enormous (~8%) scrolling perf win (on stuff like Facebook) that we can get to the hands of users 6 weeks earlier by uplifting to aurora.

Comment 25

[Jet Villegas (inactive)]

\o/ Nice work!

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

While I would be happy to see this change shipping in Firefox, we cannot accept an uplift of a patch which baked only two days (esp a week-end) and landing that directly in the first beta (today is merge day).

Comment 27

[Daniel Holbert [:dholbert]]

(Seems like we should request approval-mozilla-beta here sometime soon, then, for an uplift early in this 6-week cycle -- after this has gotten a few more days of baking on Nightly [& perhaps some baking on Aurora46].)

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

See comment 22.
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8710809 [details] [diff] [review]
Schedule image visibility update much less often when display port changes v2

See comment 23.
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Even if the backing period, I don't think I will take it.
AFAIK, it is not a regression.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #30)
> Even if the backing period, I don't think I will take it.
> AFAIK, it is not a regression.

It is a regression, though one that we've shipped for about a year.

If the current bar to beta is unshipped regressions and security fixes, we shouldn't take this. If the bar includes "very low-risk perf fixes that observably move the performance needle", then we should.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Bobby, I won't argue about the one year regression :)

Why do you think it is very low-risk? The patches are not one liner here!

Comment 33

[Timothy Nikkel (:tnikkel)]

"Properly update mLastUpdateImagesPos" would be a one line patch if PresShell was able to access mLastUpdateImagesPos on ScrollFrameHelper. But it's not because of encapsulation, so we just have to add a function for that purpose, and we need to implement that function in a few places because xul of html scroll frames are abstracted out. (Also, one-liner patches can be very risky, patch size does not correspond directly to risk.)

Further more, image visibility is only a heuristic, and as such it is not meant to be infallible. It is meant to be a best guess, so even if we royally screw it up, Firefox should still work fine (just maybe not as optimally).

This patch restores the (correct) behaviour of updating mLastUpdateImagesPos every image visibility pass, which was broken when we moved from the display list based image visibility to the frame tree walker image visibility. So this behaviour has been tested for a long time on release Firefox.

"Schedule image visibility update much less often when display port changes v2" also looks big but it's not really. It also creates a new function on scroll frames, which requires boilerplate to implement. The other added code looks complicated but it's just comparing an old rect to a new rect to see if it's changed significantly. Displayports are only used with apz, which is only used with e10s and on fennec. So it doesn't affect release, or our main beta population. Furthermore, this "check if the displayport changed enough" trigger for image visibility is in addition to all the triggers for image visibility we already have for non-apz cases. So even if we completely screw it up we should still get a similar experience to non-apz Firefox. I don't know the plans for testing e10s/apz on beta, but last I heard there were plans to try it as an experiment. If we do that we would want this fix so that our perf data is confused by having some useless work that we've already eliminated in there.

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710809 [details] [diff] [review]
Schedule image visibility update much less often when display port changes v2

I am sorry but I would like this to ride the train from 46.
45 is an esr, I would like to avoid potential regressions in the second half of the beta cycle.