Gabriele Svelto [:gsvelto] Assignee
	Description • 10 years ago

+++ 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.

	Gabriele Svelto [:gsvelto] 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.

	Gabriele Svelto [:gsvelto] 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).

	Dave Hylands [: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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 4 • 10 years ago

Thanks for the reviews! The try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=307d6c0b15d6

	Gabriele Svelto [:gsvelto] 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.

	Gabriele Svelto [:gsvelto] 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.

	Dave Hylands [:dhylands]
	Comment 7 • 10 years ago

Could be a number of CPUs issue.

	Gabriele Svelto [:gsvelto] 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.

	Gabriele Svelto [:gsvelto] 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.

	Gabriele Svelto [:gsvelto] 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.

	Gabriele Svelto [:gsvelto] 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/44e25e482ff4

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/44e25e482ff4

	Gabriele Svelto [:gsvelto] 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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/62ca1acefbcf