Closed Bug 812948 Opened 12 years ago Closed 11 years ago

While in the gallery app, main process holds alive 2.5mb of decoded images

Categories

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

defect

Tracking

(blocking-basecamp:-)

RESOLVED WONTFIX
blocking-basecamp -

People

(Reporter: justin.lebar+bug, Assigned: timdream)

References

Details

(Keywords: memory-footprint, perf, Whiteboard: [c=memory p= s=2014.02.14 u=] [MemShrink:P2][slim:2.5mb])

In the main process: ├───────2,710,989 B (00.01%) -- images │ ├──2,710,557 B (00.01%) -- content │ │ ├──2,710,557 B (00.01%) -- used │ │ │ ├──2,500,272 B (00.01%) ── uncompressed-heap (Ignore the percentages here; they're incorrect.) This is probably due to image locking and some frame in the main process is not being marked as not-visible. Yeouch.
Whiteboard: [MemShrink]
Cost control is known not to setVisible(false) properly. Does killing the cost control app make this memory usage go away?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > Cost control is known not to setVisible(false) properly. Does killing the > cost control app make this memory usage go away? I don't have a working build at the moment, but I strongly suspect that whatever frame isn't setVisibile()'ing lives in the main process.
That's what I get for commenting before my coffee :). Ignore me.
Whiteboard: [MemShrink] → [MemShrink:P2]
This probably happens the same with "while using the calculator app ..." or any other app; I just haven't tested to verify that.
Whiteboard: [MemShrink:P2] → [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
It looks like the system app's main frame, gaia/apps/system/index.html, doesn't get setVisible(false)'ed. We can't do that without making all apps invisible. I think this image is the wallpaper. At least, that's one of the images here...
I'd like us to see if we can do something here, but I don't understand the gaia code well enough to do it myself. For example, it might be sufficient if we removed the backgroundImage style on gaia/apps/system/index.html's screen element when the homescreen isn't showing.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][slim:2.5mb]
I may misunderstand the problem, but if you remove the background image from system app to homescreen, you will see empty screen when homescreen crashed and before it restarts..very weird. Or do you mean remove the style if an foreground app which is not homescreen app is displaying?
> I may misunderstand the problem, but if you remove the background image from system app > to homescreen, you will see empty screen when homescreen crashed and before it > restarts..very weird. Yes, we can't do that for exactly the reason you describe. > Or do you mean remove the style if an foreground app which is not homescreen app is > displaying? Yes, exactly this.
Me or Alive can implement comment 8 if triage ppl thinks this is a blocking+ feature.
Assignee: nobody → timdream+bugs
blocking-basecamp: --- → ?
FWIW I'd consider this to be a relatively large memory win for a relatively small amount of effort -- that is, it falls on the good side of the cost-benefit ratio spectrum for memory fixes. I also suspect that there may be more than 2.5mb to be had here; the memory reporters are incomplete, but it looks like there are other images also held alive in the parent process (maybe also the lockscreen background?). But it's hard to tell what's going on without fixing this bug first. In other words, I would like this bug fixed as part of our ongoing focus on memory usage.
(In reply to Justin Lebar [:jlebar] from comment #8) > > I may misunderstand the problem, but if you remove the background image from system app > > to homescreen, you will see empty screen when homescreen crashed and before it > > restarts..very weird. > > Yes, we can't do that for exactly the reason you describe. > > > Or do you mean remove the style if an foreground app which is not homescreen app is > > displaying? > > Yes, exactly this. Uh, I think this would be a non-trivial change because there're many cases to solve: 1. Home button event 2. Holdhome button event 3. App transitioning including disposition:window activity and foreground app crashed. 4. Any other thing could lead to homescreen or reveal part of homescreen You may see the screen element rapidly and often change the style:backgroundImage, is this you want? I wonder this may be overlapped with another bug which is about do not use dataURI in wallpaper. I don't know what :djf is going to do with dataURI stuff in https://bugzilla.mozilla.org/show_bug.cgi?id=820940#c3
> You may see the screen element rapidly and often change the style:backgroundImage, is > this you want? I guess what I really want is for the background to live in its own in-process mozbrowser iframe that's under the system app. Then we can setVisible(false) on it and be done with it. I don't know if that's feasible.
Depends on: 820940
This isn't something we can block on but we'd still like to see it fixed. Justin can use his magical uplift powers for a patch here.
blocking-basecamp: ? → -
Priority: -- → P4
Joe, who do we call out to prioritize tarako work? This is probably another low-hanging fruit to take.
Flags: needinfo?(jcheng)
adding [tarako] and 1.3? to triage this
blocking-b2g: --- → 1.3?
Flags: needinfo?(jcheng)
Whiteboard: [MemShrink:P2][slim:2.5mb] → [MemShrink:P2][slim:2.5mb][tarako]
Tim What component does the bug fall in? Also are we using a separate branch for tarako?
Flags: needinfo?(timdream)
This is a Gaia:System bug.
Component: General → Gaia::System
Flags: needinfo?(timdream)
I will see if this is still valid.
Flags: needinfo?(anygregor)
triage: waiting for gregor's confirmation before deciding whether this should be uplifted for tarako
Joe, Is this tarako specific? Or more of a general issue?
Flags: needinfo?(jcheng)
(In reply to Preeti Raghunath(:Preeti) from comment #20) > Joe, > > Is this tarako specific? Or more of a general issue? This is of course a general issue -- note the filed date of the bug -- it's just that the urgency of the bug need to be reevaluated given the fact we have new memory requirement.
Flags: needinfo?(jcheng)
On my nexus4 I get now: ├───4.34 MB (09.15%) -- images │ ├──4.34 MB (09.15%) -- content │ │ ├──4.34 MB (09.15%) -- used │ │ │ ├──3.68 MB (07.75%) ── uncompressed-heap │ │ │ ├──0.67 MB (01.40%) ── raw │ │ │ └──0.00 MB (00.00%) ── uncompressed-nonheap │ │ └──0.00 MB (00.00%) ++ unused │ └──0.00 MB (00.00%) ++ chrome
Flags: needinfo?(anygregor)
But it disappears after a GC even if the gallery app is still open: ├───0.76 MB (05.83%) -- images │ ├──0.76 MB (05.83%) -- content │ │ ├──0.76 MB (05.83%) -- used │ │ │ ├──0.76 MB (05.81%) ── raw │ │ │ └──0.00 MB (00.02%) ++ (2 tiny) │ │ └──0.00 MB (00.00%) ++ unused
triage: base on the latest data, GC will remove the additional memory use. Let GC handle this. Resolve as won't fix
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
blocking-b2g: 1.3? → ---
This should not be WONTFIX, at least without further investigation. Transient spikes in memory usage can cause other apps to get OOM killed, even if the memory is quickly reclaimed by the GC.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Whiteboard: [MemShrink:P2][slim:2.5mb][tarako] → [MemShrink:P2][slim:2.5mb][tarako-p2]
hi Tim, will you have some bandwidth to look into this further? thanks
Flags: needinfo?(timdream)
Interesting. I actually don't know how to do any better than GC to reclaim memory in Javascript in this case. We *do* need the image when the home screen is visible, and there isn't an obvious way to explicitly to remove the decoded image from memory when the user moved to an app. khuey, any particular and strait-forward in mind?
Status: REOPENED → NEW
Flags: needinfo?(timdream) → needinfo?(khuey)
We (used to) trigger a GC when an app goes to the background. Is this still the case? The description and further comments suggest that we don't handle the invisible state correctly.
(In reply to Gregor Wagner [:gwagner] from comment #28) > We (used to) trigger a GC when an app goes to the background. Is this still > the case? The description and further comments suggest that we don't handle > the invisible state correctly. System app never goes to background :P
One other possibility would be comment 12. However I suspect the amount of code in Gaia for this could outweigh the benefit?
I played a bit with removing the background in apps/system/js/bootstrap.js and apps/system/js/lockscreen.js : I've never got the 'images' down as much as in comment 23, but it looks like we spend around 1MB or uncompressed image data for the backgrounds. Note that the lockscreen is setting the background of 3 panels at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/lockscreen.js#L891 - Tim & Alive, why do we need several panels there? 1MB is roughly what it takes to store 4 24bits 320x480 images. So we could save 500k just by having a single background for the lock screen.
(In reply to Fabrice Desré [:fabrice] from comment #31) > Note that the lockscreen is setting the background of 3 panels at > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/lockscreen. > js#L891 - Tim & Alive, why do we need several panels there? 1MB is roughly > what it takes to store 4 24bits 320x480 images. So we could save 500k just > by having a single background for the lock screen. The backgrounds were set to the panels because the transaction between panels was sideways -- it's still do for emergency call (but the emergency call panel don't need a background though). We could fix that. Bug 956621 filed.
s/transaction/transition/
Keywords: footprint, perf
Whiteboard: [MemShrink:P2][slim:2.5mb][tarako-p2] → [c=memory p= s= u=tarako] [MemShrink:P2][slim:2.5mb][tarako-p2]
moving to 1.3T? flag and therefore removing [Tarako] whiteboard
blocking-b2g: --- → 1.3T?
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #27) > Interesting. I actually don't know how to do any better than GC to reclaim > memory in Javascript in this case. We *do* need the image when the home > screen is visible, and there isn't an obvious way to explicitly to remove > the decoded image from memory when the user moved to an app. > > khuey, any particular and strait-forward in mind? I don't really know what the system app is doing here, but there are ways to trigger image data to discard immediately. Let's talk next week?
Flags: needinfo?(khuey)
Whiteboard: [c=memory p= s= u=tarako] [MemShrink:P2][slim:2.5mb][tarako-p2] → [c=memory p= s= u=tarako] [MemShrink:P2][slim:2.5mb]
We talked about this in person and concluded that there's nothing to do here. The memory will be reclaimed by Gecko under memory pressure, so having Gaia manage it manually does not make sense.
Status: NEW → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → WONTFIX
Whiteboard: [c=memory p= s= u=tarako] [MemShrink:P2][slim:2.5mb] → [c=memory p= s=2014.02.14 u=] [MemShrink:P2][slim:2.5mb]
You need to log in before you can comment on or make changes to this bug.