I see this on a hamachi while panning around.

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.

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!

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.

Milan said he was going to have some input here.

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

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.

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!

BenWa instead.

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.

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.

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.

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 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.

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.

(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.

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 :)

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.

(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 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.

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.

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 on attachment 8407457 [details] [diff] [review] disable the display list building pass r=mats

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

(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.

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.

(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.

(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.

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

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?

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.

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