852925 - Follow-up Bug 849186 OOM_ADJ should not change for the topmost foreground task if the screen is switched off

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

[Jose Manuel Cantera]

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

Comment 1

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

(moving to leo? because while the potential for this bug to produce really terrible UX is high, I'm not sure we've heard many reports of the foreground app mysteriously closing when the display turns off beyond bug 849186.  suggest we consider an uplift to v1.0.1 if a patch becomes available but not block until we see another manifestation of this bug)

Comment 2

[Daniel Coloma:dcoloma]

Not sure if this is an scenario we should be targeting to fix with this bug:

I start importing contacts from Facebook (have around 500 friends), during the process I am tired to see the progress screen and go to browser, and then to check my e-mail. When I go back to contacts, I see the contact list (not the FB import progress) and I see only some of the contacts imported without any further information.

Comment 3

[Dave Hylands [:dhylands]]

I would actually expect the results in comment 2 if memory is low. I guess the contacts app (or any app for that matter) needs to keep some state information so that it pick up where it left off if it got OOM'd in the background.

Comment 4

[Jose Manuel Cantera]

(In reply to Dave Hylands [:dhylands] from comment #3)
> I would actually expect the results in comment 2 if memory is low. I guess
> the contacts app (or any app for that matter) needs to keep some state
> information so that it pick up where it left off if it got OOM'd in the
> background.

yes, the user can continue later importing more contacts by clicking on 'Update Contacts' later.

Comment 5

[Dave Hylands [:dhylands]]

(In reply to Jose M. Cantera from comment #4)
> (In reply to Dave Hylands [:dhylands] from comment #3)
> > I would actually expect the results in comment 2 if memory is low. I guess
> > the contacts app (or any app for that matter) needs to keep some state
> > information so that it pick up where it left off if it got OOM'd in the
> > background.
> 
> yes, the user can continue later importing more contacts by clicking on
> 'Update Contacts' later.

I meant that the contacts app should probably remember that it was importing contacts and didn't finish, so that it can resume or offer to resume the next time it launches.

Comment 6

[Jose Manuel Cantera]

if we go for that change, we will increase Contacts App, start up time. I don't believe we should follow that path.

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Jose M. Cantera from comment #6)
> if we go for that change, we will increase Contacts App, start up time. I
> don't believe we should follow that path.

Huh - I don't follow how you came to that conclusion. I'm not suggesting that it should do a sync each time it starts. I'm suggesting that it should remember that it didn't finish doing the previous one. I don't understand how that can have any significant impact on the startup time.

Comment 8

[Jose Manuel Cantera]

because at start-up we would need to retrieve some persistent value from indexedDB, bla, bla.

Comment 9

[Jose Manuel Cantera]

Attachment: Pointer to GH PR

Requesting also Crdlc's review as the PR modifies image loader component.

Please note that this PR in practice disable search of FB contacts by organization . In later PRs we can address that issue or keep it as it is but as this is a blocker I would prefer not to be distracted by that minor issue and address the rest later by think it more carefully.

Comment 10

[Jose Manuel Cantera]

oops wrong bug number. sorry

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to Jose M. Cantera from comment #8)
> because at start-up we would need to retrieve some persistent value from
> indexedDB, bla, bla.

I thought IndexedDB was asynchronous.

dhylands' suggestion is exactly what we should do, IMO.  It's a premature optimization to say that we can't do it because it would be too slow.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Along the lines of mvines' comment 1 we'll track instead of blocking on this, renom if a more widespread use case manifests.

Comment 13

[alberto.pastor]

What's the status of this bug?

Comment 14

[Justin Lebar (not reading bugmail)]

This isn't a blocker, and that's probably why it's not being worked on.

Comment 15

[David Flanagan [:djf]]

Okay, so here's another use case that is affected by this bug.

If the user has an sdcard with a lot of photos on it and inserts it into their phone, then launches the gallery app, the gallery app will scan the sdcard and create thumbnails for all of the photos it finds.

This takes a lot of memory and, if there are a lot of photos, it takes a long time, too.  If the screen blanks while the scan is happening, the gallery app dies with an OOM.

I noticed this while investigating bug 963917, but that bug is about something else.

I can work around the issue in  gallery by having it hold a wake lock on the screen while scanning photos, but that seems wrong.

We could fix the bug as described in the summary.

Or we could try some sort of compromise. Maybe we should treat apps that have a wake lock on the CPU specially so that if the screen blanks, they remain in the foreground and are not targeted when memory is tight.

That way, apps that really need to keep running for some reason (like scanning) have a way to keep themselves alive.  But ordinary apps don't get special status when the user let's their screen go idle.

Comment 16

[David Flanagan [:djf]]

This discusson on dev-gaia may be relevant:

On 2/5/14 8:14 AM, Fabrice Desré wrote:
> On 02/05/2014 01:00 AM, Tim Chien wrote:
>> CPU lock simply stop the phone from suspending JavaScript execution
>> cycles when the screen is asleep.
>
> That's not true. We use that to set the process priority, see
> https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#902
>
> Note that there is also the "High Priority" wake lock that could be
> useful there. You can set it with
> navigator.requestWakeLock('high-priority');


Daniel: does requesting 'high-priority' fix the contacts importing issue you describe in comment #2?

Comment 17

[Daniel Coloma:dcoloma]

(In reply to David Flanagan [:djf] from comment #16)
> This discusson on dev-gaia may be relevant:
> 
> On 2/5/14 8:14 AM, Fabrice Desré wrote:
> > On 02/05/2014 01:00 AM, Tim Chien wrote:
> >> CPU lock simply stop the phone from suspending JavaScript execution
> >> cycles when the screen is asleep.
> >
> > That's not true. We use that to set the process priority, see
> > https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#902
> >
> > Note that there is also the "High Priority" wake lock that could be
> > useful there. You can set it with
> > navigator.requestWakeLock('high-priority');
> 
> 
> Daniel: does requesting 'high-priority' fix the contacts importing issue you
> describe in comment #2?

Not sure if 'high-priority' was applied or not to contacts, anyhow, the current behaviour is quite similar:

 - Open Contacts
 - Select import from Facebook, a list of 200 contacts is shown
 - Select All and import
 - While the contacts are being imported, I open the browser go to a bookmark.
 - Go back to contacts, the application is started and only 10 contacts are available
 - However, when I go back to settings, the app shows me that only 10 of FB contacts have been imported and click on the FB toggle to continue importing the rest.

I hope this helps

Comment 18

[Gabriele Svelto [:gsvelto]]

I've been trying to fix this bug in the past as part of bug 989713 and I've got a WIP patch there that solves the root cause of this. I'll move the patch here so that I can pick it up later as soon as I have some time for it.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Prevent the foreground application from being killed when we turn off the phone

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Prevent the foreground application from being killed when we turn off the phone

Rebased patch that applies on top of master. Michael I haven't had time to re-test this but it used to work; you might want to give it a spin to see if it solves the issue of the foreground app being killed when it becomes hidden (but doesn't go into the background).

Comment 21

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

Comment on attachment 8539267 [details] [diff] [review]
[PATCH WIP] Prevent the foreground application from being killed when we turn off the phone

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

This definitely improves the basic card view use case.  

Consider this UI flow though: Contacts -> New Contact -> Select Picture -> Camera
While the Camera activity is in the foreground, both Camera and Contacts apps have OOM_ADJ=2.  With this patch, when the card view is entered the Camera remains at 2 but Contacts (Communications) is still bumped down to OOM_ADJ=10 and is likely to get killed.

Comment 22

[Mike Lee [:mlee]]

Gabriele: What's still left to be done to land this patch in 2.2?

Comment 23

[Gabriele Svelto [:gsvelto]]

There's two things to address: first of all the behavior is not completely solid as described in comment 21, second all the associated tests need to be adjusted. However those tests are currently quite complicated and part of them is going to be removed in bug 1119277. So I'd like to finish that first so that I don't do any duplicate work there.

Comment 24

[Mike Lee [:mlee]]

Thanks Gabriel.

Do you have a sense of how long each of the two parts you called out will take and a general timeframe for when you'd be able to deliver these changes?

Comment 25

[Gabriele Svelto [:gsvelto]]

Bug 1119277 is about to land and I've got a half-done patch for bug 892371 which I intend to finish by the end of this week, early next week at most. Then comes this bug so I'd say two weeks max to land everything. Does that fit with our deadlines?

The reason why I want to fix bug 892371 first is that I'm adding LRU tracking of foreground processes which will help with the work needed here.

Comment 26

[Mike Lee [:mlee]]

Thanks Gabriele.

mvines: Can you respond to Gabriele's comment 25 question re: his delivery date and CAF's schedule? 2 weeks from the date he commented will be Thursday, March 12th which is between FL and FC.

Comment 27

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

March 12th for the final fix uplifted to v2.2 sounds fine.

Comment 28

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Freeze priority changes when the screen is turned off

This patch addresses the problem in a rather simple way: by freezing all priority changes while the screen is off. This ensures that foreground processes are not demoted because their visibility changed. I feel silly of not having thought about this solution earlier :-/ This shouldn't cause regressions when launching an application with the screen off (for example in response to a message or call) because we invariably grab a wakelock to turn the screen on and in that case any pending priority changes are immediately applied.

BTW I've developed this on top of the patch for bug 892371 because they both touch the same code and we want both in 2.2 but there's no logical dependency between the two so if we want to land this one first I can rebase it on top of a clean mozilla-central tree.

Comment 29

[Gabriele Svelto [:gsvelto]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5681a61e442c

Comment 30

[Gabriele Svelto [:gsvelto]]

I had pushed the wrong queue, this is the correct try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8ecc39481a5

Comment 31

[Gabriele Svelto [:gsvelto]]

Here's a more complete try run, disregarding the MacOS 10.6 harness bustage everything is green though I had to re-trigger a couple of tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb63e8457b5

Note to the sheriffs, this needs to land after bug 892371 as the patch here applies on top of that one.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d34f33fae4e8

Comment 33

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

Looks like attachment 8574701 [details] [diff] [review] doesn't apply to v2.2 cleanly, could you please attach a v2.2 patch as well?

Comment 34

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH mozilla-b2g37_v2_2] Freeze priority changes when the screen is turned off

[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 turning the screen off even though there should be enough memory to keep it around
Testing completed: Tested on a device, a try run is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=7257705b1f95
Risk to taking this patch (and alternatives if risky): This prevents changes in the priorities of processes when the screen is off, if this is buggy or does not work as advertised after turning the screen back on some priorities might be wrong. The patch is very simple though so the risk should be very low.
String or UUID changes made by this patch: None

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d34f33fae4e8

Comment 36

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

Based on comment 20 and comment 21, I was expecting that the final patch here would also preserve the OOM_ADJ score of the (previously) foreground application when the card view is entered.  It does however fix the issue described in the bug title, so probably my bad for assuming.  I'll create a new bug for the card view issue.

Comment 37

[Gabriele Svelto [:gsvelto]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #36)
> Based on comment 20 and comment 21, I was expecting that the final patch
> here would also preserve the OOM_ADJ score of the (previously) foreground
> application when the card view is entered.  It does however fix the issue
> described in the bug title, so probably my bad for assuming.  I'll create a
> new bug for the card view issue.

Ah yes, good point. I went for the simpler solution because my original approach was trickier to get right than I had anticipated and I didn't want to delay the fix. Fixing the card view situation without doing some ugly hacks requires some cooperation from the system app which is better addressed in another bug so thanks for filing it.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/63800b4e9c4a