Closed Bug 1001455 Opened 10 years ago Closed 10 years ago

Remove some gradients to save memory

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g 1.3T+
Tracking Flags:
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: vingtetun, Unassigned, NeedInfo)

References

Details

(Whiteboard: [tarako_only])

Attachments

(2 files, 1 obsolete file)

memdump_sorted.txt
17.66 KB, text/plain
Details
remove.gradients.1.3t.patch
5.88 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
Attached patch remove.gradients.patch (obsolete) — — Splinter Review 
I instrumented a bit imgLoader.cpp and found that gradients are killing us in terms of memory. Each gradient of the size of the screen cost around 600k of memory.

I made a small patch to remove them and use a solid color instead. It's a bit less pretty but it saves ~2.52mb in the System app. And shave and additional 600k too when the dialer is open.

I'm pretty sure that the UI team will not be 100% happy, but this is close to 2% of the total memory of the Tarako...

Fwiw I didn't tried this patch on Tarako as I don't have a device handy where I can double check.
Attachment #8412648 - Flags: review?(fabrice)
Attached file memdump_sorted.txt — 
Fwiw here is the output of the instrumention I made with the system app, communications and the sms app opened.
The biggest visual impact imo is on the lockscreen. We may want to have a different patch on master but it this stuff saves 2.52mb on Tarako, I think we should not be picky on that and use it.
Can you rebase on 1.3t? They are many conflicts including missing files.
Here a 1.3t version of the patch.
Attachment #8412648 - Attachment is obsolete: true
Attachment #8412648 - Flags: review?(fabrice)
Attachment #8412720 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Can you rebase on 1.3t? They are many conflicts including missing files.

missing files mostly comes from a rename of shared/style_unstable to shared/style fwiw.
Whith just the system app + homescreen running (locked but screen turned off) I see only a 0.37MB difference in the parent with this patch.

With the dialer running, we gain 2MB in the parent and around 0.7MB in the comm app. So that's definitely worth doing!
Comment on attachment 8412720 [details] [diff] [review]
remove.gradients.1.3t.patch

Review of attachment 8412720 [details] [diff] [review]:
-----------------------------------------------------------------

What is the plan for master? We'll take a big hit once we move tarako back there...
Attachment #8412720 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 8412720 [details] [diff] [review]
> remove.gradients.1.3t.patch
> 
> Review of attachment 8412720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the plan for master? We'll take a big hit once we move tarako back
> there...

Yeah.. we need to find a solution. Ideally it would be awesome if we can replace those image gradients by CSS gradients. IIRC the issue is about dithering. I will try to see if that's still the case with some GFX folks...
Ok, so I'm fine landing that on 1.3t only if you file a follow up.
blocking-b2g: --- → 1.3T+
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #8)
> Yeah.. we need to find a solution. Ideally it would be awesome if we can
> replace those image gradients by CSS gradients. IIRC the issue is about
> dithering. I will try to see if that's still the case with some GFX folks...

I guess we need bug 627771 to use CSS gradients then?
See Also: → 627771
I dug a bit more inside the platform, to understand what's going on. My current feeling right now is that there are something which sounds like bugs in the platform, related to how those images are handled.

Firstly all those images are background-images defined in CSS.

The first line of the memdump_sorted.txt attachment looks like:
628 Name (used): app://system.gaiamobile.org/style/lockscreen/images/mask.png (raw: 28672) (u heap: 80) (u non-heap: 614400)



Even if the lockscreen is not visible anymore, this image is considered as 'used'. My very basic understanding of the layout codebase brings me to think that there is an imgProxy set up when the related CSS rule is hit, that is never released.

If the proxy is released the decoded data of the image could be thrown away by the ImageDiscarder and then only 4k of memory will be consumed when this image is not on-screen, instead of 628k.

Then the question is how can I force the release of the imgProxy from the content ? I tried to remove the targeted node from the DOM when I don't need it anymore, or to display: none the targeted node, but it seems like it does not release the image.

The only way I succeed is by removing the related rule from the css file on the fly. That's not really practical and I wonder if that's how it is supposed to work.


The second line of the memdump_sorted.txt attachment is also interesting:
604 Name (used): app://system.gaiamobile.org/shared/style/value_selector/images/ui/gradient.png (raw: 4096) (u heap: 80) (u non-heap: 614400)

Because of the decode on draw pref set in http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#304 I would have expected this background-image to be decoded only when the relative DOM elements would have been painted on the screen.

And since https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L737 since it's container is |hidden| https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L735

Please note that the rule has been overriden so instead of display: none it has a visibility: hidden and opacity: 0 at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/value_selector/value_selector.css#L11

(it is not clear to me why is has both rules or why hidden has been overidden but that's an other story)

Also if I remove the override to have a display: none instead then the image is not decoded. But then we end up into the first issue mentioned when the decoded data are not released afterward...

needinfo'ing dbaron to see if this is expected, if there is something wrong on the platform side, or just something wrong on the gaia side.
Flags: needinfo?(dbaron)
(In reply to Ben Kelly [:bkelly] from comment #10)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #8)
> > Yeah.. we need to find a solution. Ideally it would be awesome if we can
> > replace those image gradients by CSS gradients. IIRC the issue is about
> > dithering. I will try to see if that's still the case with some GFX folks...
> 
> I guess we need bug 627771 to use CSS gradients then?

I guess it depends. If the issues are on the platform side then we can keep our current images. That said just because people likes using css gradients it would be nice to have.
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth)
Flags: needinfo?(dbaron)
I haven't looked into background image handling in detail but what you describe is consistent with my understanding of it. Background images are currently fairly "dumb", and apps like the gallery take advantage of this to do their own visibility monitoring to have a valid background image or not. That's the current state, but it could definitely be improved.

So if the computed style of the background style of an element has a valid image in it, and that element is not display: none (so it gets a frame), then the image will be asked to decode. The good news is that the image will get discard at the first memory pressure event or when we have image.mem.max_decoded_image_kb of decoded images (currently 30mb) and not decoded again (unless it's asked again). Sidenote: that value should probably be lowered for 128mb devices.

I think the term "used" relates to image locking. Image locking has no effect on b2g, locked/unlocked (used/unused) images are treated exactly the same (on desktop they differ). The imgProxy existing is unrelated to the decoded image data sticking around, it can be discarded separately. If you display: none or remove from the DOM we will not discard the image data immediately (this is on purpose so that if the image is put back in the DOM or made display other than none there won't be a flash while we decode it again). But the decoded data should get released. If we aren't currently using a lot of memory for decoded image the discard could be a little bit passive, but it should happen eventually.

Decode-on-draw by itself isn't really a viable option because you either end up with very long paints that spend a lot of time decoding images or images flashing into view as they get decoded, which is a very annoying user experience. So something is needed to make sure that images are decoded _before_ we need them in the most important cases. And this can't be perfect and there are always going to be trade-offs (decoding more images then are needed vs not decoding enough images). For background images this is probably pretty sub-par right now.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #13)
> I haven't looked into background image handling in detail but what you
> describe is consistent with my understanding of it. Background images are
> currently fairly "dumb", and apps like the gallery take advantage of this to
> do their own visibility monitoring to have a valid background image or not.
> That's the current state, but it could definitely be improved.
> 
> So if the computed style of the background style of an element has a valid
> image in it, and that element is not display: none (so it gets a frame),
> then the image will be asked to decode. The good news is that the image will
> get discard at the first memory pressure event or when we have
> image.mem.max_decoded_image_kb of decoded images (currently 30mb) and not
> decoded again (unless it's asked again). Sidenote: that value should
> probably be lowered for 128mb devices.
> 
> I think the term "used" relates to image locking. Image locking has no
> effect on b2g, locked/unlocked (used/unused) images are treated exactly the
> same (on desktop they differ).

This is true in general, but this issue happens in the system app, which is in-process. Not sure if it affects your following statement ?

> The imgProxy existing is unrelated to the
> decoded image data sticking around, it can be discarded separately. If you
> display: none or remove from the DOM we will not discard the image data
> immediately (this is on purpose so that if the image is put back in the DOM
> or made display other than none there won't be a flash while we decode it
> again). But the decoded data should get released. If we aren't currently
> using a lot of memory for decoded image the discard could be a little bit
> passive, but it should happen eventually.
> 
> Decode-on-draw by itself isn't really a viable option because you either end
> up with very long paints that spend a lot of time decoding images or images
> flashing into view as they get decoded, which is a very annoying user
> experience. So something is needed to make sure that images are decoded
> _before_ we need them in the most important cases. And this can't be perfect
> and there are always going to be trade-offs (decoding more images then are
> needed vs not decoding enough images). For background images this is
> probably pretty sub-par right now.
The patch is obviously [tarako_only], unless you want to make build time change to CSS with GAIA_MEMORY_PROFILE=low build time switch.
Status: NEW → ASSIGNED
Whiteboard: [tarako_only]
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #14)
> (In reply to Timothy Nikkel (:tn) from comment #13)
> > I think the term "used" relates to image locking. Image locking has no
> > effect on b2g, locked/unlocked (used/unused) images are treated exactly the
> > same (on desktop they differ).
> 
> This is true in general, but this issue happens in the system app, which is
> in-process. Not sure if it affects your following statement ?

Oh, you're right! I forgot it was content processes only. So in that case in processes which have image locking enabled the images will be suitable for discarding when the document's locking state becomes false. In desktop terms this means that it is a background tab. It's controlled via isActive on the nsIDocShell interface. It looks like it's set to false on the system process when the screen is turned off, otherwise true.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> The patch is obviously [tarako_only], unless you want to make build time
> change to CSS with GAIA_MEMORY_PROFILE=low build time switch.

Yes, the patch is Tarako only. But I'm also trying to find a way to free the resources correctly on master, so this won't regress when 1.3t will switch back to master.

Fwiw I'm not sure the build flag for low memory is exactly what we are looking for. I would have preferred something a bit more dynamic :/
(In reply to Timothy Nikkel (:tn) from comment #16)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #14)
> > (In reply to Timothy Nikkel (:tn) from comment #13)
> > > I think the term "used" relates to image locking. Image locking has no
> > > effect on b2g, locked/unlocked (used/unused) images are treated exactly the
> > > same (on desktop they differ).
> > 
> > This is true in general, but this issue happens in the system app, which is
> > in-process. Not sure if it affects your following statement ?
> 
> Oh, you're right! I forgot it was content processes only. So in that case in
> processes which have image locking enabled the images will be suitable for
> discarding when the document's locking state becomes false. In desktop terms
> this means that it is a background tab. It's controlled via isActive on the
> nsIDocShell interface. It looks like it's set to false on the system process
> when the screen is turned off, otherwise true.


OK, that's more or less what I understood based on the DiscardTracker code and nsPresShell. So it basically means this memory won't be available to apps while the phone is used :(

So theorically I can avoid the decoded data by making those frames |display: none| instead of |visibility: hidden|, make them |display: block| when they are needed, then back to |display: none;| when they are not needed anymore.

And then the decoded data should go away on the first memory-pressure. Is that correct ?
That's what I would expect (and of course the current behavior isn't the best, hopefully it can be improved).
hi Vivien, i wonder if this can land?
and do you mind taking this bug? Thanks
Flags: needinfo?(21)
(In reply to Joe Cheng [:jcheng] from comment #20)
> hi Vivien, i wonder if this can land?
> and do you mind taking this bug? Thanks

Yes the patch for 1.3t can land. Do you take care of that ?

I would like to keep this bug open once it has landed in order to provide something for 1.5 though.
Flags: needinfo?(21)
Yang, please land it on v1.3t.
Flags: needinfo?(yang.zhao)
(In reply to James Zhang from comment #22)
> Yang, please land it on v1.3t.
Already land it.
commit:2962204befea0fc9b7297a7886d7791aaad21095
status-b2g-v1.3T: affected → ---
Flags: needinfo?(yang.zhao)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
> Fwiw I didn't tried this patch on Tarako as I don't have a device handy
> where I can double check.
Ni Steven, 
    You should send a tarako to Vivien.
Flags: needinfo?(styang)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #1)
> Created attachment 8412649 [details]
> memdump_sorted.txt
> 
> Fwiw here is the output of the instrumention I made with the system app,
> communications and the sms app opened.

Hi Vivien and Fabrice,
   Can you tell us to how to get the memory dump data? We need the method.
Flags: needinfo?(fabrice)
Flags: needinfo?(21)
You can get memory reports by running:

MOZ_IGNORE_NUWA_PROCESS=1 ./tools/get_about_memory.py --minimize --no-gc-cc-log from your toplevel b2g directory.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #26)
> You can get memory reports by running:
> 
> MOZ_IGNORE_NUWA_PROCESS=1 ./tools/get_about_memory.py --minimize
> --no-gc-cc-log from your toplevel b2g directory.

We meet this error.


SPREADTRUM\james.zhang@jameszhangubtpc:/home/500G-disk/sp6821a_gonk4.0/B2G$ MOZ_IGNORE_NUWA_PROCESS=1 ./tools/get_about_memory.py --minimize --no-gc-cc-log
Traceback (most recent call last):
  File "./tools/get_about_memory.py", line 294, in <module>
    main()
  File "./tools/get_about_memory.py", line 291, in main
    get_and_show_info(args)
  File "./tools/get_about_memory.py", line 184, in get_and_show_info
    (out_dir, merged_reports_path, dmd_files) = get_dumps(args)
  File "./tools/get_about_memory.py", line 181, in get_dumps
    return utils.run_and_delete_dir_on_exception(do_work, out_dir)
  File "/home/500G-disk/sp6821a_gonk4.0/B2G/tools/include/device_utils.py", line 147, in run_and_delete_dir_on_exception
    return fun()
  File "./tools/get_about_memory.py", line 164, in do_work
    optional_outfiles_prefixes=['dmd-'])
  File "/home/500G-disk/sp6821a_gonk4.0/B2G/tools/include/device_utils.py", line 212, in notify_and_pull_files
    (master_pid, child_pids) = get_remote_b2g_pids()
  File "/home/500G-disk/sp6821a_gonk4.0/B2G/tools/include/device_utils.py", line 120, in get_remote_b2g_pids
    raise Exception('Two copies of b2g process found?')
Exception: Two copies of b2g process found?
That's because your build has the patch from bug 977026, but probably not part 5.
(In reply to Fabrice Desré [:fabrice] from comment #28)
> That's because your build has the patch from bug 977026, but probably not
> part 5.

Yes.
By the way, when can we land bug 977026?
(In reply to James Zhang from comment #30)
> By the way, when can we land bug 977026?

Let's not discuss that here.
(In reply to James Zhang from comment #24)
> > Fwiw I didn't tried this patch on Tarako as I don't have a device handy
> > where I can double check.
> Ni Steven, 
>     You should send a tarako to Vivien.

I don't have any Tarako devices in hand now, supposedly, we get several devices in Paris.
Vivien, do you mind checking with David for the device? thanks.
Flags: needinfo?(styang)
(In reply to Steven Yang [:styang] from comment #32)
> (In reply to James Zhang from comment #24)
> > > Fwiw I didn't tried this patch on Tarako as I don't have a device handy
> > > where I can double check.
> > Ni Steven, 
> >     You should send a tarako to Vivien.
> 
> I don't have any Tarako devices in hand now, supposedly, we get several
> devices in Paris.
> Vivien, do you mind checking with David for the device? thanks.

I have a Tarako in Paris. We still need to figure out how to flash it from scratch. David just got a windows to flash it but with the wrong version, so it's coming.
Flags: needinfo?(21)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: