Closed Bug 1119277 Opened 5 years ago Closed 5 years ago

Remove the process CPU priority parameter from SetProcessPriority()

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1081871 +++

TL;DR After bug 1081871 lands we can remove the third parameter of the SetProcessPriority() call, |hal::ProcessCPUPriority aCPUPriority| and all the machinery that deals with it in the process priority manager.

Long story: originally SetProcessPriority() was meant to have a single priority value which we would use both to set the CPU priority as well as the LMK parameters. After having implemented CPU priorities via nice values we run into a very annoying corner case: we didn't have a way to drastically raise the priority of a single critical application above all other ones. One solution to this problem would have been to boost the application nice values into negative territory but this had the side-effect of starving processes we weren't in control of and which would run by default with a nice value of 0.

This case was particularly important for the dialer which could take multiple seconds to startup when the phone was under heavy load and memory pressure at the same time.

To work around this issue we split an app priority into two aspects: a generic priority which would affect both the oom_score_adj value and the nice value and a CPU-specific one which would only affect nice values and would be limited to two options (high & low).

When the appropriate conditions were met (a critical app holding the high-priority wakelock) we would downgrade all apps' CPU priority and adjust their nice values accordingly. This was implemented in bug 870181 and added a significant amount of complexity to the process priority manager.

With cgroup support we can now easily and quickly raise the priority of an application above all other ones and without starving other system processes; this should allow us to remove all the complex machinery we had introduced to support split generic/cpu priorities.
First spin at this. I've removed all the bits associated with the CPU priority parameter but left intact the machinery used to track high prioritiy tasks since we still need it.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
This is the complete patch, here's a description of what it does:

- Removes all the CPU priority management bits that were introduced in bug 870181, this greatly simplifies the process priority manager.

- Removes the tests associated with this functionality.

- Removes all the bits in the HAL component that were required for implementing it.

- Leaves the machinery used to track the high priority process intact since we still use it for inhibiting memory-pressure events in the presence of a high-priority process. This might be simplified in the future to use a single counter instead of the array of PIDs we currently use.

I'm asking two reviews because I need one for the HAL bits (Dave) and one for the DOM part (Kyle).
Attachment #8560445 - Attachment is obsolete: true
Attachment #8564152 - Flags: review?(khuey)
Attachment #8564152 - Flags: review?(dhylands)
Comment on attachment 8564152 [details] [diff] [review]
[PATCH] Remove the process CPU priority parameter and simplify all the associated code

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

Looks good to me. Great work.
Attachment #8564152 - Flags: review?(dhylands) → review+
Thanks for the reviews! The try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=307d6c0b15d6
There's a test that is repeatedly failing in the try run but not on my machine; I have to investigate further before landing.
I did another try run and got the same errors; unfortunately I can't seem to be able to reproduce them locally.
Could be a number of CPUs issue.
(In reply to Dave Hylands [:dhylands] from comment #7)
> Could be a number of CPUs issue.

Actually, it turns out it was a mistake on my part, I forgot to adjust one of the tests and the associated helper. This does fail on my machine too, I must have misinterpreted the mochitest results. Updated patch coming.
Here's the updated patch, there's only changes to the mochitests. The try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69c48d326002

I'll be waiting for it to turn green before asking for review again. Sorry for the fuss.
Attachment #8564152 - Attachment is obsolete: true
Comment on attachment 8569826 [details] [diff] [review]
[PATCH v2] Remove the process CPU priority parameter and simplify all the associated code

Carrying over the review flag as discussed on IRC since the second version of the patch only has test changes. This is r=dhylands,khuey.
Attachment #8569826 - Flags: review+
The try run in comment 9 is finally green though I had to re-trigger mochitest-3 on Windows 7 debug a couple of times to make it turn green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44e25e482ff4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a requirement to land the patches for bug 892371 and bug 852925 without drastically altering both of them; both of them are 2.2+
User impact if declined: None, this patch only removes unused functionality
Testing completed: Manually tested on a device and run the relevant mochitests locally, a try run is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8a93206b51
Risk to taking this patch (and alternatives if risky): Practically none, this removes unused functionality that's been disabled for almost a couple of months now
String or UUID changes made by this patch: None
Attachment #8576905 - Flags: approval-mozilla-b2g37?
Attachment #8576905 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Blocks: 892371
Blocks: 1144132
You need to log in before you can comment on or make changes to this bug.