This adds periodic OOM score adjustment, based on the last access time

This adds periodic OOM score adjustment, based on the last access time
of the tab, whether or not it is pinned, and (of course) how much
memory it is using.

TEST=Tested on dogfood machine, both using large numbers of tabs, and small tabs that used lots of memory.
BUG=none

[Greg Spencer (Chromium), 2010-08-31 21:36:57]

http://codereview.chromium.org/3235007/diff/2001/3002
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3235007/diff/2001/3002#newcode1417
chrome/browser/browser_main.cc:1417: browser::OomPriorityManager
oom_priority_manager;
On 2010/08/31 18:00:03, agl wrote:
> See the "!!!! READ ME !!!!" comment at the top of this function. That suggests
> that you should be documenting some times and heap allocating this object.

Done.  What times need documenting?  Do I need to do a microbenchmark of the
constructor for this object?

http://codereview.chromium.org/3235007/diff/2001/3003
File chrome/browser/oom_priority_manager.cc (right):

http://codereview.chromium.org/3235007/diff/2001/3003#newcode30
chrome/browser/oom_priority_manager.cc:30: static TimeDelta kTimeBucketDuration
= TimeDelta::FromMinutes(10);
On 2010/08/31 18:00:03, agl wrote:
> These two statics will trigger the emission of constructor functions which
hurt
> startup times. You could make them pointers and construct them in
> OomPriorityManager's constructor. However, it might be better to make them
> #defines.

Done.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode45
chrome/browser/oom_priority_manager.cc:45: if (!timer_.IsRunning())
On 2010/08/31 18:00:03, agl wrote:
> { } is nice around multiline blocks.

Done.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode64
chrome/browser/oom_priority_manager.cc:64: bool SortRendererStats(RendererStats
first, RendererStats second) {
On 2010/08/31 18:00:03, agl wrote:
> Add a comment to mention that this returns true if |first| is `less than'
> |second|

Done.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode95
chrome/browser/oom_priority_manager.cc:95: BrowserList::const_iterator
browser_iterator = BrowserList::begin();
On 2010/08/31 18:00:03, agl wrote:
> IMPORTANT: this code needs to run on the file thread, not the main thread.

Done.  Can you explain to me why this is the case?

http://codereview.chromium.org/3235007/diff/2001/3003#newcode96
chrome/browser/oom_priority_manager.cc:96: while (browser_iterator !=
BrowserList::end()) {
On 2010/08/31 18:00:03, agl wrote:
> This is a for loop and really should be written as such. (I know the line
> breaking is a pain.)

Done.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode98
chrome/browser/oom_priority_manager.cc:98: CHECK(browser);
On 2010/08/31 18:00:03, agl wrote:
> DCHECKs. If either are NULL, Release is going to abort anyway :)

Removed these.  If either are NULL, then Debug is going to abort too, and right
away, so there's no advantage to having a CHECK (or DCHECK).

http://codereview.chromium.org/3235007/diff/2001/3003#newcode106
chrome/browser/oom_priority_manager.cc:106: scoped_ptr<ProcessMetrics> metrics(
On 2010/08/31 18:00:03, agl wrote:
> This seems wrong. The number you want is the PSS, which is recorded in
> working_set_kbytes.shared. (See the comment in process_util.h.)
> 
> On old Linux systems, this may be zero. In which case you should take the
value
> in |priv|.

OK, so this is the exact algorithm that the task manager uses for "Physical
Memory".  Do you think proportional would work better as an estimate for which
process is hogging the most and hence should be killed first?

http://codereview.chromium.org/3235007/diff/2001/3004
File chrome/browser/oom_priority_manager.h (right):

http://codereview.chromium.org/3235007/diff/2001/3004#newcode14
chrome/browser/oom_priority_manager.h:14: // the browsers, and adjusts the out
of memory (OOM) adjustment value
On 2010/08/31 18:00:03, agl wrote:
> s/,//

Done.

http://codereview.chromium.org/3235007/diff/2001/3004#newcode21
chrome/browser/oom_priority_manager.h:21: // of priority.  So that we can bucket
things (because no two tabs
On 2010/08/31 18:00:03, agl wrote:
> s/of priority//
> 
> Also, flip the "So that we..." can sentence:
> 
> We round the idle times to the nearest kTimeBucketDuration so that we can
bucket
> them, as no two tabs will have exactly the same idle time.

Done.

http://codereview.chromium.org/3235007/diff/2001/3004#newcode35
chrome/browser/oom_priority_manager.h:35:
base::RepeatingTimer<OomPriorityManager> timer_;
On 2010/08/31 18:00:03, agl wrote:
> DISALLOW_COPY_AND_ASSIGN(OomPriorityManager)?

Done. Thanks, forgot that.

http://codereview.chromium.org/3235007/diff/2001/3010
File sandbox/linux/suid/process_util_linux.c (right):

http://codereview.chromium.org/3235007/diff/2001/3010#newcode24
sandbox/linux/suid/process_util_linux.c:24: static void PrintError(const char
*msg, ...)
On 2010/08/31 18:00:03, agl wrote:
> Remove this and the error printing. Too fluffy for the sandbox.

Awww. :(  But it was so pretty!  Done.

http://codereview.chromium.org/3235007/diff/2001/3010#newcode37
sandbox/linux/suid/process_util_linux.c:37: if (score < -17 || score > 15)
On 2010/08/31 18:00:03, agl wrote:
> IMPORTANT: this is a SUID process. The < 0 limit is important.

Done.  Good point. I was just trying to make the functionality of the function
match the functionality of oom_adj, but you're right that a suid process
probably shouldn't allow that.

I've reverted all changes to this file.

http://codereview.chromium.org/3235007/diff/2001/3010#newcode70
sandbox/linux/suid/process_util_linux.c:70: char buf[4];  // -17 <= |score| <=
15;
On 2010/08/31 18:00:03, agl wrote:
> please also revert this.

Done.

http://codereview.chromium.org/3235007/diff/2001/3011
File sandbox/linux/suid/sandbox.c (right):

http://codereview.chromium.org/3235007/diff/2001/3011#newcode379
sandbox/linux/suid/sandbox.c:379: 
On 2010/08/31 18:00:03, agl wrote:
> remove

Done.  Whoops, obviously didn't even mean to modify this file.

[agl, 2010-09-01 14:18:07]

LGTM

http://codereview.chromium.org/3235007/diff/2001/3002
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3235007/diff/2001/3002#newcode1417
chrome/browser/browser_main.cc:1417: browser::OomPriorityManager
oom_priority_manager;
On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> Done.  What times need documenting?  Do I need to do a microbenchmark of the
> constructor for this object?

Sorry, 'times' was a bad word. I just meant the relative order, which you have
already done.

http://codereview.chromium.org/3235007/diff/2001/3003
File chrome/browser/oom_priority_manager.cc (right):

http://codereview.chromium.org/3235007/diff/2001/3003#newcode95
chrome/browser/oom_priority_manager.cc:95: BrowserList::const_iterator
browser_iterator = BrowserList::begin();
On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> Done.  Can you explain to me why this is the case?

Although using /proc should be fast (you would think), it can still be stuck
behind locks which bring it down to the speed of the root filesystem (because
looking up /proc is a rootfs operation). In general, all disk access is
forbidden on the main thread in order to avoid jankyness.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode96
chrome/browser/oom_priority_manager.cc:96: while (browser_iterator !=
BrowserList::end()) {
On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> Done.

(Not in the code that you uploaded, but I'll take your word.)

http://codereview.chromium.org/3235007/diff/2001/3003#newcode106
chrome/browser/oom_priority_manager.cc:106: scoped_ptr<ProcessMetrics> metrics(
On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> OK, so this is the exact algorithm that the task manager uses for "Physical
> Memory".  Do you think proportional would work better as an estimate for which
> process is hogging the most and hence should be killed first?

I think you might be looking at Windows specific code in that case. PSS is the
value that you want and, according to the comments, it's in the 'shared' member.

[Greg Spencer (Chromium), 2010-09-02 19:32:41]

http://codereview.chromium.org/3235007/diff/2001/3003
File chrome/browser/oom_priority_manager.cc (right):

http://codereview.chromium.org/3235007/diff/2001/3003#newcode95
chrome/browser/oom_priority_manager.cc:95: BrowserList::const_iterator
browser_iterator = BrowserList::begin();
On 2010/09/01 14:18:07, agl wrote:
> On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> > Done.  Can you explain to me why this is the case?
> 
> Although using /proc should be fast (you would think), it can still be stuck
> behind locks which bring it down to the speed of the root filesystem (because
> looking up /proc is a rootfs operation). In general, all disk access is
> forbidden on the main thread in order to avoid jankyness.

OK, that makes sense, thanks.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode96
chrome/browser/oom_priority_manager.cc:96: while (browser_iterator !=
BrowserList::end()) {
On 2010/09/01 14:18:07, agl wrote:
> On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> > Done.
> 
> (Not in the code that you uploaded, but I'll take your word.)

Whoops.  It'll be in the submission.

http://codereview.chromium.org/3235007/diff/2001/3003#newcode106
chrome/browser/oom_priority_manager.cc:106: scoped_ptr<ProcessMetrics> metrics(
On 2010/09/01 14:18:07, agl wrote:
> On 2010/08/31 21:36:57, Greg Spencer (Chromium) wrote:
> > OK, so this is the exact algorithm that the task manager uses for "Physical
> > Memory".  Do you think proportional would work better as an estimate for
which
> > process is hogging the most and hence should be killed first?
> 
> I think you might be looking at Windows specific code in that case. PSS is the
> value that you want and, according to the comments, it's in the 'shared'
member.

Actually, it's not in Windows specific code: it's in task_manager.cc, and not
wrapped by any OS_ macros, and appears to be compiled into linux as well.

In any case, I switched to just using the PSS instead of the physical memory
algorithm, since I don't actually think it matters much -- we really just want a
relative badness score for the tabs, and either one gives us some version of
that.