981899 - PresShell::UpdateImageVisibility takes approx 30ms on the contacts app with workflow-medium

Status: NEW

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

Description

[Jeff Muizelaar [:jrmuizel]]

I see this on a hamachi while panning around.

Comment 1

[Timothy Nikkel (:tnikkel)]

We should probably just convert UpdateImageVisibility to a custom frame tree walker instead of being display list based. That should save a lot of work. It's on my todo list, but b2g stuff keeps pushing it lower. Maybe I can actually get it done now that its hurting b2g.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Based on bug 986681 being a blocker and Jeff's description of the impact of this bug here, I think perhaps we might want to raise the priority of this one.  Nom'ing for 1.4.

Timothy, is this something that we might be able to do in the short term?  Or easy enough someone outside of gfx might be able to do with mentorship?

Thanks!

Comment 3

[Timothy Nikkel (:tnikkel)]

I don't think it should be too much code to write, I've just been busy fighting b2g fire after fire. If this has smouldered into a fire then I can slot it into my work items.

Comment 4

[Andrew Overholt [:overholt]]

Milan said he was going to have some input here.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Ben, are you saying that this contributes to the contact app checkerboarding?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Milan, I think it probably contributes in that we have a certain amount of time budget available to get content rendered and anything that uses that time budget will contribute to checkerboarding.

Basically, I saw that Jeff had found a number of issues while profiling contacts.  I wanted to see if we could solve any of these to help the situation since it looks like we will be unable to change the app structure significantly for v1.4.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Triage: Hi Jeff, is the 30ms happening on every single swipe? Can you please grab a profile showing this issue? The profiling guide is here - https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler

Thanks!

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

BenWa instead.

Comment 9

[Timothy Nikkel (:tnikkel)]

We queue an event to do UpdateImageVisibility every time a scrollframe scrolls by more than half its height. The event will run when it gets to the front of the event queue for the main content thread. That should give an idea of the frequency.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

We still want to know what we can do about this, but we're moving it to 2.0 as the bug it came from is there.  If the blocked bug comes back to 1.4, we'll reopen the conversation on this one.

Comment 11

[Benoit Girard (:BenWa)]

Here's a profile of this from desktop:
http://people.mozilla.org/~bgirard/cleopatra/#report=3325c5a9a1e9789711431fd69b2088adfb959727

Part 2 from bug 995267 will help here but doing the approach in comment 1 is better.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Use the paint display list - Concept only

I did a worse case scenario with 50,000 display items on the page and even then running RebuildImageVisibility every frame is *much* cheaper then once every half viewport building a displaylist and running RebuildImageVisibility. This will help with frame uniformity. We now build a full display list with DLBI so that displaylist will have everything we need I think.

So:
* Does this approach look sound?
* I could have RebuildImageVisibility not run if you're not scrolling
* Are the nsDisplayListBuilder::IMAGE_VISIBILITY special cases needed or are they just ways to build a visibility displaylist more efficiently which we wont need?

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8405601 [details] [diff] [review]
Use the paint display list - Concept only

(In reply to Benoit Girard (:BenWa) from comment #12)
> I did a worse case scenario with 50,000 display items on the page and even
> then running RebuildImageVisibility every frame is *much* cheaper then once
> every half viewport building a displaylist and running
> RebuildImageVisibility. This will help with frame uniformity. We now build a
> full display list with DLBI so that displaylist will have everything we need
> I think.
> 
> So:
> * Does this approach look sound?
> * I could have RebuildImageVisibility not run if you're not scrolling
> * Are the nsDisplayListBuilder::IMAGE_VISIBILITY special cases needed or are
> they just ways to build a visibility displaylist more efficiently which we
> wont need?

Using the same display list as painting is basically pointless, since drawing the images will do basically the same thing as image visibility. The image visibility display list is expanded on scroll frames so it contains more content.

Here is a suggestion: we could disable UpdateImageVisibility on b2g (this doesn't disable image visibility, just one part of it). Since we have the unlocked draw notification and the first reflow check of image frames we should still get reasonable results. And because image locking is disabled we don't need UpdateImageVisibility to remove non-visible images. And , unlike desktop, we have displayports, which already expand whats visible. Pros of this are: skip an extra build display list pass, less memory use because we only ask to decode images that we draw (instead of ones that we think will be drawn soon). Cons: more image decoding will happen during painting, more images will not be drawn because they havent been decoded.

I think it could be a good trade off.

I'll also see about banging out a frame tree walker.

Comment 14

[Timothy Nikkel (:tnikkel)]

Ben informed me that the Contacts app is now using background images, which image visibility does not currently track. So the Contacts app is getting zero benefit from this call. It can enable/disabled per document via PresShell::AssumeAllImagesVisible currently we have a pref to enable it globally, a pref to enable it in the browser app only, and we also disable in chrome docs and in xul docs. The only hard part would be coming up with a reasonable way to target this only where we want it disabled.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Timothy Nikkel (:tn) from comment #13)And ,
> unlike desktop, we have displayports, which already expand whats visible.

How about we disable image visibility on platforms where we have a display port enabled then? This would mean all of mobile (and maybe later this year desktop). The problem is if this can potentially cost us 30ms to build a display list then we're going to miss frame uniformity. Decode on draw isn't great but it's fairly well bounded (5ms I think?), while building a display list is unbounded. The downside is we're going to do some decode on draw/progressive decode for images that are on the display port which isn't great.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

When you say "disable image visibility", do you actually mean "disable UpdateImageVisibility", or do you really mean disable it all?
Anyway, whenever something is being done that has to do with image visibility, make sure we test on pinterest :)

Comment 17

[Benoit Girard (:BenWa)]

What I *think* I'm suggesting would be:
* Don't decode image that are not being drawn (on mobile this is the display port)
* Stop running UpdateImageVisibility on scroll every half screen height scroll. This means that images wont be decoded -before- they enter the display port but rather -when-.
* When an image is visible we will start spending a bit of time during the draw to decode a piece of the image every paint.

So it shouldn't regress the memory usage.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: Disable UpdateImageVisibility on mobile because we have a DisplayPort

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Benoit Girard (:BenWa) from comment #15)
> How about we disable image visibility on platforms where we have a display
> port enabled then? This would mean all of mobile (and maybe later this year
> desktop).

We can't turn it on on Fennec as is because they have image locking enabled (unlike b2g), and that's important. For desktop I think the different perf characteristics mean we might want to make quite different choices.

The other downside that you didn't mention is that this basically chains us to doing the majority of our image decoding synchronously during painting on the content thread (instead of off the main content thread). So that's why I don't think this approach can be anything more than a hack.

> The problem is if this can potentially cost us 30ms to build a
> display list then we're going to miss frame uniformity. Decode on draw isn't
> great but it's fairly well bounded (5ms I think?), while building a display
> list is unbounded. The downside is we're going to do some decode on
> draw/progressive decode for images that are on the display port which isn't
> great.

Decode on draw is bounded to 5 ms _per image_, not 5 ms. So this can easily balloon too.

Did we ever get a profile of the contacts app doing this? I ask because we will currently do some sync decoding during this pass when we mark an image as visible. So it would be interesting to confirm that most of the time is in display list building or image decoding (or elsewhere?). This is not relevant to the Contacts app as it is today because they moved to background images, but could have been relevant when Jeff originally reported this bug.

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8406350 [details] [diff] [review]
Disable UpdateImageVisibility on mobile because we have a DisplayPort

This needs to be gonk only, this will interact badly on Fennec because they have image locking enabled.

Also, we need to stop using the list of visible images, otherwise we'll get hanging references without anything to clean them up.

Also we'll need to hear from the Gallery folks as they've been comparing their own visibility tracker and the gecko one with the aim of using the gecko one, and I think they are pretty close to that now. This will likely change their analysis.

I will write a new patch.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: disable the display list building pass

Bit of background, the display list building pass is taking a significant amount of time on b2g. And some apps only used background images, making it wasted effort. Because image locking is disabled on b2g the net effect of this is to decode the images before they are needed. Since we already have a displayport on b2g we already render content that is not visible so pre-decoding images about to  be scrolled in is somewhat less of a worry. When we paint any images we will do a limited amount of decoding on it (for small images it will finish), and then it will finished later, after painting. So this is one approach we can try to see how well it works.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Not sure if it will effect this or not, but there are some issues with how contacts/gaia uses images and image rotation.  See bug 989290 and this thread on dev-webapi:

  https://groups.google.com/forum/#!topic/mozilla.dev.webapi/Pk2OEdtsyRE

One suggestion there has been to try to go back to <img> elements, but its not clear what the final resolution will be.

I just wanted to mention it here in case we want to take that discussion into account.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8407457 [details] [diff] [review]
disable the display list building pass

r=mats

Comment 24

[Mats Palmgren (inactive)]

It would be nice for all platforms if we could make UpdateImageVisibility
faster and maybe also try to schedule it less often.
Could we delay it if:
* mVisibleImages.Count() is a small number
* some RefreshDriver metric says we're very busy right now
* we ran it < K millisec ago
* mVisibleImages.Count() is not much more than last time

Also, what happened to the nsTHashtable::SwapElements idea?
https://bugzilla.mozilla.org/show_bug.cgi?id=847223#c14

Comment 25

[Benoit Girard (:BenWa)]

(In reply to Timothy Nikkel (:tn) from comment #19)
> Decode on draw is bounded to 5 ms _per image_, not 5 ms. So this can easily
> balloon too.

That's right. We should maybe change that to a global budget.

> 
> Did we ever get a profile of the contacts app doing this?

The contact app calls Stop while scrolling which stops the image decoding. You wont see any progressive image decoding while scrolling the contact app.

Comment 26

[Timothy Nikkel (:tnikkel)]

I filed bug 1002992 to write a frame tree walker with a patch, it seems to be about 3-4x faster.

I also profiled the Contacts app, the image visibility pass seemed to only be taking about 3% of the non-idle time on the content thread.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Benoit Girard (:BenWa) from comment #25)
> > Did we ever get a profile of the contacts app doing this?
> 
> The contact app calls Stop while scrolling which stops the image decoding.
> You wont see any progressive image decoding while scrolling the contact app.

I don't see have anything javascript does can have an affect on what happens when RasterImage::StartDecoding is called. Anyways since the Contacts app is using background images now it won't see any partially decoded images with or without any other hacks it might be doing.

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #24)
> It would be nice for all platforms if we could make UpdateImageVisibility
> faster and maybe also try to schedule it less often.

I agree, so I wrote bug bug 1002992 and hope to land that instead of this patch.

> Could we delay it if:
> * mVisibleImages.Count() is a small number
> * some RefreshDriver metric says we're very busy right now

This cuts both ways because if we ask to decode an image before it's needed we can decode it on a background thread on potentially an idle cpu core (however currently due to a limitation of how image visibility is implemented we will do some decoding on the main thread, this should be fixed), whereas if we paint an image we will do some decoding of it sync during painting (on the main thread). So if we're busy it's even more important to push work to other threads.

> * we ran it < K millisec ago
> * mVisibleImages.Count() is not much more than last time

Interesting ideas, I hadn't thought about them. We'll see if we need them.

> Also, what happened to the nsTHashtable::SwapElements idea?
> https://bugzilla.mozilla.org/show_bug.cgi?id=847223#c14

Sorry about that, it got a little lost in other followups. In any case it doesn't seem to be a significant contribution to time taken, so not super high priority.

Comment 29

[Jet Villegas (inactive)]

If this is still a blocker, I think you want to flag bug 1002992 instead.

Comment 30

[Mike Lee [:mlee]]

Milan, you flagged this as 2.0+ back in comment 10. Do you agree with Jet that bug 1002992 should instead be the 2.0 blocker?

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

If Jet thinks this is the root cause, I'm good.  If we do that, we probably want bug 1002992 to block bug 982210 and/or bug 986681.

Comment 32

[Mike Lee [:mlee]]

Unblocking 2.0 per comment 29 and comment 31. Bug 1002992 has been updated as the correct 2.0 blocker.