1040341 - System app keeps many small images alive

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Benoit Girard (:BenWa)]

Starting a few app on b2g leaves several images loaded in the b2g system process include header-background.png, pattern.png, and app icons. On my device after starting a few apps the system app appears to be permanently keeping 8 MB of images alive.

Comment 1

[Benoit Girard (:BenWa)]

Ok so from discussing with khuey and tn:

- System app keeps the image locked. Which means they only get discard from a memory pressure if we allocate more than 30 MBs. (There might be another discard trigger I haven't seen).
- If the background is set in the style sheet it will be kept around EVEN if it doesn't match anything!

I'm still not sure why app icons are kept alive since they shouldn't be in the DOM or in any style sheet.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: CSS image load

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Benoit Girard (:BenWa) from comment #0)
> Starting a few app on b2g leaves several images loaded in the b2g system
> process include header-background.png, pattern.png, and app icons. On my
> device after starting a few apps the system app appears to be permanently
> keeping 8 MB of images alive.

I already noticed that in a separate bug. (Can't find it right now). What we did on 1.3t is to remove the gradient for a solid color, and it was a win of 2.4mo.


For the gradient I tried to play with them and some CSS rules to remove them, but nothing was working. Only a memory-pressure even. As I said, a simple workaround here is to use a solid color in order to save 2.4mb.

The 2 blobs are the background of the system app, and the background of the lockscreen IIRC.
We can probably get rid of the one from the lockscreen - and it will happens for free once the lockscreen will be a separated app.

App Icons appears in the background of an application, when it is launched. We can probably removed them once the app is loaded.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Just FYI we see the app icons much more in the new task manager for 2.1. If we remove them we'll have to recreate them when showing task manager. Which might be ok for apps outside the viewport but risks janking the animation to show the current and recent apps in the task manager. We're still scoping this work for 2.1 and getting bugs filed but heres an older spec that shows the usage, I'd like to get this icon from the appWindow rather than reload it: https://mozilla.app.box.com/s/nwr2w7g32sbkfjx0efau

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Sam Foster [:sfoster] from comment #4)
> Just FYI we see the app icons much more in the new task manager for 2.1. If
> we remove them we'll have to recreate them when showing task manager. Which
> might be ok for apps outside the viewport but risks janking the animation to
> show the current and recent apps in the task manager. We're still scoping
> this work for 2.1 and getting bugs filed but heres an older spec that shows
> the usage, I'd like to get this icon from the appWindow rather than reload
> it: https://mozilla.app.box.com/s/nwr2w7g32sbkfjx0efau

Last time I looked at the memory usage of those images, the blobs and gradients were much more concerning than app icons.

For the app icons we needs to make sure that the right icons are loaded, no need to use a 192px icon from the window manager if the task manager use a 84px icon. Depends on the spec.

Comment 6

[Benoit Girard (:BenWa)]

The gradient usage is fixed. The icons are small but from what I can tell they start growing and pile up as you launch more new apps. This will start to add up when testing the stability of the system over time.

Comment 7

[Preeti Raghunath(:Preeti)]

Milan

Please provide next steps here.

Comment 8

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

This falls into a general "look, we can use less memory" and it's good that we're going after this in the context of running out of memory on smaller devices. I don't know about picking this particular one as a CAF blocker (e.g., see comment 5), there may be other places where similar waste of memory goes on, or can be handled separately (e.g., bug 1040827.)
We're looking at some potential improvements at the Gecko level, will keep you posted if they pan out.

Comment 9

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I saw that multiple bugs related to homescreen memory usage got fixed and I re-measured homescreen memory usage today. 

I checked with following gaia/gecko 

https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v2.0&id=bf3fb88039843359d0a5759b2c0fb787abae544f

https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v2.0&id=e46acc521bf8e25787d9ed3937b0213774355c2d

adb reboot && sleep 60 && adb shell b2g-procrank
APPLICATION        PID       Vss      Rss      Pss      Uss  cmdline
b2g                240   261676K   82540K   67798K   59160K  /system/b2g/b2g
Homescreen        1014   107332K   34708K   22914K   17160K  /system/b2g/plugin-container
(Preallocated a   1174    62368K   20104K   10265K    6036K  /system/b2g/plugin-container
(Nuwa)             972    55120K   21392K    8750K    2616K  /system/b2g/plugin-container
                                            ------   ------  ------
                                           136541K  106664K  TOTAL

Homescreen is still using 17MB USS in v2.0 but it was using 12MB USS in v1.3 (See bug 1038884 Comment 15)

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug is about the system app.  It has nothing to do with the homescreen.

Comment 11

[Kai-Chih Hu [:khu]]

(In reply to Sam Foster [:sfoster] from comment #4)
> Just FYI we see the app icons much more in the new task manager for 2.1. If
> we remove them we'll have to recreate them when showing task manager. 

Can you provide more information about this? 

Which
> might be ok for apps outside the viewport but risks janking the animation to
> show the current and recent apps in the task manager. We're still scoping
> this work for 2.1 and getting bugs filed but heres an older spec that shows
> the usage, I'd like to get this icon from the appWindow rather than reload
> it: https://mozilla.app.box.com/s/nwr2w7g32sbkfjx0efau

Comment 12

[Sam Foster [:sfoster] (he/him)]

Updated spec: https://mozilla.app.box.com/s/m8lnw8mbx0dy3rawdhuo
User story: https://bugzilla.mozilla.org/show_bug.cgi?id=967420

Whereas today we just show a screenshot with a X to close, the new design have a tray with close button, favorite button and icon. We also transition the icon when opening and closing the task switcher and it may serve to mask delays in loading a screenshot. This code is not written yet, but knowing the AppWindow instance has an icon I was hoping to be able to use that. 

We don't need all icons of all appWindows in the stack necessarily. We could do a thing where we load those in or near the viewport - and request them as the user pans across - if keeping them all around is a problem. In practice that would mean the top 3 or so in the stack (StackManager.snapshot())

Comment 13

[Aus Lacroix [:aus]]

Going to have a look at this ...

Comment 14

[Aus Lacroix [:aus]]

per lmandel request, a quick update: Should be done by Wednesday... There's not really much to do here other than wait to load the icons in the task manager. The dependent bug should take care of the rest.

Comment 15

[Aus Lacroix [:aus]]

So, while digging through all this stuff, I see that we already should be unloading everything that we can:

  Card.prototype.onOutViewport = function c_onOutViewport(event) {
    this.element.style.display = 'none';
  };

In apps/system/js/card.js (line 251), already has the effect of unloading the uncompressed icon data from memory. Compressed data would also be tossed once the dependent bug is fixed.

There is no actionable item for 2.0 in this bug.

Comment 16

[Gregor Wagner [:gwagner]]

benwa, do you still see this issue? Is this only on master?

Comment 17

[Aus Lacroix [:aus]]

Quick update here: This bug at this point is only to verify that the dependent patch does make the impact that we're hoping for here. See https://bugzilla.mozilla.org/show_bug.cgi?id=1042393 for more details. As far as I can tell, no code changes are required by this specific bug.

Comment 18

[Benoit Girard (:BenWa)]

Looks like there no point in checking until the dependand bug is fixed. I don't know if other branches are affected.

Comment 19

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> This bug is about the system app.  It has nothing to do with the homescreen.

isn't this bug created as dependent bug for bug 1029902 to minimize Homescreen USS ? We are tracking it for that purpose only. Please help us to understand it better.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug is about the system app.  It has nothing to do with the homescreen.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should unblock on this.  See the rationale in bug 1042393 comment 4.

This is on the CAF list due to a miscommunication as far as I can understand.  See comments 10, 19, and 20.  Nothing in here will help with the homescreen.  It will nominally (less than 1 MB now that bug 1039631 has landed) reduce the memory usage in the main b2g process.

Comment 22

[Aus Lacroix [:aus]]

I'm in agreement with Kyle. It makes little sense to block on this now. We will see some impact once the dependent bug is fixed but it will be very slim in comparison to the gains made by removing the gradients and other background images (which saved 1MB *PER PROCESS*).

Comment 23

[Aus Lacroix [:aus]]

This bug also remains opened purely for verification purposes. We want to check that the patch for bug 1042393 also reduces memory usage in the system app. 

We could realistically duplicate this bug against 1042393 to reduce the amount of confusion that is happening. All applications will see benefits from the resolution of bug 1042393 so why have an application specific bug open as well?

Comment 24

[Jason Smith [:jsmith]]

Backlog per comment 21.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Tapas, I don't think this belongs on the CAF list.  See comment 21.  If you still think it does we need to sort this out ASAP.

Comment 26

[Inder]

Considering that this approach has risks (bug 1042393), we are unblocking on it.

Comment 27

[Aus Lacroix [:aus]]

I'll keep this on my radar to verify when 1042393 lands.

Comment 28

[Aus Lacroix [:aus]]

The dependent bug hasn't moved in a while. I'm going to reset the assignee here.

Comment 29

[Eric Rahm [:erahm]]

Closing out old Firefox OS specific memshrink bugs.