Scale backing store cache size

Scale backing store cache size.

The approach is borrowed from render_process_host's kMaxRenderersByRamTier.

The values were chosen to keep a minimal number of DIBs in the extreme low RAM
case, and scale up linearly from there to the previous (constant) value of 5.

Patch by Joel Stanley.

BUG=12028

[Dean McNamee, 2009-05-18 08:05:30]

I am fine with the code not being share w/ the number of renders.  The tables
are different anyway.  Might be worth filing a bug to try to clean it up later.

Adding iyengar since this was originally his TODO.

http://codereview.chromium.org/115452/diff/1/3
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/115452/diff/1/3#newcode20
Line 20: 
Drop the extra new line.

http://codereview.chromium.org/115452/diff/1/3#newcode25
Line 25: 5                         // 768MB and above
bring the comments in so there are two spaces after the , (and three for the
last line).

http://codereview.chromium.org/115452/diff/1/3#newcode28
Line 28: static unsigned int max_size = 0;
you can actually make this one statement, so you don't need the if() part.  See
below.

http://codereview.chromium.org/115452/diff/1/3#newcode34
Line 34: max_size = kMaxDibCountByRamTier[memory_tier];
can this be simplified to:

max_size = kMaxDibCountByRamTier[std::max(memory_tier,
arraysize(kMaxDibCountByRamTier) - 1)];

Or do the std::max along with the call to AmountOfPhysicalMemoryMB.  Whatecer
comes out cleanest.

http://codereview.chromium.org/115452/diff/1/4
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/115452/diff/1/4#newcode86
Line 86: const unsigned int kMaxDibCount = 5;
I am not sure this should be in the constants, and additionally now the name
isn't completely truthful.  It is max dib count if you have over a certain
amount of ram.

[Joel Stanley (old), 2009-05-19 12:06:48]

On 2009/05/18 08:05:30, Dean McNamee wrote:
> I am fine with the code not being share w/ the number of renders.  The tables
> are different anyway.  Might be worth filing a bug to try to clean it up
later.
> 
> Adding iyengar since this was originally his TODO.
> 
> http://codereview.chromium.org/115452/diff/1/3
> File chrome/browser/renderer_host/backing_store.cc (right):
> 
> http://codereview.chromium.org/115452/diff/1/3#newcode20
> Line 20: 
> Drop the extra new line.

fixed.

> 
> http://codereview.chromium.org/115452/diff/1/3#newcode25
> Line 25: 5                         // 768MB and above
> bring the comments in so there are two spaces after the , (and three for the
> last line).

fixed.

> http://codereview.chromium.org/115452/diff/1/3#newcode28
> Line 28: static unsigned int max_size = 0;
> you can actually make this one statement, so you don't need the if() part. 
See
> below.
> 
> http://codereview.chromium.org/115452/diff/1/3#newcode34
> Line 34: max_size = kMaxDibCountByRamTier[memory_tier];
> can this be simplified to:
> 
> max_size = kMaxDibCountByRamTier[std::max(memory_tier,
> arraysize(kMaxDibCountByRamTier) - 1)];
> 
> Or do the std::max along with the call to AmountOfPhysicalMemoryMB.  Whatecer
> comes out cleanest.

I made the change to use std::max() instead of the if/else.  The outer if check
is still there, as this saves the calculation being done for every call of
GetBackingStoreCacheSize().

> http://codereview.chromium.org/115452/diff/1/4
> File chrome/common/chrome_constants.cc (right):
> 
> http://codereview.chromium.org/115452/diff/1/4#newcode86
> Line 86: const unsigned int kMaxDibCount = 5;
> I am not sure this should be in the constants, and additionally now the name
> isn't completely truthful.  It is max dib count if you have over a certain
> amount of ram.

Glad to drop it.  I didn't think it made much sense, but like most of this patch
I was following the lead of GetMaxRendererProcessCount in
browser/renderer_host/render_process_host.cc

[Dean McNamee, 2009-05-19 12:14:58]

http://codereview.chromium.org/115452/diff/2001/2003
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/115452/diff/2001/2003#newcode27
Line 27: static unsigned int max_size = 0;
don't use unsigned.

http://codereview.chromium.org/115452/diff/2001/2003#newcode31
Line 31: std::max(memory_tier, arraysize(kMaxDibCountByRamTier) - 1)];
you can still do this without the if(), since it can be a single statement, the
fact that it is static will have it only be evaluated once:

static int max_size = kMaxDibCountByRamTier[
    std::max(base::SysInfo::AmountOfPhysicalMemoryMB() / 256,
             arraysize(kMaxDibCountByRamTier) - 1)];

Maybe that is a mouthful, and the current code is better.  Dunno.

[Joel Stanley (old), 2009-05-19 13:27:12]

http://codereview.chromium.org/115452/diff/2001/2003
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/115452/diff/2001/2003#newcode27
Line 27: static unsigned int max_size = 0;
On 2009/05/19 12:14:58, Dean McNamee wrote:
> don't use unsigned.

fixed.

why don't we use unsigned for things like this, ie can't be negative?

http://codereview.chromium.org/115452/diff/2001/2003#newcode31
Line 31: std::max(memory_tier, arraysize(kMaxDibCountByRamTier) - 1)];
On 2009/05/19 12:14:58, Dean McNamee wrote:
> you can still do this without the if(), since it can be a single statement,
the
> fact that it is static will have it only be evaluated once:
> 
> static int max_size = kMaxDibCountByRamTier[
>     std::max(base::SysInfo::AmountOfPhysicalMemoryMB() / 256,
>              arraysize(kMaxDibCountByRamTier) - 1)];
> 
> Maybe that is a mouthful, and the current code is better.  Dunno.

Hrm.  I think the original version is much more readable.

This will be the most efficient at run time, being only run once and having no
branching.  Perhaps that's most important.

[Dean McNamee, 2009-05-19 13:32:04]

I am fine with this code-wise.

I think there maybe should be some debate on what we actually want the tier to
look like, but I'm not sure whose the right person to ask for that.

http://codereview.chromium.org/115452/diff/15/1009
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/115452/diff/15/1009#newcode29
Line 29: arraysize(kMaxDibCountByRamTier) - 1)];
The way MSVC implements this it will basically be equivalent to the previous if
() { }.  Another thing you could do is cache in the caller to
GetBackingStoreCacheSize(), so like

static int cache_size = GetBackingStoreCacheSize().

It's up to you what you think is cleanest.

As for unsigned, that's a long complicated story.  The style guide might give
you some idea of the reasoning.

[Joel Stanley (old), 2009-05-25 02:30:39]

On 2009/05/19 13:32:04, Dean McNamee wrote:
> I am fine with this code-wise.

iyengar, are you able to take a look at this for me?

[Joel Stanley (old), 2009-05-28 10:31:32]

Hey Evan, agl,

Dean suggested I ask for your input on what numbers and thresholds we should use
for the tier.

[agl, 2009-05-28 16:45:45]

The numbers seem reasonable. I'm not sure that I have any special insight into
what they should be. This is only a partial solution (we would like to drop
backing stores in response to memory pressure), but it's easy at least. I'd be
ok with landing this with the nits below sorted out.

http://codereview.chromium.org/115452/diff/15/1009
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/115452/diff/15/1009#newcode8
Line 8: #include "base/sys_info.h"
nit: includes should be alphabetical.

http://codereview.chromium.org/115452/diff/15/1009#newcode28
Line 28: std::max(base::SysInfo::AmountOfPhysicalMemoryMB() / 256,
maybe you want std::min here?

[Joel Stanley (old), 2009-05-29 07:37:48]

> http://codereview.chromium.org/115452/diff/15/1009#newcode8
> Line 8: #include "base/sys_info.h"
> nit: includes should be alphabetical.

fixed

> http://codereview.chromium.org/115452/diff/15/1009#newcode28
> Line 28: std::max(base::SysInfo::AmountOfPhysicalMemoryMB() / 256,
> maybe you want std::min here?

Yes, I did. Thanks.

It appears I did not compile this change previously.  arraysize() returns an
unsigned int, while AmountOfPhysicalMemoryMB() is an int.  std::max didn't know
how to compare a int and unsigned int.  Is a cast the correct fix here?

[Dean McNamee, 2009-05-29 08:41:06]

Ok, looks fine to me.

Want me to commit it for you?

[Joel Stanley (old), 2009-05-29 13:53:43]

On 2009/05/29 08:41:06, Dean McNamee wrote:
> Ok, looks fine to me.
> 
> Want me to commit it for you?

Yep, please do

[Joel Stanley (old), 2009-05-31 14:21:38]

Dean,

This should be fixed now.  If you could commit it for me that would be great.