957880 - Expose memory pressure events to gaia's system app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Gecko notifies of memory pressure situation with an observer notification. We need to turn that into an event for gaia's system app and let it clean up whatever we could.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: mem-pressure-gaia.patch

The gecko side.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: mem-pressure-kbd.patch

The goal of this patch is to unload keyboards when they are not in use and we get a memory pressure event. Memory reports show that we use ~400kb in the keyboard frame:

─0.43 MB (01.07%) -- window(app://keyboard.gaiamobile.org/index.html#en)
│   │     ├──0.34 MB (00.83%) ++ js-compartment(app://keyboard.gaiamobile.org/index.html#en)
│   │     ├──0.08 MB (00.19%) ── style-sheets
│   │     ├──0.02 MB (00.04%) ++ dom
│   │     └──0.00 MB (00.00%) ── property-tables


The patch seems to work ok but I can't run new memory reports for now for some strange reason on my device.

Tim, let me know if what I did makes any sense!

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8357496 [details] [diff] [review]
mem-pressure-kbd.patch

This is an exciting feature, and I am very excited to see it exposed to all apps. However, only exposing that to System app doesn't make sense to me. Every (oop) app frame is subject to OOM; if there is memory pressure, Gecko could simply kill the background app with existing mechanism.

Keyboard apps are OOP'd after bug 942790 for v1.4. Unless you are looking at 1.3 (tarako?) I don't think we should land this (or, for some reason, we decided we should not OOP the built-in keyboard app later).

If you expose the event to keyboard apps (or other apps), keyboard apps could certainly do their own efforts (e.g. ditch the dictionary worker) when receiving the event. That would be a very useful feature for the web.

Comment 4

[[:fabrice] Fabrice Desré]

Tim, you're right that every process can receive a memory pressure notification (and we use it on the platform side in content processes). But exposing that to the web at large is quite controversial so I'd like to take baby steps here.

The goal is not to kill OOP apps (we rely on the LMK for that), but just to let the system app cleanup what it can clean up. Indeed the main motivation is tarako, where we won't run the keyboards OOP. If you wish, I can update the patch to only remove keyboards when oop keyboards are disabled.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: mem-pressure-kbd.patch

Tim, I updated the patch to only wipe out frames when we are not using OOP keyboards.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8360078 [details] [diff] [review]
mem-pressure-kbd.patch

Yeah that makes more sense, thanks!

Looping Gary for feedback.

>diff --git a/apps/system/js/keyboard_manager.js b/apps/system/js/keyboard_manager.js
>index 30ffeec..00fe307 100644
>--- a/apps/system/js/keyboard_manager.js
>+++ b/apps/system/js/keyboard_manager.js
>@@ -136,6 +136,7 @@ var KeyboardManager = {
>     window.addEventListener('attentionscreenshow', this);
>     window.addEventListener('mozbrowsererror', this);
>     window.addEventListener('applicationsetupdialogshow', this);
>+    window.addEventListener('mozmemorypressure', this);
> 
>     // To handle keyboard layout switching
>     window.addEventListener('mozChromeEvent', function(evt) {
>@@ -443,6 +444,16 @@ var KeyboardManager = {
>       case 'mozbrowsererror': // OOM
>         this.removeKeyboard(evt.target.dataset.frameManifestURL, true);
>         break;
>+      case 'mozmemorypressure':
>+        // Memory pressure event. If a keyboard is loaded but not opened,
>+        // get rid of it.
>+        // We only do that when we don't run keyboards OOP.
>+        this._debug('mozmemorypressure event');
>+        if (this.isOutOfProcessEnabled && this.keyboardHeight == 0) {

if (!this.isOutOfProcessEnabled
    ^

>+          Object.keys(this.runningLayouts).forEach(this.removeKeyboard, this);
>+          this.runningLayouts = {};
>+        }
>+        break;
>     }
>   },
>

Comment 7

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

Comment on attachment 8360078 [details] [diff] [review]
mem-pressure-kbd.patch

looks good for me.

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/3f48bdbc6808

Comment 9

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/34b01ea815956879ef0471393d0995dcadf3c30b

Comment 10

[Salvador de la Puente González [:salva]]

Thank you Fabrice. Is this available for any app or just for the system one?

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f48bdbc6808

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Salvador de la Puente González [:salva] from comment #10)
> Thank you Fabrice. Is this available for any app or just for the system one?

Only for the system app. No plans to expose it to others for now.

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ for tarako

Comment 14

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/574846be6730

Comment 15

[[:fabrice] Fabrice Desré]

Tim, can you deal with the gaia uplift?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.3t, Gaia: 2b2e67b5ddd112d3774ff25cc09f63b1ff4fdd8d