Closed Bug 802423 Opened 12 years ago Closed 11 years ago

Updater needs to set oom_adj and nice appropriately

Categories

(Toolkit :: Application Update, defect, P4)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: cjones, Assigned: dhylands)

References

Details

(Keywords: dogfood, Whiteboard: [LOE:S] [Triaged:1/17])

Attachments

(1 file)

Currently, while an update is being applied in the background, the device becomes nigh unusable.  We need to nice the updater *way* up so it doesn't affect UX.

Relatedly, it would be disastrous for the updater binary to crash while staging an update.  It uses relatively little memory, maxed out at about 10MB in a quick test I just did, so pinning it to the "system service" OOM class is totally fine.
Whiteboard: [LOE:S]
We might also want to set the I/O priority, since the stager does mostly extracting and copying:

http://linux.die.net/man/2/ioprio_set
That shouldn't affect user-perceived perf much (unless we're hitting some of the badness sync disk IO in gecko), but it's a good idea to guard against degenerate cases.
Based on recent discussions this may not make be enabled, so can't block on that.
blocking-basecamp: + → ---
Renoming. Even if gecko/gaia updates aren't enabled, the extraction process for FOTA updates shouldn't cause the system to stutter..
blocking-basecamp: --- → ?
We discussed during triage that we probably can't block on stutter during (hopefully) infrequent updates.  Please re-nom if you disagree.
blocking-basecamp: ? → -
Tentatively agreed because while the experience is extremely bad, other OS processes are getting CPU time slices fairly reliably so the user can make progress.  (Unlike bug 813765 where the system completely freezes for seconds at a time.)

Suggest soft-blocking though.  The patch for this should be about 3 lines and is safe.
blocking-basecamp: - → ?
Priority: -- → P4
Sorry, don't know how the bb- was reset.
blocking-basecamp: ? → -
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Suggest soft-blocking though.

Sorry, missed this.  Setting it now.
tracking-b2g18: --- → +
Assignee: nobody → dhylands
I'm going to assume that we only want to change the nice and not the oom_adj.

Changing oom_adj will make it more likely that the updater gets killed.
Changing nice make the updater play better with the other apps.
I think it was changing oom_adj to make the updater even less killable.

don't forget comment 1 if possible too :-)
Yes, we should set oom_adj to -16.  The b2g process runs with oom_adj 0 which the updater most likely will inherit.

Be nicer about CPU but meaner about memory.
OK - my current plan of action then is to add 4 prefs (ioprio needs both a class and a level), which nsUpdateDriver.cpp will use to set env vars which updater.cpp will detect and set.
I could also combine the 4 prefs into a single env var.
I'm going to propose the following 4 preference names:

app.update.updater.nice
app.update.updater.oom_score_adj
app.update.updater.ioprio.class
app.update.updater.ioprio.level
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
With this patch, I don't notice any UI degradation while the updater is applying in the background.
Attachment #703071 - Flags: review?(robert.bugzilla)
Whiteboard: [LOE:S] → [LOE:S] [Triaged:1/17]
Comment on attachment 703071 [details] [diff] [review]
Make the updater run with reduced priority under B2G.

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

r=me with the pref defaults issue either fixed or explained.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2417,5 @@
> +    int32_t ioprioClass;
> +    int32_t ioprioLevel;
> +    if (sscanf(prioEnv, "%d/%d/%d/%d",
> +               &prioVal, &oomScoreAdj, &ioprioClass, &ioprioLevel) == 4) {
> +      LOG(("MOZ_UPDATER_PRIO=%s", prioEnv));

Please move this to before the if block and remove the needless else block below.

::: toolkit/xre/nsUpdateDriver.cpp
@@ +99,5 @@
>  #if defined(MOZ_WIDGET_GONK)
> +#include <linux/ioprio.h>
> +#include "mozilla/Preferences.h"
> +
> +using namespace mozilla;

Hiding this using namespace statement behind an #ifdef is just asking for trouble.  Please either move it outside of the #ifdef or fully quality Preferences further down.

@@ +112,5 @@
> +
> +static const int  kAppUpdaterPrioDefault        = 19;     // -20..19 where 19 = lowest priority
> +static const int  kAppUpdaterOomScoreAdjDefault = -1000;  // -1000 = Never kill
> +static const int  kAppUpdaterIOPrioClassDefault = IOPRIO_CLASS_IDLE;
> +static const int  kAppUpdaterIOPrioLevelDefault = 0;      // Doesn't matter for CLASS IDLE

I'm not sure why you're repeating the default values here...  With these, why do we need to set the defaults in prefs.js?

@@ +599,1 @@
>                          getter_AddRefs(osApplyToDir));

Nit: indentation.

@@ +836,5 @@
>    }
> +#if defined(MOZ_WIDGET_GONK)
> +  // We want the updater to be CPU friendly and not subject to being killed by
> +  // the low memory killer, so we pass in some preferences to allow it to
> +  // adjust it's priority.

Nit: its.  :-)
Attachment #703071 - Flags: review?(robert.bugzilla) → review+
(In reply to :Ehsan Akhgari from comment #18)
> Comment on attachment 703071 [details] [diff] [review]
> Make the updater run with reduced priority under B2G.
> 
> Review of attachment 703071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the pref defaults issue either fixed or explained.
> 
> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +2417,5 @@
> > +    int32_t ioprioClass;
> > +    int32_t ioprioLevel;
> > +    if (sscanf(prioEnv, "%d/%d/%d/%d",
> > +               &prioVal, &oomScoreAdj, &ioprioClass, &ioprioLevel) == 4) {
> > +      LOG(("MOZ_UPDATER_PRIO=%s", prioEnv));
> 
> Please move this to before the if block and remove the needless else block
> below.

My intention was to have the log if the sscanf passed. The log in the else was added for debugging and I forgot to remove it.

> ::: toolkit/xre/nsUpdateDriver.cpp
> @@ +99,5 @@
> >  #if defined(MOZ_WIDGET_GONK)
> > +#include <linux/ioprio.h>
> > +#include "mozilla/Preferences.h"
> > +
> > +using namespace mozilla;
> 
> Hiding this using namespace statement behind an #ifdef is just asking for
> trouble.  Please either move it outside of the #ifdef or fully quality
> Preferences further down.

I'll pull out the #include and using from the #ifdef

> @@ +112,5 @@
> > +
> > +static const int  kAppUpdaterPrioDefault        = 19;     // -20..19 where 19 = lowest priority
> > +static const int  kAppUpdaterOomScoreAdjDefault = -1000;  // -1000 = Never kill
> > +static const int  kAppUpdaterIOPrioClassDefault = IOPRIO_CLASS_IDLE;
> > +static const int  kAppUpdaterIOPrioLevelDefault = 0;      // Doesn't matter for CLASS IDLE
> 
> I'm not sure why you're repeating the default values here...  With these,
> why do we need to set the defaults in prefs.js?

I'll remove the defaults from b2g.js. I added defaults here because I didn't want this code to rely on the prefs being present.

Thanks for the review.
Blocks: 833063
https://hg.mozilla.org/mozilla-central/rev/a3313ce63be0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
And a follow-up bustage fix since DebugOnly.h doesn't exist on the b2g18 branch.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a4087b8286e
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: