1212833 - Delay the MemoryPressure when an application goes to background

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[vnicolas]

When an application is launched from the homescreen, the homescreen priority is changed to background. 
This change triggers a memory-pressure, which unfortunately may compete for resources with the app which is trying to start.

The attached patch delay the memory-pressure to 3000ms which should be enough for any apps to have visually rendered some content to the user.

Comment 1

[vnicolas]

Attachment: delay.memory.pressure.event.on.background.patch

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

This is something I wanted to try for a while but it might be less interesting than it used to be thanks to bug 1081871. Have you measured what's the impact on startup speed? The reason I'm asking is that after bug 1081871 has landed the task sent to the background can use only 5% of the total CPU usage, with the remaining 95% reserved for the foreground task so I doubt that the memory minimization can slow down the foreground task. Also moving a task to the background has a grace period IIRC, I'm not sure if it applies to app startup though (it might not, and the app might be immediately sent to the background, I have to check).

If there's a measurable speedup then I'm all for taking this, otherwise I'd prefer to not add more complexity to the process priority manager and leave the job of prioritizing CPU allocation to the cgroup machinery.

BTW there's another thing that might be interesting to do though. We currently always try to minimize the memory usage when receiving that message but it's not very useful if the app has already been minimized in the recent past so we might be burning some precious battery for little to no gain. Instead we could measure the USS of the process after a minimization and ignore all further minimizations if the USS hasn't grown by a certain amount (possibly a percentage of the app's previous USS). I've never had the time to try that but it might be worth experimenting with it.

Comment 3

[vnicolas]

(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> This is something I wanted to try for a while but it might be less
> interesting than it used to be thanks to bug 1081871. Have you measured
> what's the impact on startup speed? The reason I'm asking is that after bug
> 1081871 has landed the task sent to the background can use only 5% of the
> total CPU usage, with the remaining 95% reserved for the foreground task so
> I doubt that the memory minimization can slow down the foreground task. Also
> moving a task to the background has a grace period IIRC, I'm not sure if it
> applies to app startup though (it might not, and the app might be
> immediately sent to the background, I have to check).
> 
> If there's a measurable speedup then I'm all for taking this, otherwise I'd
> prefer to not add more complexity to the process priority manager and leave
> the job of prioritizing CPU allocation to the cgroup machinery.
> 

I ran twice the following command with the patch and without the patch:
 - raptor test coldlaunch --app clock --runs 10

Without the patch:
First run:

| Metric                           | Mean    | Median  | Min     | Max     | StdDev | p95     |
| -------------------------------- | ------- | ------- | ------- | ------- | ------ | ------- |
| coldlaunch.navigationLoaded      | 397.700 | 400.500 | 356.000 | 420.000 | 17.545 | 420.000 |
| coldlaunch.navigationInteractive | 468.700 | 472.000 | 427.000 | 489.000 | 17.720 | 489.000 |
| coldlaunch.visuallyLoaded        | 611.000 | 618.500 | 535.000 | 660.000 | 34.728 | 660.000 |
| coldlaunch.contentInteractive    | 611.300 | 618.500 | 535.000 | 661.000 | 34.949 | 661.000 |
| coldlaunch.fullyLoaded           | 611.400 | 619.000 | 535.000 | 661.000 | 34.969 | 661.000 |
| coldlaunch.uss                   | 12.380  | 12.400  | 12.300  | 12.400  | 0.040  | 12.400  |
| coldlaunch.pss                   | 17.820  | 17.800  | 17.800  | 17.900  | 0.040  | 17.900  |
| coldlaunch.rss                   | 31.800  | 31.800  | 31.800  | 31.800  | 0.000  | 31.800  |

With the patch:

First run:

| Metric                           | Mean    | Median  | Min     | Max     | StdDev | p95     |
| -------------------------------- | ------- | ------- | ------- | ------- | ------ | ------- |
| coldlaunch.navigationLoaded      | 391.900 | 395.500 | 356.000 | 432.000 | 21.934 | 432.000 |
| coldlaunch.navigationInteractive | 465.700 | 469.000 | 434.000 | 511.000 | 22.249 | 511.000 |
| coldlaunch.visuallyLoaded        | 608.600 | 603.500 | 558.000 | 676.000 | 33.341 | 676.000 |
| coldlaunch.contentInteractive    | 608.800 | 604.000 | 558.000 | 676.000 | 33.223 | 676.000 |
| coldlaunch.fullyLoaded           | 608.900 | 604.000 | 559.000 | 676.000 | 33.071 | 676.000 |
| coldlaunch.uss                   | 12.230  | 12.300  | 11.700  | 12.300  | 0.179  | 12.300  |
| coldlaunch.pss                   | 17.650  | 17.700  | 17.100  | 17.800  | 0.186  | 17.800  |
| coldlaunch.rss                   | 31.630  | 31.700  | 31.100  | 31.700  | 0.179  | 31.700  |


Which is a few ms better.

But with this order or magnitude it clearly can be noise, as raptor is pretty noisy. But this is more or less what I would expect from this patch with cgroup enabled. As memory-pressure event often takes between 60ms/100ms on the homescreen side which is likely long because of the 5% of CPU allocated to this process.

Now with third-party homescreen memory-pressure can vary and while I understand the will to not add additional complexity for very small gain, I don't think the patch is particularly complex, and having this memory-pressure event in the hot startup path makes me uncomfortable.

It makes me uncomfortable as I'm trying to flatten the homescreen profile right after an app is launched (with this bug, bug 1212784 and bug 1212321). And stop thinking about those while profiling.



> BTW there's another thing that might be interesting to do though. We
> currently always try to minimize the memory usage when receiving that
> message but it's not very useful if the app has already been minimized in
> the recent past so we might be burning some precious battery for little to
> no gain. Instead we could measure the USS of the process after a
> minimization and ignore all further minimizations if the USS hasn't grown by
> a certain amount (possibly a percentage of the app's previous USS). I've
> never had the time to try that but it might be worth experimenting with it.

One of thing I changed in this patch is also to only do a memory-pressure event if the active state has changed. Not sure how much this happens in practice as we may have various code that is already double-checking this.

For what you suggest it may be interesting for battery life I agree. But this is way out of the scope of this particular bug :)

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Consistency in the raptor tests sounds like a good goal in and by itself, besides I think that if we run out of memory during startup it's always better to kill a background app than to wait for some memory to be recovered in another way as it's faster.

Comment 5

[Carsten Book [:Tomcat]]

this patch didn't apply cleanly :

adding 1212833 to series file
renamed 1212833 -> delay.memory.pressure.event.on.background.patch
applying delay.memory.pressure.event.on.background.patch
patching file dom/ipc/ProcessPriorityManager.cpp
Hunk #4 FAILED at 1033
1 out of 5 hunks FAILED -- saving rejects to file dom/ipc/ProcessPriorityManager.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh delay.memory.pressure.event.on.background.patch

Comment 6

[vnicolas]

Attachment: Patch to check-in

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/0f15d62411bf

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f15d62411bf

Comment 9

[vnicolas]

For what it worth, raptor.mozilla.org shows something like a 50ms improvement starting from the first marker (navigationLoaded) between 7ef21759cf8a and 0c3339654f8c.

For Gecko this is the only relevant patch to improve performance that I can see. 

It seems a big win, so it could be a Gaia change too between 5ab39e (which is Gabriele patch for bug 1221060) and ccc064c. In this range I don't see anything particularly related to perf though.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Agreed, this seems like a very significant win and it's also surprisingly consistent with the time we had allocated to background apps using cgroups (max 5% of total CPU time, so 50ms out of every second). I hoped that the kernel would be smart enough to schedule the backgrounded app to a different core than that being launched but apparently that was not the case and the old app was interfering with the startup of the new one.