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

In FFOS v2.0, any FFOS app will release ION memory if we put it in background. 

Consider a use case when we are playing video and pressing home key for a long time to bring up "cards view" window so that we can kill video app manually on device . 

But during app transition, both video app and homescreen app becomes forground for a moment. This happens randomly and it can be confirmed from b2g-info/logcat logs

Result is that Homesreen app allocates new ION memory for tiler layer and video app does not allow media-server to delete 20MB ION memory. So we are seeing >40MB ION allocationg during app transition. Normally we see only 29MB ION allocation during video playback. This ION spike causes kernel 'kswapd0' to use memory to swapout old process on 256MB device . Result is that kernel Low Memory Killer is killing video app randomly. 


@alive: it seems like we are doing something like this now for above use case :
1) Assign homescreen app to OOM_ADJ=2 to make it forground app immediately after pressing homekey for a long time
2) We are not putting video app in background before making homescreen as foreground process. 

Can we do something like below :
1) Put video app in background and don't put homescreen to foreground when we press home key for long time ('card view' window to kill video app manually).
2) Assign homescreen app to OOM_ADJ=2 only it is visible on device.


@sotaro: Any better idea for allocation/deletion for homescreen tiler ION memory ?

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

[Blocking Requested - why for this release]:

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

@timdream: Could you please help me for Comment 0 ?

	Sotaro Ikeda [:sotaro]
	Comment 3 • 10 years ago

> 
> @sotaro: Any better idea for allocation/deletion for homescreen tiler ION
> memory ?

From gfx Layers and video elements just allocates memory when an application becomes a foreground state. The following is the actual cause of the problem. It seems better to investigate about this problem by WindowManager's engineers. 

> But during app transition, both video app and homescreen app becomes forground for a moment.

	Sotaro Ikeda [:sotaro]
	Comment 4 • 10 years ago

Changed to a correct component.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 5 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > 
> > @sotaro: Any better idea for allocation/deletion for homescreen tiler ION
> > memory ?
> 
> From gfx Layers and video elements just allocates memory when an application
> becomes a foreground state. The following is the actual cause of the
> problem. It seems better to investigate about this problem by
> WindowManager's engineers. 
> 
> > But during app transition, both video app and homescreen app becomes forground for a moment.

It's because if we don't put these two apps at the foreground during transitioning,
you will see one of them is empty because gecko will drop the rendering once an app is going to background and cause bad user experience.

The long term way to fix is implement https://bugzilla.mozilla.org/show_bug.cgi?id=1034001
and the short term way might be just go to background on a low end device.

But unfortunately we don't have the way to identify a device is high end or low end in gaia side.

Both of these two proposals are not gaia dev's round.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 6 • 10 years ago

What Alive said is correct.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> The long term way to fix is implement
> https://bugzilla.mozilla.org/show_bug.cgi?id=1034001
> and the short term way might be just go to background on a low end device.
> 
> But unfortunately we don't have the way to identify a device is high end or
> low end in gaia side.

We do, since 2.0 we are able to get the total memory of the device from getFeature() API.
Do you think this is feasible to be work on? Could you mentor George or anyone in Systems FE to do it?

Flagging Sam and George too until we could find people to work on it or if the above is deemed not useful.
Falgging :fxosux for UX change/proposal.

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

There is work going into bug 1064185 and bug 1061324 which I think will be relevant here. Although that should be complete this week, it is not blocking 2.1, so we may need to extract an uplift-able patch there. We are also seeing issues with losing screenshots from card view that appear to be memory related (bug 1057300). AFAIK we don't currently actively manage the number of app window screenshots we keep, or the number of apps displayed in the card view (or edge swipe - which uses the same stack). 

It seems like at some threshold that is based on total memory of the device, we should remove the screenshot for app windows lower down the stack. We also need to add logic to consumers of the screenshots to be able to display the app icon when no screenshot is available.

	Stephany Wilkes
	Comment 8 • 10 years ago

Sorry - Alive, can you clarify the UX request here? What would our new proposal need to cover? Thanks.

	bhavana bajaj [:bajaj]
	Comment 9 • 10 years ago

Blocking on this given this blocks CAF's sign-off on 8x10. Looks like Tim's recommendation for 2.0 in comment #6 is the best way to go in the short term.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 10 • 10 years ago

The UX change is we will loose any closing transition from this proposal. The user will just see homescreen being zoom out when press home button.

(In reply to Stephany Wilkes from comment #8)
> Sorry - Alive, can you clarify the UX request here? What would our new
> proposal need to cover? Thanks.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 11 • 10 years ago

(In reply to Sam Foster [:sfoster] from comment #7)
> There is work going into bug 1064185 and bug 1061324 which I think will be
> relevant here. Although that should be complete this week, it is not
> blocking 2.1, so we may need to extract an uplift-able patch there. We are
> also seeing issues with losing screenshots from card view that appear to be
> memory related (bug 1057300). AFAIK we don't currently actively manage the
> number of app window screenshots we keep, or the number of apps displayed in
> the card view (or edge swipe - which uses the same stack). 
> 
> It seems like at some threshold that is based on total memory of the device,
> we should remove the screenshot for app windows lower down the stack. We
> also need to add logic to consumers of the screenshots to be able to display
> the app icon when no screenshot is available.

Yes but I suspect it's dangerous for v2.0,
so let's just setVisible(false) when doing closing transition for all apps.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 12 • 10 years ago

The next step is detecting hardware memory before doing setVisible(false) but let's see if this fixes your issue.

	Stephany Wilkes
	Comment 13 • 10 years ago

Thanks for the clarification, Alive. Flagging Rob on the homescreen transition issue.

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

Comment on attachment 8486983 [details]
wip for v2.0

I am still seeing same issue in Comment 0:

                          |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS SWAP VSIZE OOM_ADJ USER
            b2g  227    1   51.3    0 24.1 25.7 30.0 27.2 261.8       0 root
         (Nuwa)  952  227    1.3    0  0.0  0.6  3.0  7.3  53.8       0 root
     Homescreen 1036  952   10.9    1 12.6 14.2 18.7  7.8  78.6       2 u0_a1036
          Video 1258  952   24.4    1  8.8 10.9 15.9  8.8  99.0       2 u0_a1258
(Preallocated a 1861  952    0.9   18  0.0  0.8  3.9 10.6  60.9       1 u0_a1861

	Rob MacDonald [:robmac]
	Comment 15 • 10 years ago

Flagging Eric as I'll be out of the office tomorrow and he has been managing motion design / transition issues. Thanks Eric!

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

Could you please give me a low risk temporary working patch here ? It will help me to debug another internal stability issue in parallel. We are blocked on that issue because of this issue .

	Eric Pang [:epang] UX
	Comment 17 • 10 years ago

Hey Alive, can I have more clarification?  I'm trying to understand the bug...Is the issue when you launch task manager from Video (with a long press)?  

Is there a specific part of the transition that is causing the issue?  Is it the resizing?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 18 • 10 years ago

Isn't comment 12 addressing your problem here?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 19 • 10 years ago

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #14)
> Comment on attachment 8486983 [details]
> wip for v2.0
> 
> I am still seeing same issue in Comment 0:
> 
>                           |     megabytes     |
>            NAME  PID PPID CPU(s) NICE  USS  PSS  RSS SWAP VSIZE OOM_ADJ USER
>             b2g  227    1   51.3    0 24.1 25.7 30.0 27.2 261.8       0 root
>          (Nuwa)  952  227    1.3    0  0.0  0.6  3.0  7.3  53.8       0 root
>      Homescreen 1036  952   10.9    1 12.6 14.2 18.7  7.8  78.6       2
> u0_a1036
>           Video 1258  952   24.4    1  8.8 10.9 15.9  8.8  99.0       2
> u0_a1258
> (Preallocated a 1861  952    0.9   18  0.0  0.8  3.9 10.6  60.9       1
> u0_a1861

please give me precise STR, it's confusing now you are pressing home or long pressing home.
I guess we are totally at different page, again.
press home and long press home is something totally different...

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

Here is two STRs:

1st STR:

1) Open video app, play video and press homekey for a LONG time to see 'card view' so that you can kill video app.

   Expectation: Video app will go to background and homescreen should not become foreground app at any point during app transition. Both homescreen app and Video app should have OOM_ADJ > 2. This will force both video app to release memory and homescreen app not to allocated any memory for tiles. Please note that we are not displaying homescreen app in cardview window. So it is harmless to put homescreen in background.

2) If user press home key again then we will display homescreen window. 
   Expectation: Video app will remain background and homescreen should become foreground now as it is displayed now. Please note that we don't want to see both homescreen app and video app are assigned to OOM_ADJ=2 at any point in app transition here too.

2nd STR:
1) Open video app, play video and press homekey (You are not pressing for long time). 
    Expectation: Video app will be in background before we make homescreen foreground during app transition. Please note that we don't want to see both homescreen app and video app  are assigned to OOM_ADJ=2 at any point in app transition here too.


Hope it is clear now !

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 21 • 10 years ago

Well, this is talking about: invoking task manager will not put homescreen at foreground. Will update patch soon.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 22 • 10 years ago

This patch is 2.0 only and:
* It does not open homescreen window on task manager shown anymore. (Already introduced in master)
* But still update active app to homescreen to avoid UX change. (Different from master)
* Skip sending app to foreground if there is active attention screen/lockscreen in AppTransitionController (note: this is introducing additional dependency into ATC but it's already fixed in master)

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> Created attachment 8489200 [details]
> v2.0-only patch
> 
> This patch is 2.0 only and:
> * It does not open homescreen window on task manager shown anymore. (Already
> introduced in master)
> * But still update active app to homescreen to avoid UX change. (Different
> from master)
> * Skip sending app to foreground if there is active attention
> screen/lockscreen in AppTransitionController (note: this is introducing
> additional dependency into ATC but it's already fixed in master)

Do I need to cherry-pick any other dependent fix before applying this patch in v2.0 ?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 24 • 10 years ago

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #23)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> > Created attachment 8489200 [details]
> > v2.0-only patch
> > 
> > This patch is 2.0 only and:
> > * It does not open homescreen window on task manager shown anymore. (Already
> > introduced in master)
> > * But still update active app to homescreen to avoid UX change. (Different
> > from master)
> > * Skip sending app to foreground if there is active attention
> > screen/lockscreen in AppTransitionController (note: this is introducing
> > additional dependency into ATC but it's already fixed in master)
> 
> Do I need to cherry-pick any other dependent fix before applying this patch
> in v2.0 ?

No, unless you are testing bug 1055299 in this bug.

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> Created attachment 8489200 [details]
> v2.0-only patch
> 
> This patch is 2.0 only and:
> * It does not open homescreen window on task manager shown anymore. (Already
> introduced in master)

Was confused here for a minute. To clarify, the behavior where we open the homescreen window when showing the task manager was changed in bug 1047143, which is in 2.1 but not 2.0.

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

Comment on attachment 8489200 [details]
v2.0-only patch

This patch looks good to me as it is fixing 1st STR of Comment 20. But it does not fix 2nd STR of Comment 20. 

I can still see both video app and homescreen app is becoming foreground app for 2nd STR in Comment 20: (both video and homescreen has OOM_ADJ=2)

                         |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS SWAP VSIZE OOM_ADJ USER    
            b2g  228    1   60.3    0 27.9 30.1 36.8 24.5 257.2       0 root    
         (Nuwa)  941  228    2.4    0  0.0  0.9  4.7  7.1  53.8       0 root    
     Homescreen 1016  941    8.1    1  9.6 12.2 19.4 10.4  84.0       2 u0_a1016
          Video 1179  941   42.0    1 10.2 13.2 20.8  6.1  95.4       2 u0_a1179
(Preallocated a 1371  941    0.9   18  0.3  1.7  6.7 10.0  60.9       1 u0_a1371




It can be detected easily by following 2nd STR in Comment 20 and running below command :

|while true; do adb shell b2g-info ; adb shell cat /d/ion/heaps/system | grep 'total ' ; done|

I am putting -ve FEEDBACK as this patch is not fixing 2nd STR of comment 20.

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

For 2nd STR in Comment 20, my expectation is that FFOS must put video app in background before making Homescreen as foreground . This will save at least 10MB memory usage spike during app transition for video app in 256MB target. 

Once we get another patch for 2nd STR of Comment 20 then this bug can be closed IMO.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 28 • 10 years ago

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #27)
> For 2nd STR in Comment 20, my expectation is that FFOS must put video app in
> background before making Homescreen as foreground . This will save at least
> 10MB memory usage spike during app transition for video app in 256MB target. 
> 
> Once we get another patch for 2nd STR of Comment 20 then this bug can be
> closed IMO.

It's impossible to make this kind of order happen. We don't know when gecko will change OOM_ADJ.
What we could only do is change the page visibility "at the same time".

Could we stop fixing everything in one bug? They are different things and this one involves gecko change if the first patch I gave you doesn't fix.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 29 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28)
> (In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from
> comment #27)
> > For 2nd STR in Comment 20, my expectation is that FFOS must put video app in
> > background before making Homescreen as foreground . This will save at least
> > 10MB memory usage spike during app transition for video app in 256MB target. 
> > 
> > Once we get another patch for 2nd STR of Comment 20 then this bug can be
> > closed IMO.
> 
> It's impossible to make this kind of order happen. We don't know when gecko
> will change OOM_ADJ.
> What we could only do is change the page visibility "at the same time".
> 
> Could we stop fixing everything in one bug? They are different things and
> this one involves gecko change if the first patch I gave you doesn't fix.

Sum up:
It seems What tapas want is *we definitely do not want to see there is two foreground app at the same time*, but this kind of change is very risky and dangerous and will have user experience impact when switching apps. I am still going to make a patch but I highly suggest we don't do this.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 30 • 10 years ago

Comment on attachment 8489200 [details]
v2.0-only patch

Part 2:
* Do not wait the next app to be ready anymore (this one should be low-end device only).
* If next app is homescreen, wait the previous app going to background before setting foreground.

The impact of this part is the user will not see any closing animation of normal app and opening animation of home anymore. This is a big UX change.

Also this is a mechanism change to WindowManager so I don't promise no regression will happen.

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #30)
> Comment on attachment 8489200 [details]
> v2.0-only patch
> 
> Part 2:
> * Do not wait the next app to be ready anymore (this one should be low-end
> device only).
> * If next app is homescreen, wait the previous app going to background
> before setting foreground.
> 
> Also this is a mechanism change to WindowManager so I don't promise no
> regression will happen.

It seems like it is better if we don't land this 'part 2' patch. Could you please land patch from comment 22 as a solution of this bug ? Please also uplift this to master branch. Thanks for your help.


> The impact of this part is the user will not see any closing animation of
> normal app and opening animation of home anymore. This is a big UX change.
> 

hmm. We definitely don't want to do this type of change in FFOS 2.0 at this stage. But we may want to do this for future 256MB targets. Could you please create a new bugid for this ? This seems to be potential memory optimization which can always save >10MB  (may be >20MB too in some use cases)  memory during app transition which is indeed big savings for 256MB target.

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

Comment on attachment 8489200 [details]
v2.0-only patch

Please land this in 2.0 and also uplift for master (if applicable)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 33 • 10 years ago

I am going to drop the part 2 for this issue and ask approval of part 1.
We don't need it in master/v2.1 because homescreen is always background in task manager after v2.1 according to comment 25.

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #32)
> Comment on attachment 8489200 [details]
> v2.0-only patch
> 
> Please land this in 2.0 and also uplift for master (if applicable)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 34 • 10 years ago

Comment on attachment 8489200 [details]
v2.0-only patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
This change is partially borrowed from bug 1047143,
* Put homescreen to background while task manager is displayed
[Bug caused by] (feature/regressing bug #):
We had this issue from v1.0.1: never send homescreen to background.
[User impact] if declined:
Not user facing impact.
[Testing completed]:
Yes
[Risk to taking this patch] (and alternatives if risky):
Riskless since it's already running in master.
[String changes made]:
No.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 35 • 10 years ago

(In reply to Sam Foster [:sfoster] from comment #25)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> > Created attachment 8489200 [details]
> > v2.0-only patch
> > 
> > This patch is 2.0 only and:
> > * It does not open homescreen window on task manager shown anymore. (Already
> > introduced in master)
> 
> Was confused here for a minute. To clarify, the behavior where we open the
> homescreen window when showing the task manager was changed in bug 1047143,
> which is in 2.1 but not 2.0.

Sorry if I don't notice you that I borrowed some change from bug 1047143 because I think you are working some other stuff so I decide to fix this issue on my own.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 36 • 10 years ago

All green, merging.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 37 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/31434a3949556171f3565ca47ac2b44e810e95e6

	Kai-Zhen Li [:seinlin][:kli]
	Comment 38 • 10 years ago

v2.0m: https://github.com/mozilla-b2g/gaia/commit/8caf6744371ee990b51ddc87b9648b3ce85966bd

	sarsenyev
	Comment 39 • 10 years ago

Verified the bug is fixed on 2.0,
As per comment
The homescreen is not on foreground while task manager is displayed
As per comment 33 in 2.1 and 2.2 don't need it in master/v2.1 because homescreen is always background in task manager after v2.1 according to comment 25 in bug 1047143

Device: Flame 2.0
BuildID: 20141029000205
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: de8cfd54bf93
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

	Brogan Zumwalt [Inactive]
	Comment 40 • 10 years ago

In 2.0 on Flame I am seeing Homescreen set to OOM_ADJ=2 during and after transition from card view to homescreen (Step 2 from comment 20) while the Video app is set to OOM_ADJ=10 

Is this acceptable behavior since the Homescreen is in the foreground?

	Brogan Zumwalt [Inactive]
	Comment 41 • 10 years ago

Upon reexamination I believe I have misread the expected results for the STR. Apologies.

	Coler
	Comment 42 • 10 years ago

This issue has been verified successfully on Woodduck 2.0 with Comment 20.
Reproducing rate: 0/5
See attachment: Verify_Woodduck_Taskmanager.mp4

Woodduck build version:
Gaia-Rev        d742e375aca6dc1bf3a36638000ad7f5338ef457
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141127050313
Version         32.0

	Alissa（leave from Mozilla）
	Comment 44 • 10 years ago

This issue has been successfully verified on Flame 2.1, 2.2
Reproducing rate: 0/5

FLame2.1:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0
Flame 2.2:
Gaia-Rev        39214fb22c203e8849aaa1c27b773eeb73212921
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/08be3008650f
Build-ID        20141201040205
Version         37.0a1