Gabriele Svelto [:gsvelto] Assignee
	Description • 9 years ago

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

Currently whenever an application is sent to the background we send a memory-pressure message to it so that it will try to save as much memory as possible (flushing caches, running the GC/CC) etc... This has the drawback that when the app comes back again it might respond slowly (see bug 1224268 for a practical effect of this).

The reason why we've implemented this policy was lack of memory on early devices. We don't have that problem anymore even on 256 MiB devices (thanks to Nuwa) so it's time to revisit it.

A good approach would be to let apps consume as much memory as they want - even when they're in the background - as long as there's memory available. As we start running out of memory then we should start minimizing applications so that the situation improves.

The LMK memory trigger can be used to detect low-memory scenarios but it's currently fixed at a value between the LMK levels for background and foreground applications so it won't send memory-pressure messages until we've already killed all background apps. We want to retain this behavior while at the same time use the trigger to send memory-pressure messages before we start killing apps. This would require to adjust the trigger dynamically in response to changes in the environment.

To sum it up what we need to do in this bug is:

- Do not minimize applications sent into the background anymore 

- Add logic to the memory pressure detector so that we'll be able to set the low-memory trigger to a higher value than the background LMK level and use it to minimize app

- Add logic that will switch the trigger to the current level when under memory pressure, and then back to the higher level when memory pressure is over. We'll probably need to add an exponential back-off mechanism to prevent the trigger from firing too often in contrived scenarios.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 2 • 9 years ago

This patch removes the code that we used to send memory-pressure events to all background processes and replaces it with a dynamic low-memory trigger that will minimize memory usage only when it's actually needed.

The trigger is implemented by re-purposing the memory pressure watcher. Previously the watcher was setup so that all background apps would be killed and only then it would start sending memory pressure to try and keep alive the foreground apps. This was implemented by setting the LMK low-memory trigger to a fixed level.

In this patch we define two levels instead, a soft memory-pressure level that sits above the LMK threshold for killing background apps and the regular hard memory-pressure level. The rationale is that we'll start sending memory-pressure events when we begin to run out of memory, then we'll switch to the old behavior.

Once memory pressure is over we'll start re-adjusting the trigger to the soft level by using a timeout and checking if enough free memory is available once it expires. The timeout has an exponential back-off mechanism to avoid triggering too often and draining the battery.

This patch has multiple benefits:

- It fixes bug 1224268 under most common scenarios

- It saves power as we're not needlessly minimizing memory usage when there's still plenty of memory available

- It makes a few interactions a lot smoother: notably going back to the homescreen, coming back from an activity and using edge-swipe navigation. Also invoking the cards view is also smoother as the current app doesn't flicker when taking the screenshot.

A final note, I added an helper function called WriteSysFile() to replace a private function I used only in the HAL implementation. It mirrors ReadSysFile() and is implemented alongside it.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 9 years ago

I've split the patch in three parts for easier review and to avoid lumping changes that are not strictly dependent on each other in the same patch. I'll upload them shortly.

	Dave Hylands [:dhylands]
	Comment 7 • 9 years ago

Comment on attachment 8702540 [details] [diff] [review]
[PATCH 3/3] Introduce a dynamic trigger to send memory pressure events before background applications are reaped by the LMK

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

::: widget/gonk/GonkMemoryPressureMonitoring.cpp
@@ +246,5 @@
> +      "hal.processPriorityManager.gonk.notifySoftLowMemUnderKB",
> +      /* default */ 43008);
> +    Preferences::AddUintVarCache(&mHardLowMemTriggerKB,
> +      "hal.processPriorityManager.gonk.notifyHardLowMemUnderKB",
> +      /* default */ 14336);

Shouldn't we add these preferences to b2g.js to make them a bit more visible?

@@ +306,5 @@
> +
> +    struct sysinfo info;
> +
> +    int rv = sysinfo(&info);
> +

nit: the above 2 whitespace lines feel out of place

@@ +316,5 @@
> +
> +    if (freeMemory > mSoftLowMemTriggerKB) {
> +      SetLowMemTrigger(mSoftLowMemTriggerKB);
> +      return -1; // Trigger adjusted, wait indefinitely.
> +    } else {

nit: remove else, unindent next line

	Gabriele Svelto [:gsvelto] Assignee
	Comment 8 • 9 years ago

Thanks for the reviews Dave!

(In reply to Dave Hylands [:dhylands] from comment #7)
> ::: widget/gonk/GonkMemoryPressureMonitoring.cpp
> @@ +246,5 @@
> > +      "hal.processPriorityManager.gonk.notifySoftLowMemUnderKB",
> > +      /* default */ 43008);
> > +    Preferences::AddUintVarCache(&mHardLowMemTriggerKB,
> > +      "hal.processPriorityManager.gonk.notifyHardLowMemUnderKB",
> > +      /* default */ 14336);
> 
> Shouldn't we add these preferences to b2g.js to make them a bit more visible?

Yes, I forgot to add them. I'll fix this and the nits and post an updated patch.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 9 • 9 years ago

Review comments addressed, I've also renamed the prefs to reflect the fact that they're not used in the process priority manager anymore and capped the exponential back-off timer to a maximum of 24h (I realized late I forgot about that in the original patch). Carrying over the r+ flag.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 9 years ago

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a59a42050ac

	Gabriele Svelto [:gsvelto] Assignee
	Comment 11 • 9 years ago

The try run is green except for a known intermittent, I've also tested this on my foxfooding phone for the past couple of days and it's working well so time to land it.

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/18a588877774
https://hg.mozilla.org/integration/b2g-inbound/rev/74ad280efe25
https://hg.mozilla.org/integration/b2g-inbound/rev/f617d69d602b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/18a588877774
https://hg.mozilla.org/mozilla-central/rev/74ad280efe25
https://hg.mozilla.org/mozilla-central/rev/f617d69d602b