Changing OOM range to 0, 1000 and tweaking OOM algorithm.

Changing OOM range to 0, 1000 and tweaking OOM algorithm.

With this change, we now use the newer oom_score_adj file (with
fallback to oom_adj when on a system that doesn't support it) so that
we can take advantage of a finer range ([0, 1000] instead of [0, 15]).

Also tweaked the OOM priority manager to prioritize things in a
slightly different order, preferring (even more) not to kill tabs that
the user has currently selected.

BUG=chromium-os:18421, chromium:65009
TEST=Ran on device, observed OOM adj values, forced OOM conditions to
     watch kills.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97724

[stevenjb, 2011-08-18 00:20:06]

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc
File base/process_util_linux.cc (right):

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:737: if (score < 0 || score > 1000)
bit: kMaxOOMScore instead of 1000

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:748: return (static_cast<int>(score_str.length()) ==
nit: size_t bytes = score_str.length() would make this easier to read (|bytes|
can also be used in WriteFile call).

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:757: std::string score_str = base::IntToString(score
* 15 / 1000);
nit: const for old score range also

http://codereview.chromium.org/7671033/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7671033/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:185: const int kMiscScore = (kPluginScore +
kRendererScore) / 2;
While this is logical, it might be more readable if we just list the value
explicitly.

http://codereview.chromium.org/7671033/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:193: score = kPluginScore + 100;
nit: const for 100

http://codereview.chromium.org/7671033/diff/1/chrome/browser/oom_priority_man...
File chrome/browser/oom_priority_manager.cc (right):

http://codereview.chromium.org/7671033/diff/1/chrome/browser/oom_priority_man...
chrome/browser/oom_priority_manager.cc:168: const int kMaxPriority = 1000;
It would be good to avoid having these constants defined in two places. Can we
have one class that sets these values and provides accessors?

http://codereview.chromium.org/7671033/diff/1/content/browser/zygote_host_lin...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/7671033/diff/1/content/browser/zygote_host_lin...
content/browser/zygote_host_linux.cc:278: const int kRendererScore = 300;
One class to rule them all?

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
sandbox/linux/suid/process_util_linux.c:23: if (score < 0 || score > 1000)
And in the darkness bind them?

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
sandbox/linux/suid/process_util_linux.c:61: char buf[5];  // 0 <= |score| <=
1000;
Maybe size this for any 32 bit range?

[Greg Spencer (Chromium), 2011-08-18 23:10:50]

PTAL, thanks!

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc
File base/process_util_linux.cc (right):

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:737: if (score < 0 || score > 1000)
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> bit: kMaxOOMScore instead of 1000

Done.

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:748: return (static_cast<int>(score_str.length()) ==
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> nit: size_t bytes = score_str.length() would make this easier to read (|bytes|
> can also be used in WriteFile call).

Can't be a size_t, it has to be an int, but I did split it out.

http://codereview.chromium.org/7671033/diff/1/base/process_util_linux.cc#newc...
base/process_util_linux.cc:757: std::string score_str = base::IntToString(score
* 15 / 1000);
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> nit: const for old score range also

Done.

http://codereview.chromium.org/7671033/diff/1/chrome/browser/oom_priority_man...
File chrome/browser/oom_priority_manager.cc (right):

http://codereview.chromium.org/7671033/diff/1/chrome/browser/oom_priority_man...
chrome/browser/oom_priority_manager.cc:168: const int kMaxPriority = 1000;
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> It would be good to avoid having these constants defined in two places. Can we
> have one class that sets these values and provides accessors?

OK, I added two constants to chrome/common/chrome_constants.h so that it can be
accessed from both places without having to include across their boundaries.

http://codereview.chromium.org/7671033/diff/1/content/browser/zygote_host_lin...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/7671033/diff/1/content/browser/zygote_host_lin...
content/browser/zygote_host_linux.cc:278: const int kRendererScore = 300;
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> One class to rule them all?

OK, moved this to common/chrome_constants.h

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
sandbox/linux/suid/process_util_linux.c:23: if (score < 0 || score > 1000)
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> And in the darkness bind them?

Also moved into a constant.

http://codereview.chromium.org/7671033/diff/1/sandbox/linux/suid/process_util...
sandbox/linux/suid/process_util_linux.c:61: char buf[5];  // 0 <= |score| <=
1000;
On 2011/08/18 00:20:07, Steven Bennetts wrote:
> Maybe size this for any 32 bit range?

Done.

[Greg Spencer (Chromium), 2011-08-18 23:18:38]

Adding Darin and Brett for OWNERS review in both content/browser and base, since
they both are OWNERS in both places, and so I only need one of them to review.

[stevenjb, 2011-08-18 23:31:05]

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:182: const int kPluginScore = 100;
Really, I'm not trying to be a pain :), but I think this should either be
defined with kLowestRendererOomScore, or relative to it (i.e. kRendererScore -
200, kRendererScore - 100 for kMiscScore)

http://codereview.chromium.org/7671033/diff/22/content/browser/zygote_host_li...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/7671033/diff/22/content/browser/zygote_host_li...
content/browser/zygote_host_linux.cc:278: const int kRendererScore = 300;
Any reason for a value other than 0, or maybe 1? Magic numbers kind of suck.

http://codereview.chromium.org/7671033/diff/22/sandbox/linux/suid/process_uti...
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/7671033/diff/22/sandbox/linux/suid/process_uti...
sandbox/linux/suid/process_util_linux.c:24: static const int kMaxOomScore =
1000;
Can't we get this from base/process_util.h?

[Greg Spencer (Chromium), 2011-08-18 23:59:32]

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:182: const int kPluginScore = 100;
On 2011/08/18 23:31:05, Steven Bennetts wrote:
> Really, I'm not trying to be a pain :), but I think this should either be
> defined with kLowestRendererOomScore, or relative to it (i.e. kRendererScore -
> 200, kRendererScore - 100 for kMiscScore)

No worries: good comments are never a pain.

OK, I can define it relative, but should be less than the lowest renderer:
that's kind of the point.

(So, you're OK with a "- 100", but not with an average? :) )

http://codereview.chromium.org/7671033/diff/22/content/browser/zygote_host_li...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/7671033/diff/22/content/browser/zygote_host_li...
content/browser/zygote_host_linux.cc:278: const int kRendererScore = 300;
On 2011/08/18 23:31:05, Steven Bennetts wrote:
> Any reason for a value other than 0, or maybe 1? Magic numbers kind of suck.

Yes, this should be chrome::kLowestRendererOomScore, but content code can't
include chrome/common/chrome_constants.h  I'll add more comments though.

http://codereview.chromium.org/7671033/diff/22/sandbox/linux/suid/process_uti...
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/7671033/diff/22/sandbox/linux/suid/process_uti...
sandbox/linux/suid/process_util_linux.c:24: static const int kMaxOomScore =
1000;
On 2011/08/18 23:31:05, Steven Bennetts wrote:
> Can't we get this from base/process_util.h?

Sadly, no.  This executable doesn't link with base, or we wouldn't need a
separate implementation of AdjustOOMScore here.

[stevenjb, 2011-08-19 17:25:30]

LGTM

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/7671033/diff/22/chrome/app/chrome_main.cc#newc...
chrome/app/chrome_main.cc:182: const int kPluginScore = 100;
On 2011/08/18 23:59:32, Greg Spencer (Chromium) wrote:
> On 2011/08/18 23:31:05, Steven Bennetts wrote:
> > Really, I'm not trying to be a pain :), but I think this should either be
> > defined with kLowestRendererOomScore, or relative to it (i.e. kRendererScore
-
> > 200, kRendererScore - 100 for kMiscScore)
> 
> No worries: good comments are never a pain.
> 
> OK, I can define it relative, but should be less than the lowest renderer:
> that's kind of the point.
> 
> (So, you're OK with a "- 100", but not with an average? :) )

The question is, an average of what? With absolute scores there is an implicit
knowledge that chrome::kLowestRendererOomScore == 300. At least with deltas, as
long as chrome::kLowestRendererOomScore > 200 the logic works. The DCHECKs are a
good addition and avoid the need to define these in the header with
kLowestRendererOomScore.

[Greg Spencer (Chromium), 2011-08-19 18:11:06]

On 2011/08/19 17:25:30, Steven Bennetts wrote:
> > (So, you're OK with a "- 100", but not with an average? :) )
> 
> The question is, an average of what? With absolute scores there is an implicit
> knowledge that chrome::kLowestRendererOomScore == 300. At least with deltas,
as
> long as chrome::kLowestRendererOomScore > 200 the logic works. The DCHECKs are
a
> good addition and avoid the need to define these in the header with
> kLowestRendererOomScore.

It's OK, I was just yanking your chain: this is more clear.

[Greg Spencer (Chromium), 2011-08-19 22:11:35]

Darin or Brett, just need an OWNERS review now.

[brettw, 2011-08-21 16:12:22]

LGTM rubberstamp for my parts.

Please only list one owner reviewer to avoid spamming people.