Closed Bug 838615 Opened 9 years ago Closed 9 years ago

Add some missing nice values to hal's process-priority-manager values

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

(Keywords: qawanted)

Attachments

(1 file, 1 obsolete file)

Self-explanatory patch in a moment.
Assignee: nobody → justin.lebar+bug
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #710708 - Flags: review?(jones.chris.g)
Comment on attachment 710708 [details] [diff] [review]
Patch, v1

Is the rationale for this change that we were trying to look up the prefs but not finding them and choosing a default nice like 0?  If so this makes sense.

But I'm convinced we should nice up background++ processes above normal background processes; if they're behaving well in the background, they'll request CPU infrequently and for short bursts, and the kernel scheduler will be kind to them.  If they're not behaving well in the background, then I don't think we should let them degrade the experience of the foreground app more than other bg apps can.  I could be convinced otherwise, though! :)
Attachment #710708 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> But I'm convinced

*NOT* convinced.  Significant typo.
> Is the rationale for this change that we were trying to look up the prefs but not finding 
> them and choosing a default nice like 0?  If so this makes sense.

That's half the rationale.

> If they're not behaving well in the background, then I don't think we should let them 
> degrade the experience of the foreground app more than other bg apps can.

I don't want my music player to skip.  It's not at all clear to me that on our hardware audio decoding is so cheap as to make a bg audio task (which is the majority of bg++ tasks) cheap.

I guess it's a question of whether we want to assume good or bad faith about bg++ processes.  Since they can kill the battery anyway, assuming good faith makes sense to me...
Similarly, I don't want my foreground process to jank if a bg++ process doesn't behave well.

The use cases for bg++ are homescreen (100% idle unless misbehaving) and playing music.  The product targets for music assume HW decoding, and for that the music player only needs to wake up once in a while to refill one buffer and drain another.

How about we make background nice +20, background-homescreen nice +20, and background-perceivable nice +10.  We can test if that has any effect on bg music playback.
Attached patch Patch, v2Splinter Review
Do we put qawanted on this bug to ask someone to test the media player once this lands?
Attachment #710708 - Attachment is obsolete: true
Attachment #711075 - Flags: review?(jones.chris.g)
Keywords: qawanted
Yes, that's a good idea.

tchung, can we get someone to test playback of music while the Music app is in the background with
 - mp3 file (hw decoder)
 - ogg vorbis (sw decoder)

while doing a variety of intensive tasks in the foreground like loading websites, playing games, etc.

What we want to know is whether this patch causing audio jitter or skips.
Attachment #711075 - Flags: review?(jones.chris.g) → review+
Comment on attachment 711075 [details] [diff] [review]
Patch, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 821440.

User impact if declined: Process running at the wrong priority, causing jank.

Testing completed: Trivial patch, although it needs testing per earlier comments.  We're just tweaking prefs here, which is about as safe as it gets, so I'd like to land and get testing on branch.  This ensures that we get the most testers banging on this.
 
Risk to taking this patch (and alternatives if risky): Trivial patch.

String or UUID changes made by this patch: None.
Attachment #711075 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/75899c659a0b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
naoki can you test this out on nightlies, would like confirmation before we approve for uplift.
QA Contact: nhirata.bugzilla
If this testing cannot be done and these patches landed on b2g18 in the next twelve hours,  I would like to land this on b2g18 and test on that branch.

Every patch I have on m-c but not on b2g18 increases the risk that I won't be able to fix my critical blockers (bug 836199, bug 836638, and bug 837927) in a timely fashion, because it's very complex to reason about which patches are where, who is building with which sets of patches, and which patches have which effects.

Please help me keep this as simple as possible.
Comment on attachment 711075 [details] [diff] [review]
Patch, v2

We can get this tested on branches then - go ahead with uplift.
Attachment #711075 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Batch edit: Bugs fixed on b2g18 after 1/25 merge to v1.0 branch are fixed on v1.0.1 branch.
Naoki, have you had a chance to test this?  We really some manual testing here.
You need to log in before you can comment on or make changes to this bug.