Closed Bug 957880 Opened 10 years ago Closed 10 years ago

Expose memory pressure events to gaia's system app


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


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

blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed


(Reporter: fabrice, Assigned: fabrice)




(2 files, 1 obsolete file)

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.
The gecko side.
Assignee: nobody → fabrice
Attachment #8357490 - Flags: review?(anygregor)
Attached patch mem-pressure-kbd.patch (obsolete) — Splinter Review
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://
│   │     ├──0.34 MB (00.83%) ++ js-compartment(app://
│   │     ├──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]

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)
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.
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]

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(, 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]

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?
Closed: 10 years ago
Resolution: --- → FIXED
(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]
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.