Last Comment Bug 1241371 - UpdateImageVisibility eats up ~5ms/frame when scrolling Facebook
: UpdateImageVisibility eats up ~5ms/frame when scrolling Facebook
Status: RESOLVED FIXED
[platform-rel-Facebook]
:
Product: Core
Classification: Components
Component: Layout: Images (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla46
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 1002992 1169881
  Show dependency treegraph
 
Reported: 2016-01-20 19:05 PST by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2016-06-22 09:20 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?
affected
fixed


Attachments
Properly update mLastUpdateImagesPos (5.76 KB, patch)
2016-01-20 22:22 PST, Timothy Nikkel (:tnikkel)
mats: review+
sledru: approval‑mozilla‑aurora-
sledru: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Schedule image visibility update much less often when display port changes (3.65 KB, patch)
2016-01-20 22:25 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Schedule image visibility update much less often when display port changes v2 (8.92 KB, patch)
2016-01-21 18:48 PST, Timothy Nikkel (:tnikkel)
bugmail: review+
sledru: approval‑mozilla‑aurora-
sledru: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description User image Bobby Holley (:bholley) (busy with Stylo) 2016-01-20 19:05:14 PST
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 User image Bobby Holley (:bholley) (busy with Stylo) 2016-01-20 19:06:35 PST
Timothy said he'd have a look here.
Comment 2 User image Timothy Nikkel (:tnikkel) 2016-01-20 22:20:55 PST
Two problems here, regressions from bug 1169881 and bug 1002992.
Comment 3 User image Timothy Nikkel (:tnikkel) 2016-01-20 22:22:39 PST
Created attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

This is a regrettable mistake that we should have caught sooner.
Comment 4 User image Timothy Nikkel (:tnikkel) 2016-01-20 22:25:17 PST
Created attachment 8710272 [details] [diff] [review]
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 User image Timothy Nikkel (:tnikkel) 2016-01-21 02:20:04 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06c7dc28592
Comment 6 User image Mats Palmgren (:mats) 2016-01-21 07:00:35 PST
Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

r=mats
Comment 7 User image Mats Palmgren (:mats) 2016-01-21 07:02:38 PST
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 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2016-01-21 08:08:33 PST
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 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2016-01-21 08:09:13 PST
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> come out of GetDisplayPortImpl before after setting the margins?

before *and* after
Comment 10 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2016-01-21 12:53:26 PST
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 12 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2016-01-21 17:35:19 PST
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 User image Timothy Nikkel (:tnikkel) 2016-01-21 17:48:57 PST
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 User image Timothy Nikkel (:tnikkel) 2016-01-21 18:48:19 PST
Created attachment 8710809 [details] [diff] [review]
Schedule image visibility update much less often when display port changes v2
Comment 15 User image Timothy Nikkel (:tnikkel) 2016-01-21 18:55:03 PST
(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 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2016-01-21 19:53:20 PST
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 User image Timothy Nikkel (:tnikkel) 2016-01-21 22:28:21 PST
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 20 User image William Lachance (:wlach) (use needinfo!) 2016-01-22 10:48:53 PST
Some nice improvements on tp5o scroll from this: https://treeherder.allizom.org/perf.html#/alerts?id=1865&framework=1&hideImprovements=0
Comment 21 User image Timothy Nikkel (:tnikkel) 2016-01-22 12:33:01 PST
(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 User image Timothy Nikkel (:tnikkel) 2016-01-22 12:59:07 PST
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 User image Timothy Nikkel (:tnikkel) 2016-01-22 13:01:23 PST
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 User image Bobby Holley (:bholley) (busy with Stylo) 2016-01-22 13:03:53 PST
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 User image Jet Villegas (:jet) 2016-01-24 17:18:32 PST
\o/ Nice work!
Comment 26 User image Sylvestre Ledru [:sylvestre] 2016-01-25 02:52:02 PST
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 User image Daniel Holbert [:dholbert] 2016-01-25 10:33:14 PST
(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 User image Timothy Nikkel (:tnikkel) 2016-01-25 11:26:08 PST
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 User image Timothy Nikkel (:tnikkel) 2016-01-25 11:26:23 PST
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 User image Sylvestre Ledru [:sylvestre] 2016-01-25 13:22:53 PST
Even if the backing period, I don't think I will take it.
AFAIK, it is not a regression.
Comment 31 User image Bobby Holley (:bholley) (busy with Stylo) 2016-01-25 13:27:26 PST
(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 User image Sylvestre Ledru [:sylvestre] 2016-02-01 05:32:54 PST
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 User image Timothy Nikkel (:tnikkel) 2016-02-01 11:17:58 PST
"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 User image Sylvestre Ledru [:sylvestre] 2016-02-07 18:52:12 PST
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.

Note You need to log in before you can comment on or make changes to this bug.