Closed
Bug 1119277
Opened 10 years ago
Closed 10 years ago
Remove the process CPU priority parameter from SetProcessPriority()
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: gsvelto, Assigned: gsvelto)
References
Details
Attachments
(2 files, 2 obsolete files)
36.02 KB,
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
36.14 KB,
patch
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Attachment #8564152 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Thanks for the reviews! The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=307d6c0b15d6
Assignee | ||
Comment 5•10 years ago
|
||
There's a test that is repeatedly failing in the try run but not on my machine; I have to investigate further before landing.
Assignee | ||
Comment 6•10 years ago
|
||
I did another try run and got the same errors; unfortunately I can't seem to be able to reproduce them locally.
Comment 7•10 years ago
|
||
Could be a number of CPUs issue.
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e25e482ff4
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44e25e482ff4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 14•10 years ago
|
||
[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?
Updated•10 years ago
|
Attachment #8576905 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•