1142806 - Follow-up Bug 852925 OOM_ADJ should not change for the topmost foreground task in the card view

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

+++ This bug was initially created as a clone of Bug #852925 +++

This is follow-up of bug 852925 and should address the issue defined in the title and discussed in bug 852925.

When the card view is entered, the OOM_ADJ value for the (previously) foreground application drops below the homescreen, preallocated process, and higher priority background processes.  This makes it highly likely in a RAM constrained device that the (previously) foreground application will be killed in the simple scenario of:

1. User is in some app (say composing an email).
2. User press and holds home to enter the card view
3. User decides to "abort" the card view and returns to the app. (email is gone)

Instead the OOM_ADJ values could be frozen in the card view until the user selects a different application.

Comment 1

[Mike Lee [:mlee]]

Hi Gabriele,

As this is a follow-up to your work in bug 852925, what's the plan for fixing this and do you plan to create the fix within the fxOS 2.2 FC timeline?

Thanks,
Mike

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Mike Lee [:mlee] from comment #1)
> As this is a follow-up to your work in bug 852925, what's the plan for
> fixing this and do you plan to create the fix within the fxOS 2.2 FC
> timeline?

I will try but I'm not sure if I'll have enough spare cycles.

Comment 3

[Mike Lee [:mlee]]

Hi Francisco,

Since Gabriele is currently occupied with other critical issues please have someone else pick this up. Per CAF the scenario described in comment 0 leads to user data being lost if a user was composing an email before going to card view.

Thanks,
Mike

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Hi Mike,

I wish we could find another candidate to pick this up, but our team is out of resources.

Gabrielle do you have any suggestion for someone to picking this up, even if it's not in our team?

(In reply to Mike Lee [:mlee] from comment #3)
> Hi Francisco,
> 
> Since Gabriele is currently occupied with other critical issues please have
> someone else pick this up. Per CAF the scenario described in comment 0 leads
> to user data being lost if a user was composing an email before going to
> card view.
> 
> Thanks,
> Mike

Comment 5

[Gabriele Svelto [:gsvelto]]

There's two ways of fixing this: the first is in the system not to mark the former foreground card as hidden when entering the cards view.

This second can be achieved in the process priority manager by demoting the homescreen to a regular background priority before the last foreground task is sent into the background (when no other foreground tasks are present which is the scenario we have here). I think I might be able to do this between today and tomorrow but if I can't the person who's looked at this code more lately is Kyle Huey who's done most of my reviews. Ting-Yu Chou, Kan-Ru Chen and Sean Lin have also landed patches in the past so they might be able to handle this too.

Comment 6

[Gabriele Svelto [:gsvelto]]

I've tried working on this today but couldn't because I've had yet another dialer blocker landing in my lap yesterday. I'll be on PTO until the 6th of April so so I won't be able to be of further help before the FC date.

Comment 7

[Mike Lee [:mlee]]

Thinker & Marco,

As Gabriele & Kyle are both on PTO can you have someone on your team (i.e. Ting-Yu, Kan-Ru, or Sean) pick this up?

Thanks,
Mike

Comment 8

[Marco Chen [:mchen]]

Hi Thinker,

Could you help this issue because my team are working on different form-factors other then phone now?

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

I'll take a look.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Try to keep at least one foreground process

Whoever available please review.

I used third approach: try to keep at least one foreground process at any time. When a process is set to lower than foreground priority but it's the last foreground process, we delay the priority change until another process is set to foreground. That we could ensure "all processes are background" will not happen in normal operation.

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8589543 [details] [diff] [review]
Try to keep at least one foreground process

Review of attachment 8589543 [details] [diff] [review]:
-----------------------------------------------------------------

This is a valid approach too but it has a few complications I've highlighted below. There's another problem besides the actual changes and that's modifying the existing mochitests. This is going to break quite a few of them since they expect that a foreground process which goes into the background is demoted immediately; you should modify them to comply with your changes, possibly by always using two iframes when there's only one so that when one is sent into the background you bring the other one to the foreground. Additionally you should a new test to cover the new scenarios introduced by your patch.

::: dom/ipc/ProcessPriorityManager.cpp
@@ +678,5 @@
> +  nsRefPtr<ParticularProcessPriorityManager> aValue,
> +  void* aUserData)
> +{
> +  uint32_t* accumulator = static_cast<uint32_t*>(aUserData);
> +  if (aValue->CurrentPriority() == PROCESS_PRIORITY_FOREGROUND) {

You should count PROCESS_PRIORITY_FOREGROUND_HIGH processes too and not delay the priority downgrade if one is present.

@@ +1125,5 @@
> +      aPriority < PROCESS_PRIORITY_FOREGROUND &&
> +      ProcessPriorityManagerImpl::GetSingleton()->
> +        NumberOfForegroundProcesses() == 1) {
> +    LOGP("Attempting to demote the last foreground process is delayed.");
> +    ProcessPriorityManagerImpl::GetSingleton()->

You should also consider the case where the single process that was supposed to go into the background comes to the foreground again and the operation needs to be canceled.

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5998b5bd2d8d

Comment 13

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8590658 [details] [diff] [review]
patch v2

Review of attachment 8590658 [details] [diff] [review]:
-----------------------------------------------------------------

Great work fixing the tests and adding the new one but there's still a couple of issues to iron out.

::: dom/browser-element/mochitest/priority/test_Audio.html
@@ +17,5 @@
>  browserElementTestHelpers.setEnabledPref(true);
>  browserElementTestHelpers.addPermission();
>  browserElementTestHelpers.enableProcessPriorityManager();
>  
> +function setupTest() {

nit: Put a comment explaining why we have to do this here otherwise somebody who didn't know the rationale would find this odd. This applies to all other test files.

::: dom/ipc/ProcessPriorityManager.cpp
@@ +678,5 @@
> +  nsRefPtr<ParticularProcessPriorityManager> aValue,
> +  void* aUserData)
> +{
> +  uint32_t* accumulator = static_cast<uint32_t*>(aUserData);
> +  if (aValue->CurrentPriority() == PROCESS_PRIORITY_FOREGROUND) {

As I mentioned in my previous review comment you should also count FOREGROUND_HIGH processes. If one is present there is no point in delaying the priority change and in fact it could regress performance (though only marginally now that we have cgroup support).

@@ +1153,5 @@
>      ProcessPriorityToString(mPriority));
> +
> +  if (aPriority >= PROCESS_PRIORITY_FOREGROUND) {
> +    LOGP("More than one foreground processes. Run delayed priority change");
> +    ProcessPriorityManagerImpl::GetSingleton()->

What happens if the same process is sent into the background - thus delaying the priority change - and then back into the foreground? I get the feeling that this code will trigger and send it back into the background which is something we don't want. I think we should explicitly cancel the delayed change when this happens.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> Comment on attachment 8590658 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8590658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work fixing the tests and adding the new one but there's still a
> couple of issues to iron out.
> 
> ::: dom/browser-element/mochitest/priority/test_Audio.html
> @@ +17,5 @@
> >  browserElementTestHelpers.setEnabledPref(true);
> >  browserElementTestHelpers.addPermission();
> >  browserElementTestHelpers.enableProcessPriorityManager();
> >  
> > +function setupTest() {
> 
> nit: Put a comment explaining why we have to do this here otherwise somebody
> who didn't know the rationale would find this odd. This applies to all other
> test files.
> 
> ::: dom/ipc/ProcessPriorityManager.cpp
> @@ +678,5 @@
> > +  nsRefPtr<ParticularProcessPriorityManager> aValue,
> > +  void* aUserData)
> > +{
> > +  uint32_t* accumulator = static_cast<uint32_t*>(aUserData);
> > +  if (aValue->CurrentPriority() == PROCESS_PRIORITY_FOREGROUND) {
> 
> As I mentioned in my previous review comment you should also count
> FOREGROUND_HIGH processes. If one is present there is no point in delaying
> the priority change and in fact it could regress performance (though only
> marginally now that we have cgroup support).
> 
> @@ +1153,5 @@
> >      ProcessPriorityToString(mPriority));
> > +
> > +  if (aPriority >= PROCESS_PRIORITY_FOREGROUND) {
> > +    LOGP("More than one foreground processes. Run delayed priority change");
> > +    ProcessPriorityManagerImpl::GetSingleton()->
> 
> What happens if the same process is sent into the background - thus delaying
> the priority change - and then back into the foreground? I get the feeling
> that this code will trigger and send it back into the background which is
> something we don't want. I think we should explicitly cancel the delayed
> change when this happens.

ouch. I did fix the first and second issues but failed to refresh my patch.. new patch on the way.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch v3

Comment 16

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8590747 [details] [diff] [review]
patch v3

Review of attachment 8590747 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, excellent stuff!

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/246819a7ee89

Comment 18

[Michael Vines [:m1] [:evilmachines]]

Thanks guys, dom/ipc/ProcessPriorityManager.cpp from comment 17 doesn't apply to v2.2.  Could you please rebase so I can take it for a spin here before uplifting?

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/246819a7ee89

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch for v2.2

Here you go.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8592016 [details] [diff] [review]
patch for v2.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long standing bug
User impact if declined: On low memory devices the current foreground app can be killed when switching to card view even though there should be enough memory to keep it around
Testing completed: Tested on a device and in testsuite
Risk to taking this patch (and alternatives if risky): This delays changes in the priorities of processes when there is only one foreground process, if this is buggy or does not work as advertised after switching processes foreground/background some priorities might be wrong.
String or UUID changes made by this patch: None

Comment 22

[bhavana bajaj [:bajaj]]

:kanru, any additional testing that QA can help with here? Do you think its worth letting this bake on m-c for a few days before we uplift to the branch?

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to bhavana bajaj [:bajaj] from comment #22)
> :kanru, any additional testing that QA can help with here? 

I'm not sure.. maybe some smoke test? I think we could leverage Michael's QA resource here.

> Do you think its
> worth letting this bake on m-c for a few days before we uplift to the branch?

A few more days will definitely help us identify potential problems. So yes if we are not in a hurry.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Kan-Ru Chen [:kanru] from comment #23)
> (In reply to bhavana bajaj [:bajaj] from comment #22)
> > :kanru, any additional testing that QA can help with here? 
> 
> I'm not sure.. maybe some smoke test? I think we could leverage Michael's QA
> resource here.
> 
> > Do you think its
> > worth letting this bake on m-c for a few days before we uplift to the branch?
> 
> A few more days will definitely help us identify potential problems. So yes
> if we are not in a hurry.

:mvines/Inder, while we let this bake on our side, we think it'll be helpful if you guys can do some sanity testing around it to see if we discover any stability issues. The b2g37 patch is ready for you to pull into your repo, in-case you can help here. Thanks!

Comment 25

[Michael Vines [:m1] [:evilmachines]]

We've had attachment 8592016 [details] [diff] [review] in our tree since 4/14, so far no issues we've seen since have been traced back to it. 

http://shipitsquirrel.github.io

Comment 26

[Michael Vines [:m1] [:evilmachines]]

One behaviour I've observed is that even with this patch is that OOM_ADJ is not adjusted for all foreground apps.

eg:
1. Start Contacts
2. Add new Contact.
3. Select contact picture
4. Camera app starts.
5. Hold task switcher


|b2g-ps --oom| then looks something like this:

  APPLICATION    SEC  OOM_ADJ  OOM_SCORE  OOM_SCORE_ADJ  USER     PID   PPID   NAME
  b2g              0     0        200         0          root      5878  1     /system/b2g/b2g
  (Nuwa)           0     -17      0           -1000      root      5886  5878  /system/b2g/b2g
  Communications   2     12       768         667        u0_a10071 10071 5886  /system/b2g/b2g
  Camera           2     3        217         134        u0_a12730 12730 5886  /system/b2g/b2g
  (Preallocated a  2     6        334         300        u0_a13121 13121 5886  /system/b2g/b2g

Communications is OOM_ADJ=12, ripe for the killing.  This is about the worst possible thing we could do in this situation.  Instead first kill "(Preallocated a", then "Camera" if needed.

Comment 27

[Gabriele Svelto [:gsvelto]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #26)
> Communications is OOM_ADJ=12, ripe for the killing.  This is about the worst
> possible thing we could do in this situation.  Instead first kill
> "(Preallocated a", then "Camera" if needed.

Ouch. This is unexpected and looks like a genuine regression. In this scenario both Camera and Communications should have OOM_ADJ set to 2. Can you check if backing out this patch solves the problem?

Comment 28

[Michael Vines [:m1] [:evilmachines]]

(In reply to Gabriele Svelto [:gsvelto] from comment #27)
>
> Ouch. This is unexpected and looks like a genuine regression. In this
> scenario both Camera and Communications should have OOM_ADJ set to 2. Can
> you check if backing out this patch solves the problem?

The OOM_ADJ values remain correct when not in the card view, I don't think this is a regression.

When in the card view, if the logical foreground app is actually comprised of more than one content processes (eg, Camera picker from Contacts app) then the app that launched the picker moves to the background but not the picker itself.  That's probably per-design of the current patch?  Ideally the "oldest" content process that forms the logical for an application remains at the foreground process priority instead the "newest" (ie, the "picker" and least valuable to the user).

Comment 29

[Gabriele Svelto [:gsvelto]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #28)
> The OOM_ADJ values remain correct when not in the card view, I don't think
> this is a regression.

OK, I misunderstood that part.

> When in the card view, if the logical foreground app is actually comprised
> of more than one content processes (eg, Camera picker from Contacts app)
> then the app that launched the picker moves to the background but not the
> picker itself.  That's probably per-design of the current patch?  Ideally
> the "oldest" content process that forms the logical for an application
> remains at the foreground process priority instead the "newest" (ie, the
> "picker" and least valuable to the user).

Yes, the patch does indeed work this way. Unfortunately that's a situation where it's hard to tell which is the most important app. In the picker case (attach an image to a message) it's the older app, in the action case (sending a message from the contact list) it's the younger one. I'll file a follow up to address this stuff in a more thorough way but I'm not sure if I can make it in the 2.2+ timeframe.

Comment 30

[Michael Vines [:m1] [:evilmachines]]

(In reply to Gabriele Svelto [:gsvelto] from comment #29)
>
> Yes, the patch does indeed work this way. Unfortunately that's a situation
> where it's hard to tell which is the most important app. In the picker case
> (attach an image to a message) it's the older app, in the action case
> (sending a message from the contact list) it's the younger one. I'll file a
> follow up to address this stuff in a more thorough way but I'm not sure if I
> can make it in the 2.2+ timeframe.

SGTM, thanks.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/33969ef439f1