When OOM the keyboard App should have lower priority than the foreground App

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

unspecified
1.2 C2(Oct11)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: QARegressExclude)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
I think the role=keyboard app should have lower priority than the foreground app that triggers it but higher priority than the rest of other apps. So we try to keep the keyboard open as hard as possible but don't let it kill the foreground app.
(Assignee)

Comment 1

5 years ago
Controlled by dom/ipc/ProcessPriorityManager.cpp
(Assignee)

Comment 2

5 years ago
Hmm.. should we reuse the BACKGROUND_PERCEIVABLE class or create a new BACKGROUND_KEYBOARD (or FOREGROUND?)

Probably FOREGROUND_KEYBOARD when the keyboard is activated and BACKGROUND when it hides.

Comment 3

5 years ago
1. We can only WRITE 6 pairs of KillUnderMB/OomScoreAdjust to sys/module/lowmemorykiller/parameters/. We are currently running out of these pairs.

Since OOM_ADJUST_MAX = 15 (or OOM_SCORE_ADJ_MAX = 1000 in later kernel),
We can enlarge these pairs' OomScoreAdjust. But in my experience, I suggest we keep hal.processPriorityManager.gonk.BACKGROUND.OomScoreAdjust no larger than 400+267. We still need spaces for background LRU, which are bug 822325 and bug 913893. I will open another bug for enlarging OomScoreAdjusts later.

2. We can still define more LMK levels, write to each process but not write to kernel config by GonkHal. 

Take Unagi as an example, we now write 0,1,2,3,4,6 to /sys/module/lowmemorykiller/parameters/adj. We cannot write 0,1,2,3,4,5,6 to it, but we can still set 5 to a process' /proc/[pid]/oom_adj.

If we enlarge the series 0,1,2,3,4,6 to 0,1,2,6,8,10 or something else, we can have more benefit.

Updated

5 years ago
Depends on: 914728

Comment 4

5 years ago
Created attachment 802824 [details] [diff] [review]
an example to add new ProcessPriority

Hi Kan-ru,

If you need to add new ProcessPriority, the attachment is for what we should do now. I think you may need to wait after bug 914728 land.
(Assignee)

Comment 5

5 years ago
(In reply to Alan Huang [:ahuang] from comment #4)
> Created attachment 802824 [details] [diff] [review]
> an example to add new ProcessPriority
> 
> Hi Kan-ru,
> 
> If you need to add new ProcessPriority, the attachment is for what we should
> do now. I think you may need to wait after bug 914728 land.

Thanks, Alan. This example is really helpful.
(Assignee)

Updated

5 years ago
Depends on: 912340
(Assignee)

Comment 6

5 years ago
Created attachment 802969 [details] [diff] [review]
WIP check role=keyboard for initial priority
(Assignee)

Updated

5 years ago
Assignee: nobody → kchen
(Assignee)

Comment 7

5 years ago
Created attachment 809074 [details] [diff] [review]
Assign higher priority for mozapptype=inputmethod processes

keyboard_manager.js will have to set mozapptype=keyboard for the keyboard iframe.
Attachment #802824 - Attachment is obsolete: true
Attachment #802969 - Attachment is obsolete: true
Attachment #809074 - Flags: review?(fabrice)
Attachment #809074 - Flags: feedback?(ahuang)
(Assignee)

Updated

5 years ago
No longer depends on: 912340
Comment on attachment 809074 [details] [diff] [review]
Assign higher priority for mozapptype=inputmethod processes

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

::: b2g/app/b2g.js
@@ +624,5 @@
>  pref("hal.processPriorityManager.gonk.FOREGROUND.KillUnderMB", 6);
>  pref("hal.processPriorityManager.gonk.FOREGROUND.Nice", 1);
>  
> +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);

do we have a default value for the KillUnderMB pref?
Attachment #809074 - Flags: review?(fabrice) → review+

Updated

5 years ago
Attachment #809074 - Flags: feedback?(ahuang) → feedback+

Comment 9

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #8)
> > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);
> 
> do we have a default value for the KillUnderMB pref?
No, we don't and we cannot assign any more KillUnderMBs. The android kernel can only accept 6 (OomScoreAdjust, KillUnderMB) pairs. 

But it is okay, kernel will still kill processes with larger OomScoreAdjust first, even its OomScoreAdjust don't have corresponding KillUnderMB. The (OomScoreAdjust, KillUnderMB) pairs are raising critiria for LMK.
(Assignee)

Comment 10

5 years ago
(In reply to Alan Huang [:ahuang] from comment #9)
> (In reply to Fabrice Desré [:fabrice] from comment #8)
> > > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.OomScoreAdjust", 200);
> > > +pref("hal.processPriorityManager.gonk.FOREGROUND_KEYBOARD.Nice", 1);
> > 
> > do we have a default value for the KillUnderMB pref?
> No, we don't and we cannot assign any more KillUnderMBs. The android kernel
> can only accept 6 (OomScoreAdjust, KillUnderMB) pairs. 
> 
> But it is okay, kernel will still kill processes with larger OomScoreAdjust
> first, even its OomScoreAdjust don't have corresponding KillUnderMB. The
> (OomScoreAdjust, KillUnderMB) pairs are raising critiria for LMK.

Thanks for explanation. I'll add them to the comments in b2g.js
https://hg.mozilla.org/mozilla-central/rev/b8f0eace02bc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 13

5 years ago
This is the blocker for OOP which is needed for new keyboard framework. So nominate it to koi+
blocking-b2g: --- → koi+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/00c23047048a
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
Keywords: checkin-needed
Target Milestone: --- → 1.2 QE1(Oct11)

Updated

5 years ago
Whiteboard: QARegressExclude
(Assignee)

Updated

5 years ago
Blocks: 928270
You need to log in before you can comment on or make changes to this bug.