296538 - limit the memory cache to a reasonable value

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[Jo Hermans]

I have 1GB of RAM, so I'm using 31MB of memory cache (b/c of the changes in bug
105344). But it takes a very long time to actually fill up 31MB of memory cache
 (often around +100MB VM size), I think this is much too large. It's a lot
better than the old one (4MB I think ?), but 31MB for 1GB or 58MB for 4GB is
ridiculous.

This is part of the reason why people are claiming that Firefox has a memory
leak, and keeps growing. What they don't see, is that Firefox will really delete
some memory, but it takes too long before that actually happens.

Read bug 105344 comment 31 (comment by Darin) : maybe we can add a limit on the
amount of memory usage ? I've modified browser.cache.memory.capacity to 16384
KB, and it runs much better now. I didn't notice a slow-down (16MB is still a
huge cache), and my browser isn't growing anymore beyond 60 MB VM Size. I think
this might shut up most critics ("FF 1.1 is using 40MB less than FF 1.0"), while
still providing enough cache for a speedy browser. The exact limit can be
debated ofcourse. 

See also bug 204164.

Comment 1

[Asa Dotzler [:asa]]

Darin, are you going to have time to do this for the upcoming release? 

Comment 2

[Darin Fisher]

Yeah, I can add this to my list.  It is lower priority than other things on my
list, but I generally agree that we should set a reasonable limit on the size of
the memory cache.

Alfred: Can you help create a patch for this bug?

Comment 3

[Alfred Kayser]

Darin, I will be on holidays for the next two weeks...

Comment 4

[Jo Hermans]

Something like this (to be placed at the end of
nsCacheService::CacheMemoryAvailable) :

#define MEMORY_CACHE_MAX_CAPACITY  (16 * 1024 * 120)

if (LL_CMP(capacity, >, LL_MEMORY_CACHE_MAX_CAPACITY))
   capacity = MEMORY_CACHE_MAX_CAPACITY;


This would limit the memory cache to 16MB, no matter what the calculation
reached. I don't see the point of placing yet another entry in the preferences
(browser.cache.memory.max_capacity ?). You can still override it by placing a
value in browser.cache.memory.capacity (0 included).

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

> #define MEMORY_CACHE_MAX_CAPACITY  (16 * 1024 * 120)

should that 120 be 1024?

> if (LL_CMP(capacity, >, LL_MEMORY_CACHE_MAX_CAPACITY))

if you use LL_CMP, you should use real PRInt64s, or you might as well use >
directly...

Comment 6

[Daniel Ovsiannikov]

Does that "limit" work at all? 

Memory cache device

Number of entries: 	771
Maximum storage size: 	16384 KiB
Storage in use: 	38443 KiB (?!!)
Inactive storage: 	0 KiB

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 ID:2005111116

Comment 7

[Jo Hermans]

yes, but there are 2 things :

- if there's a page on the screen (visible or in a tab), it might use more memory that actually allowed, becuase that memory can't be thrown away.

- if you're using Adblock 0.5.2.056, then you should better remove it (not just disable it), because it has serious memory leaks. It could be the reason that all memory is active (bottom counter), so it can't be thrown away

Comment 8

[Marius Strumyla]

in addition to the list above

- blazingly fast back feature will ignore the memory cache limit. a tab may keep in memory several already visited pages even though the cache memory limit has been reached.
See Bug 321661.

Comment 9

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060114 Firefox/1.6a1 ID:2006011405

what's wrong with browser.cache.memory.capacity pref ?
I set browser.cache.memory.capacity 16384 (integer) as suggested in comment 0.
I decided to reset it, to check the default value.
Instead it resets to an empty STRING i/o an INT with a value.

http://kb.mozillazine.org/Browser.cache.memory.capacity also seems to suggest it should be an integer

Comment 10

[Jo Hermans]

(In reply to comment #9)
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060114
> Firefox/1.6a1 ID:2006011405
> 
> what's wrong with browser.cache.memory.capacity pref ?
> I set browser.cache.memory.capacity 16384 (integer) as suggested in comment 0.
> I decided to reset it, to check the default value.
> Instead it resets to an empty STRING i/o an INT with a value.
> 
> http://kb.mozillazine.org/Browser.cache.memory.capacity also seems to suggest
> it should be an integer
> 

I'm not seeing it here, I see an int field without a value.

But anyway, since it has no default, it will disappear when you reset it. It's still visible until you restart the browser. I don't think an empty string or empty int would hurt.

Comment 11

[Steve Chapel]

A while ago, I coded up new logic (see code below) for determining the default memory cache size that will reduce memory cache usage. It fixes three problems:

1) It reduces the default size of the memory cache for common RAM sizes. On a system with 512 MB of RAM, currently the default size is currently 21 or 22 MB (see #3). With this patch, it would be 14 MB.

Here's a table comparing the default cache sizes for common RAM sizes:
  RAM        current    proposed
  32 MB        2 MB        2 MB
  64 MB        4 MB        4 MB
 128 MB        8 MB        6 MB
 256 MB       14 MB       10 MB
 512 MB       22 MB       14 MB
1024 MB       32 MB       18 MB
2048 MB       44 MB       24 MB
4096 MB       58 MB       30 MB

2) It sets an upper limit on the cache size. No matter how much physical RAM is present, the default cache size would not exceed 32 MB. This maximum would apply only to systems with more than 4 GB of RAM.

3) It corrects a rounding error in the current computation. On my 1 GB computer, the default cache size is 31 MB, not 32 MB like it's supposed to be.

If you like these changes, I can attach a genuine source code patch.

Here's the new logic (compare to patch in bug 105344):

    double x = log((double)kbytes)/log((double)2) - 14;
    if (x > 0) {
        capacity = (PRInt32)(x * x / 3 + x + 2.0 / 3 + 0.1); // 0.1 for rounding
        if (capacity > 32)
            capacity = 32;
        capacity   *= 1024;
    } else {
        capacity    = 0;
    }

Comment 12

[Darin Fisher]

Steve: Thanks for taking the time to revise the algorithm.  I like your suggested values.  If anyone has time to write up a patch for this, I'll gladly review it and help get it into the tree.

Comment 13

[Alfred Kayser]

Attachment: Simple patch to implement the algo. from comment #11

Simple replacement of calculation, including the limit check to 32MiB.

Comment 14

[Steve Chapel]

I suppose we should also change the corresponding code at
http://lxr.mozilla.org/seamonkey/source/docshell/shistory/src/nsSHistory.cpp#199

Comment 15

[Jo Hermans]

(In reply to comment #14)
> I suppose we should also change the corresponding code at
> http://lxr.mozilla.org/seamonkey/source/docshell/shistory/src/nsSHistory.cpp#199
> 

Why ? This only calculates the number of content viewers (= pages in the fbcache), not the amount of memory used. If you would copy the algorithm verbatim, you would end up at maximum 4 contentviewers (1 GB and up). The maxmimum of 8 would not be reached, even at 4GB. That's half of the current number of viewers. Although that uses less memory, it also drastically reduces the number of pages in the fb cache, which is a totally different discussion.

Comment 16

[Alfred Kayser]

Let's keep the scope of this bug to the memory cache itself.
Memory usage issues of bfcache are / will be addressed in separate bugs.

Comment 17

[Darin Fisher]

Comment on attachment 212735 [details] [diff] [review]
Simple patch to implement the algo. from comment #11

I think it'd be nice to make the ceiling pref controlled, but this is definitely an improvement over what we have today.

Can you please revise the comments above nsCacheProfilePrefObserver::MemoryCacheCapacity that describe this equation?

r=me with the comment change

Comment 18

[Darin Fisher]

Comment on attachment 212735 [details] [diff] [review]
Simple patch to implement the algo. from comment #11

please post a revised patch, thanks!

Comment 19

[Alfred Kayser]

Attachment: Now with updated comments

Comment 20

[Darin Fisher]

Comment on attachment 213296 [details] [diff] [review]
Now with updated comments

r=darin

Comment 21

[Darin Fisher]

fixed-on-trunk

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This may have been responsible for a 20ms Tp jump on btek.

Comment 23

[Darin Fisher]

> This may have been responsible for a 20ms Tp jump on btek.

Yeah, that is likely.  The size of the memory cache has a pretty direct impact on our page loader test.  The more of it we can keep in memory the better we do on that test.  Sort of arbitrary.  Do you think we should care?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, we probably shouldn't care, but we should give more people a chance to care :-)

Comment 25

[Darin Fisher]

fixed1.8.1