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)
Firefox OS Graveyard
Gaia::System
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.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [MemShrink]
Cost control is known not to setVisible(false) properly. Does killing the cost control app make this memory usage go away?
Reporter | ||
Comment 2•12 years ago
|
||
(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.
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Reporter | ||
Comment 4•12 years ago
|
||
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]
Reporter | ||
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Reporter | ||
Comment 5•12 years ago
|
||
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...
Reporter | ||
Comment 6•12 years ago
|
||
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]
Comment 7•12 years ago
|
||
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?
Reporter | ||
Comment 8•12 years ago
|
||
> 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.
Assignee | ||
Comment 9•12 years ago
|
||
Me or Alive can implement comment 8 if triage ppl thinks this is a blocking+ feature.
Assignee: nobody → timdream+bugs
blocking-basecamp: --- → ?
Reporter | ||
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
(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
Reporter | ||
Comment 12•12 years ago
|
||
> 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.
Comment 13•12 years ago
|
||
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
Assignee | ||
Comment 14•11 years ago
|
||
Joe, who do we call out to prioritize tarako work? This is probably another low-hanging fruit to take.
Flags: needinfo?(jcheng)
Comment 15•11 years ago
|
||
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]
Comment 16•11 years ago
|
||
Tim
What component does the bug fall in?
Also are we using a separate branch for tarako?
Flags: needinfo?(timdream)
Assignee | ||
Comment 17•11 years ago
|
||
This is a Gaia:System bug.
Component: General → Gaia::System
Flags: needinfo?(timdream)
Comment 19•11 years ago
|
||
triage: waiting for gregor's confirmation before deciding whether this should be uplifted for tarako
Comment 20•11 years ago
|
||
Joe,
Is this tarako specific? Or more of a general issue?
Flags: needinfo?(jcheng)
Assignee | ||
Comment 21•11 years ago
|
||
(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)
Comment 22•11 years ago
|
||
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)
Comment 23•11 years ago
|
||
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
Comment 24•11 years ago
|
||
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
Updated•11 years ago
|
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 → ---
Updated•11 years ago
|
Whiteboard: [MemShrink:P2][slim:2.5mb][tarako] → [MemShrink:P2][slim:2.5mb][tarako-p2]
Comment 26•11 years ago
|
||
hi Tim, will you have some bandwidth to look into this further? thanks
Flags: needinfo?(timdream)
Assignee | ||
Comment 27•11 years ago
|
||
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)
Comment 28•11 years ago
|
||
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.
Assignee | ||
Comment 29•11 years ago
|
||
(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
Assignee | ||
Comment 30•11 years ago
|
||
One other possibility would be comment 12. However I suspect the amount of code in Gaia for this could outweigh the benefit?
Comment 31•11 years ago
|
||
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.
Assignee | ||
Comment 32•11 years ago
|
||
(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.
Assignee | ||
Comment 33•11 years ago
|
||
s/transaction/transition/
Updated•11 years ago
|
Comment 34•11 years ago
|
||
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)
Updated•11 years ago
|
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 ago → 11 years ago
Resolution: --- → WONTFIX
blocking-b2g: 1.3T? → ---
Updated•11 years ago
|
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.
Description
•