Benoit Girard (:BenWa) Reporter
	Description • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 1 • 10 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 3 • 10 years ago

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

	Sam Foster [:sfoster] (he/him)
	Comment 4 • 10 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 5 • 10 years ago

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

	Benoit Girard (:BenWa) Reporter
	Comment 6 • 10 years ago

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.

	Preeti Raghunath(:Preeti)
	Comment 7 • 10 years ago

Milan

Please provide next steps here.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 8 • 10 years ago

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.

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)
	Comment 9 • 10 years ago

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)

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 10 • 10 years ago

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

	Kai-Chih Hu [:khu]
	Comment 11 • 10 years ago

(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

	Sam Foster [:sfoster] (he/him)
	Comment 12 • 10 years ago

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

	Aus Lacroix [:aus]
	Comment 13 • 10 years ago

Going to have a look at this ...

	Aus Lacroix [:aus]
	Comment 14 • 10 years ago

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.

	Aus Lacroix [:aus]
	Comment 15 • 10 years ago

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.

	Gregor Wagner [:gwagner]
	Comment 16 • 10 years ago

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

	Aus Lacroix [:aus]
	Comment 17 • 10 years ago

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.

	Benoit Girard (:BenWa) Reporter
	Comment 18 • 10 years ago

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

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)
	Comment 19 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 20 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 10 years ago

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.

	Aus Lacroix [:aus]
	Comment 22 • 10 years ago

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

	Aus Lacroix [:aus]
	Comment 23 • 10 years ago

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?

	Jason Smith [:jsmith]
	Comment 24 • 10 years ago

Backlog per comment 21.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 25 • 10 years ago

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.

	Inder
	Comment 26 • 10 years ago

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

	Aus Lacroix [:aus]
	Comment 27 • 10 years ago

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

	Aus Lacroix [:aus]
	Comment 28 • 10 years ago

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

	Eric Rahm [:erahm]
	Comment 29 • 7 years ago

Closing out old Firefox OS specific memshrink bugs.