Expose memory pressure events to gaia's system app

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

unspecified
All
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8357490 [details] [diff] [review]
mem-pressure-gaia.patch

The gecko side.
Assignee: nobody → fabrice
Attachment #8357490 - Flags: review?(anygregor)
(Assignee)

Comment 2

5 years ago
Created attachment 8357496 [details] [diff] [review]
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!
Attachment #8357496 - Flags: feedback?(timdream)
Attachment #8357490 - Flags: review?(anygregor) → review+
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.
Attachment #8357496 - Flags: feedback?(timdream)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 8360078 [details] [diff] [review]
mem-pressure-kbd.patch

Tim, I updated the patch to only wipe out frames when we are not using OOP keyboards.
Attachment #8357496 - Attachment is obsolete: true
Attachment #8360078 - Flags: review?(timdream)
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;
>     }
>   },
>
Attachment #8360078 - Flags: review?(timdream)
Attachment #8360078 - Flags: review+
Attachment #8360078 - Flags: feedback?(gchen)
Comment on attachment 8360078 [details] [diff] [review]
mem-pressure-kbd.patch

looks good for me.
Attachment #8360078 - Flags: feedback?(gchen) → feedback+
Thank you Fabrice. Is this available for any app or just for the system one?
https://hg.mozilla.org/mozilla-central/rev/3f48bdbc6808
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

5 years ago
(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.
triage: 1.3T+ for tarako
blocking-b2g: --- → 1.3T+
Whiteboard: [tarako][POVB]
(Assignee)

Comment 15

5 years ago
Tim, can you deal with the gaia uplift?
Flags: needinfo?(timdream)
v1.3t, Gaia: 2b2e67b5ddd112d3774ff25cc09f63b1ff4fdd8d
Flags: needinfo?(timdream)
You need to log in before you can comment on or make changes to this bug.