[gallery] remove visibility monitor code if it is no longer needed

RESOLVED FIXED in 2.0 S1 (9may)

Status

Firefox OS
Gaia::Gallery
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: djf, Assigned: chens)

Tracking

unspecified
2.0 S1 (9may)
x86
Mac OS X

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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?
Assignee: nobody → dflanagan
Flags: needinfo?(tnikkel)

Updated

4 years ago
blocking-b2g: --- → 1.4?
(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!
Flags: needinfo?(tnikkel)
(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.
blocking-b2g: 1.4? → -
What's the cost of having the visibility monitor still in there?
(Assignee)

Comment 5

4 years ago
Created attachment 8396955 [details] [review]
Pull request

Hi David, 

Visibility monitor is removed from gallery app as per previous discussion.
Provides a pull request in the attachment.
Assignee: dflanagan → shchen
Attachment #8396955 - Flags: review?(dflanagan)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8396955 [details] [review]
Pull request

r-. You have to convert the thumbnails to use <img> instead of background-image or this won't work. See comment #3.
Attachment #8396955 - Flags: review?(dflanagan) → review-
(Reporter)

Comment 7

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8399335 [details] [review]
Pull request

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.
Attachment #8396955 - Attachment is obsolete: true
Attachment #8399335 - Flags: review?(dflanagan)
(Assignee)

Comment 10

4 years ago
Created attachment 8399829 [details] [review]
Pull request

Update previous pull request
Attachment #8399335 - Attachment is obsolete: true
Attachment #8399335 - Flags: review?(dflanagan)
Attachment #8399829 - Flags: review?(dflanagan)
(Reporter)

Comment 11

4 years ago
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?
Flags: needinfo?(shchen)
(Reporter)

Comment 12

4 years ago
Comment on attachment 8399829 [details] [review]
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.
Attachment #8399829 - Flags: review?(dflanagan) → review?(pdahiya)
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
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(shchen)
Created attachment 8402249 [details]
about:memory logs showing memory usage with and without bug967089 patch

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
Flags: needinfo?(tnikkel)
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.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 19

4 years ago
Hi Punam, 

Thanks for the feedback, should I land this change or wait for further investigation on memory report?
Flags: needinfo?(pdahiya)
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
Flags: needinfo?(pdahiya)
Created attachment 8414161 [details]
about-memory logs with 967089 fix

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.
Attachment #8402249 - Attachment is obsolete: true
Flags: needinfo?(dflanagan)
Attachment #8399829 - Flags: review?(pdahiya) → review+
(Assignee)

Comment 22

4 years ago
merged to master

https://github.com/mozilla-b2g/gaia/commit/006cab98e7844de9adeb93f6faa8d33205c8b8f3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

4 years ago
Thanks, Sherman for the patch.  Thanks, Punam for the research on this.
Flags: needinfo?(dflanagan)
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
Flags: needinfo?(mozillamarcia.knous)
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!
Flags: needinfo?(pdahiya)
(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.
Flags: needinfo?(pdahiya)
See Also: → bug 967884

Updated

4 years ago
Target Milestone: --- → 2.0 S1 (9may)
Blocks: 1015413
Flags: needinfo?(mozillamarcia.knous)
You need to log in before you can comment on or make changes to this bug.