The Gallery app uses a "visibility monitor" module to track which thumbnails are on-screen (or close to being onscreen) and only sets the CSS background-image style for images that have some chance of being displayed soon. Back when this was introduced, it was necessary to prevent OOMs when there were thousands of photos on the phone because otherwise we were holding thousands of thumbnails decoded in memory and memory use for the app increased linearly with the number of thumbnails. Now that bug 943248 has landed, I believe that Gecko does this same thing for us automatically, and we should remove the visiblity monitor code We should do this early in the 1.4 cycle so that there is plenty of time to catch any regressions this causes.

My preliminary testing (as part of diagnosis for bug 963917) seems to show that we can remove visibility monitor without causing OOM while scanning 1000s of images or while scrolling through a list of 1000s of thumbnails. But I would like to confirm this with someone who understands the underlying gecko patch. Timothy: can you confirm that your patch in bug 943248 means that Gecko will not decode images if they are far off screen and will release decoded images that are offscreen when there is memory pressure?

(In reply to David Flanagan [:djf] from comment #1) > Timothy: can you confirm that your patch in bug 943248 means that Gecko will > not decode images if they are far off screen Yes, that is the intention. > and will release decoded images > that are offscreen when there is memory pressure? On b2g image locking is currently disabled, so we don't "hold" onto images. Decoded image data can be discard whenever asked, and I believe that memory pressure does ask for it. I would recommend to do some testing with the visibility monitor removed to see if there are any regressions. If there are we can investigate how to fix them. If not, great!

(In reply to David Flanagan [:djf] from comment #0) > The Gallery app uses a "visibility monitor" module to track which thumbnails > are on-screen (or close to being onscreen) and only sets the CSS > background-image style for images that have some chance of being displayed > soon. The work in bug 943248 (and all the other bugs leading up to it) do not deal with CSS background images, they deal with img elements and a few others. So bug 943248 won't have any effect on background images.

What's the cost of having the visibility monitor still in there?

Hi David, Visibility monitor is removed from gallery app as per previous discussion. Provides a pull request in the attachment.

Comment on attachment 8396955 [details] Pull request r-. You have to convert the thumbnails to use <img> instead of background-image or this won't work. See comment #3.

See bug 985345 for details that are relevant to this bug.

Also please do not remove the test if the file is still in the codebase.

Removed visibility monitor in gallery app, use <img> element to display thumbnail and let Gecko to manage the memory. Also keep the test as per comment 8 suggest.

Update previous pull request

Sherman, Wasn't it decided in the other 1.3T bug (I forget the number) that this approach did not save enough memory for Tarako and that the visibility monitor was still required in that case so that we could be more aggressive about memory reductions? Do you recall what was discovered about the memory use of this approach?

Comment on attachment 8399829 [details] Pull request Punam: I'm re-assigning the review of this patch to you, if you don't mind. This seems like something that would be nice to be able to land early in 1.5 if we can. But I'd like to know how it affects memory usage and panning frame rate in the gallery when there are about 1000 images displayed. For the 1.3T bug, memory was measured just using b2g-ps. If we could get real about:memory reports showing the actual amount of decoded image memory for the current gallery and this patch, that would be great. If you are overloaded or don't feel like you can do those investigations, assign this back to me.

Hi David Looks like 985345 is the related 1.3T bug in which removing tag visibility monitor and its memory usage was discussed. https://bugzilla.mozilla.org/show_bug.cgi?id=985345#c3 I will pick up the investigation (comment #12) and report my findings. Thanks

(In reply to David Flanagan [:djf] from comment #11) > Sherman, > > Wasn't it decided in the other 1.3T bug (I forget the number) that this > approach did not save enough memory for Tarako and that the visibility > monitor was still required in that case so that we could be more aggressive > about memory reductions? Do you recall what was discovered about the memory > use of this approach? David, That was Bug 985345 we were discussing, that issue is focusing on device with low memory available, and also we got 1.3T's time frame to keep up with, so we made the choice. Memory usage reports can be found in https://bugzilla.mozilla.org/show_bug.cgi?id=985345#c30 Make it in short, memory usage has merely no difference when we remove the visibility monitor. But I think let Gecko to manage the decoded image memory would be the better than we do that in Gaia. Previous test result shows that, given over 500 images in gallery app, the scrolling feels more smooth when visibility monitor is removed.

Attaching about:memory logs for 1000 images with and without bug960789 patch. With 1000 images (1600x1200) loaded with make reference-workload-x-heavy On Master (Gaia 471bb1e57c35b3282a5c046fd8a73e49ca1e9686)- with tag visibility monitor Gallery memory usage seen: 25.04 MB Memory shown under images: 3.23 MB (12.91%) To view memory log unzip the attached logs and open below url in firefox about:memory?file=<path to unzip folder>/master-1000-images-about-memory/memory-reports FPS - in 50 while scrolling - peaks at 60 ------------------ Bug 967089 patch - on removing tag visibility monitor Gallery memory usage: 24.21 MB Memory shown under images: 8.10 MB (33.45%) FPS - in 50- max 60 link to view logs about:memory?file=<path to unzip folder>/967089-1000-images-about-memory/memory-reports In both the above cases while scrolling very fast for a split second black screen followed by gray squares are observed that go away eventually. Scrolling experience in both the above cases is similar. Also attaching about:memory logs for gallery app loaded with 250 images. Memory usage of gallery app with 250 images with and without patch stayed around ~20 MB and memory under images seen is 10%, ~2MB Links to view respective memory report about:memory?file=<path to unzip folder>/967089-250-images-about-memory/memory-reports about:memory?file=<path to unzip folder>/master-250-images-about-memory/memory-reports Environment: BuildId:20140401160201 OS Version:1.5 Device: Hamachi

Including Timothy to give his input on gallery memory logs breakdown and increased memory usage seen under images after removing tag visibility monitor even though overall gallery memory usage stays same. Thanks

Thanks Sherman, patch to remove tag visibility monitor looks good and has my r+. My only query is as listed in comment #15 and comment #16, why the memory shown under images shows high after removing visibility monitor and where exactly the decoded image memory by gecko is shown in about:memory logs. If we can get those answers from some one familiar with memory reporting on gecko side, will be much more comfortable landing this change.

I think there must be a bug with about:memory for images. There is definitely one that I found: bug 995880. Perhaps that will fix this case too. The other thing is you probably want to break down the about:memory reports under images into uncompressed (decoded) and compressed. That will tell you exactly where the memory is going.

Hi Punam, Thanks for the feedback, should I land this change or wait for further investigation on memory report?

Sorry for delay, as per comment #18, i will run about:memory logs again on the patch and see if 995880 has resolved the bug and shows uncompressed images memory usage. If not, will get Timothy feedback on how to get memory logs fixed, i see bug 889902 / 995116 has a similar need of seeing correct uncompressed memory usage. Thanks

Attaching the memory logs showing decoded image memory for the current gallery and this patch. Gallery memory usage while scrolling 1200 images in list view without patch on latest master 40.93 MB (100.0%) -- explicit ├──22.09 MB (53.97%) -- images │ ├──22.09 MB (53.97%) -- content │ │ ├──22.09 MB (53.97%) -- used │ │ │ ├──13.89 MB (33.92%) ── uncompressed-nonheap │ │ │ ├───8.18 MB (19.98%) ── raw │ │ │ └───0.03 MB (00.07%) ── uncompressed-heap │ │ └───0.00 MB (00.00%) ++ unused │ └───0.00 MB (00.00%) ++ chrome Gallery memory usage while scrolling 1200 images in list view with patch 40.19 MB (100.0%) -- explicit ├──22.66 MB (56.39%) -- images │ ├──22.66 MB (56.39%) -- content │ │ ├──22.66 MB (56.39%) -- used │ │ │ ├──12.48 MB (31.05%) ── uncompressed-nonheap │ │ │ ├──10.16 MB (25.27%) ── raw │ │ │ └───0.03 MB (00.07%) ── uncompressed-heap │ │ └───0.00 MB (00.00%) ++ unused The scrolling experience with the gecko handling decoded memory looks good (FPS shared in comment #15). Need Info djf to share memory logs as requested in comment #12.

merged to master https://github.com/mozilla-b2g/gaia/commit/006cab98e7844de9adeb93f6faa8d33205c8b8f3

Thanks, Sherman for the patch. Thanks, Punam for the research on this.

Hi Marcia, With 967089 fix landed in master, gallery app is no longer using tag visibility monitor to prevent OOMs due to thumbnails decoded in memory when there are thousands of photos on the phone. With bug 943248, Gecko does this for us automatically, and that's why we have removed the visibility monitor code. Just wanted to update you of this change, if you see any regressions this causes in gallery app (thumbnails list view) on a really loaded device. Thanks Punam

Nice work here guys! Punam - as we have now removed this from gallery, should we also remove this from Contacts? Just wanted to check with you if we've filed a bug for that work yet, or if we should. Thanks!

(In reply to Kevin Grandon :kgrandon from comment #25) > Nice work here guys! Punam - as we have now removed this from gallery, > should we also remove this from Contacts? Just wanted to check with you if > we've filed a bug for that work yet, or if we should. Thanks! We haven't filed a bug yet for removing visibility monitor from contacts. I see only Bug 967884 that's related to visibility monitor in contacts.