Nice automatic updates up to a point, then renice if necessary.

Nice automatic updates up to a point, then renice if necessary.

BUG=5488
TEST=unit tests, gmerged on device and completed an update
successfully with two 100% CPU "nice -n -20" processes
running in the background.

[petkov, 2010-08-05 19:52:19]

Take a look. The timeouts are kind of random values...

[adlr, 2010-08-05 23:09:53]

http://codereview.chromium.org/3053049/diff/2001/3001
File update_attempter.cc (right):

http://codereview.chromium.org/3053049/diff/2001/3001#newcode336
update_attempter.cc:336: if (priority_ >= utils::kProcessPriorityLow) {
2 things.

1. rather than test if we're beyond or at low, perhaps test if we're beyond
normal? for a world where there's a priority between Normal and Low, that makes
more sense.

2. this comparison feel a little weird, esp since you're not using numeric
values directly but enum-ing. What if you made an inlinable function called
something like IsLowerPriority(a, b) // return true iff a is lower priority than
b

Then have: if (IsLowerPriority(priority_, kProcessPriorityNormal)) {...}

http://codereview.chromium.org/3053049/diff/2001/3003
File utils.cc (right):

http://codereview.chromium.org/3053049/diff/2001/3003#newcode450
utils.cc:450: TEST_AND_RETURN_FALSE(setpriority(PRIO_PROCESS, 0, priority) ==
0);
not sure if it's important, but are enums always the same width as int? maybe an
explicit cast to int for arg 3 of setpriority()?

[petkov, 2010-08-05 23:52:18]

PTAL

Althought the timeout/retry thing bothers me a bit -- I'm not if it's a good
idea to push this CL until that is resolved.

http://codereview.chromium.org/3053049/diff/2001/3001
File update_attempter.cc (right):

http://codereview.chromium.org/3053049/diff/2001/3001#newcode336
update_attempter.cc:336: if (priority_ >= utils::kProcessPriorityLow) {
On 2010/08/05 23:09:53, adlr wrote:
> 2 things.
> 
> 1. rather than test if we're beyond or at low, perhaps test if we're beyond
> normal? for a world where there's a priority between Normal and Low, that
makes
> more sense.
> 
> 2. this comparison feel a little weird, esp since you're not using numeric
> values directly but enum-ing. What if you made an inlinable function called
> something like IsLowerPriority(a, b) // return true iff a is lower priority
than
> b
> 
> Then have: if (IsLowerPriority(priority_, kProcessPriorityNormal)) {...}

Good point, this was a bug.

Fixed the comparison values and added a comparison function along with unit
tests.

http://codereview.chromium.org/3053049/diff/2001/3003
File utils.cc (right):

http://codereview.chromium.org/3053049/diff/2001/3003#newcode450
utils.cc:450: TEST_AND_RETURN_FALSE(setpriority(PRIO_PROCESS, 0, priority) ==
0);
On 2010/08/05 23:09:53, adlr wrote:
> not sure if it's important, but are enums always the same width as int? maybe
an
> explicit cast to int for arg 3 of setpriority()?

Done.

[adlr, 2010-08-06 01:05:31]

LGTM

[petkov, 2010-08-09 21:29:31]

PTAL

I'm managing the priority through a GLib's timeout source now.
Also, tested successfully with the production update server.

[adlr, 2010-08-11 00:26:46]

LGTM

http://codereview.chromium.org/3053049/diff/18001/19002
File update_attempter.h (right):

http://codereview.chromium.org/3053049/diff/18001/19002#newcode123
update_attempter.h:123: // high).
can you describe what returning true/false means?

[petkov, 2010-08-11 20:31:56]

pushing...

http://codereview.chromium.org/3053049/diff/18001/19002
File update_attempter.h (right):

http://codereview.chromium.org/3053049/diff/18001/19002#newcode123
update_attempter.h:123: // high).
On 2010/08/11 00:26:46, adlr wrote:
> can you describe what returning true/false means?

Done.