Closed Bug 1241371 Opened 8 years ago Closed 8 years ago

UpdateImageVisibility eats up ~5ms/frame when scrolling Facebook

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
platform-rel --- ?
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: bholley, Assigned: tnikkel)

References

Details

(Whiteboard: [platform-rel-Facebook])

Attachments

(2 files, 1 obsolete file)

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.
Timothy said he'd have a look here.
Flags: needinfo?(tnikkel)
Two problems here, regressions from bug 1169881 and bug 1002992.
Assignee: nobody → tnikkel
Blocks: 1169881, 1002992
Flags: needinfo?(tnikkel)
This is a regrettable mistake that we should have caught sooner.
Attachment #8710271 - Flags: review?(mats)
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.
Attachment #8710272 - Flags: review?(bugmail.mozilla)
Comment on attachment 8710271 [details] [diff] [review]
Properly update mLastUpdateImagesPos

r=mats
Attachment #8710271 - Flags: review?(mats) → review+
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 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?
Attachment #8710272 - Flags: review?(bugmail.mozilla)
(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 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.
Attachment #8710272 - Flags: review?(bugmail.mozilla)
Keywords: leave-open
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.
(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".
Attachment #8710272 - Attachment is obsolete: true
Attachment #8710272 - Flags: review?(bugmail.mozilla)
Attachment #8710809 - Flags: review?(bugmail.mozilla)
(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 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.
Attachment #8710809 - Flags: review?(bugmail.mozilla) → review+
(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!
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7a7def04840d
https://hg.mozilla.org/mozilla-central/rev/0947272393af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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 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
Attachment #8710271 - Flags: approval-mozilla-aurora?
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
Attachment #8710809 - Flags: approval-mozilla-aurora?
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.
\o/ Nice work!
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).
Attachment #8710271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8710809 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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].)
Flags: needinfo?(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]:
Flags: needinfo?(tnikkel)
Attachment #8710271 - Flags: approval-mozilla-beta?
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]:
Attachment #8710809 - Flags: approval-mozilla-beta?
Even if the backing period, I don't think I will take it.
AFAIK, it is not a regression.
(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.
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!
"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 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.
Attachment #8710809 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8710271 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
platform-rel: --- → ?
Whiteboard: [platform-rel-Facebook]
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.