cap the number of tabs in the backing store cache

cap the number of tabs in the backing store cache

BUG=19353
TEST=None

submitted in r23584

[Erik does not do reviews, 2009-08-14 17:11:24]

cpu: backing store changes (since you said you're making changes there)
mbelshe: memory_details.cc (I updated a trivial comment, just making sure I
didn't misread)

[cpu_(ooo_6.6-7.5), 2009-08-14 18:27:22]

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

http://codereview.chromium.org/165538/diff/1/3#newcode42
Line 42: static size_t GetBackingStoreCacheMemoryTier() {
After reading the CL, I found that it would have been easier to read if this
function has the word Max or Maximum.

For example:

if (large_cache->size() >= GetBackingStoreCacheMemoryTier())
  ExpireLastBackingStore(large_cache);
would be

if (large_cache->size() >= MaxNumberOfBackingStores())
    ExpireLastBackingStore(large_cache);


I am not attached to these names btw.

Same goes for GetBackingStoreCacheMemorySize() given that we also have
BackingStoreManager::MemorySize(). They both measure the cached memory but one
is an actual value and the other one is a maximum.

http://codereview.chromium.org/165538/diff/1/3#newcode130
Line 130: if (large_cache->size() >= GetBackingStoreCacheMemoryTier())
I think it is better to Expire before Alloc, this way there might be a chance
for the OS to reuse these warm pages.

[cpu_(ooo_6.6-7.5), 2009-08-14 18:29:33]

As a general comment I find it interesting that we have two relatively
independent eviction policies here: one that looks at the memory allocated and
one that looks at the number of backing stores. My gut feeling is that one of
them is superfluous.

[Erik does not do reviews, 2009-08-15 01:23:27]

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

http://codereview.chromium.org/165538/diff/1/3#newcode42
Line 42: static size_t GetBackingStoreCacheMemoryTier() {
On 2009/08/14 18:27:22, cpu wrote:
> After reading the CL, I found that it would have been easier to read if this
> function has the word Max or Maximum.
> 
> For example:
> 
> if (large_cache->size() >= GetBackingStoreCacheMemoryTier())
>   ExpireLastBackingStore(large_cache);
> would be
> 
> if (large_cache->size() >= MaxNumberOfBackingStores())
>     ExpireLastBackingStore(large_cache);
> 
> 
> I am not attached to these names btw.
> 
> Same goes for GetBackingStoreCacheMemorySize() given that we also have
> BackingStoreManager::MemorySize(). They both measure the cached memory but one
> is an actual value and the other one is a maximum.
> 

Fair enough.  The lack of max was bugging me too this time around, just not
enough to overcome my laziness on renaming. ;-)  I think your name pattern is
fine.

http://codereview.chromium.org/165538/diff/1/3#newcode130
Line 130: if (large_cache->size() >= GetBackingStoreCacheMemoryTier())
On 2009/08/14 18:27:22, cpu wrote:
> I think it is better to Expire before Alloc, this way there might be a chance
> for the OS to reuse these warm pages.

Good idea.  Fixed.

[Mike Belshe, 2009-08-15 04:30:28]

lgtm

http://codereview.chromium.org/165538/diff/1/2
File chrome/browser/memory_details.cc (left):

http://codereview.chromium.org/165538/diff/1/2#oldcode279
Line 279: // Memory is measured in units of 10KB.
Yes, you're right.

[cpu_(ooo_6.6-7.5), 2009-08-17 19:56:20]

LGTM