Closed
Bug 296538
Opened 20 years ago
Closed 19 years ago
limit the memory cache to a reasonable value
Categories
(Core :: Networking: Cache, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: jo.hermans, Assigned: darin.moz)
References
(Blocks 1 open bug)
Details
(Keywords: fixed1.8.1, helpwanted)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
darin.moz
:
review+
bzbarsky
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Updated•20 years ago
|
Flags: blocking1.8b4?
Comment 1•20 years ago
|
||
Darin, are you going to have time to do this for the upcoming release?
Assignee | ||
Comment 2•20 years ago
|
||
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•20 years ago
|
||
Darin, I will be on holidays for the next two weeks...
Reporter | ||
Comment 4•20 years ago
|
||
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•20 years ago
|
||
> #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...
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4-
Assignee | ||
Updated•19 years ago
|
Target Milestone: mozilla1.8beta4 → mozilla1.9alpha
Comment 6•19 years ago
|
||
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
Reporter | ||
Comment 7•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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
Reporter | ||
Comment 10•19 years ago
|
||
(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•19 years ago
|
||
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;
}
Assignee | ||
Comment 12•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Comment 13•19 years ago
|
||
Simple replacement of calculation, including the limit check to 32MiB.
Attachment #212735 -
Flags: review?(darin)
Comment 14•19 years ago
|
||
I suppose we should also change the corresponding code at
http://lxr.mozilla.org/seamonkey/source/docshell/shistory/src/nsSHistory.cpp#199
Reporter | ||
Comment 15•19 years ago
|
||
(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•19 years ago
|
||
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.
Assignee | ||
Comment 17•19 years ago
|
||
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
Assignee | ||
Comment 18•19 years ago
|
||
Comment on attachment 212735 [details] [diff] [review]
Simple patch to implement the algo. from comment #11
please post a revised patch, thanks!
Attachment #212735 -
Flags: review?(darin) → review-
Comment 19•19 years ago
|
||
Attachment #212735 -
Attachment is obsolete: true
Attachment #213296 -
Flags: review?(darin)
Assignee | ||
Comment 20•19 years ago
|
||
Comment on attachment 213296 [details] [diff] [review]
Now with updated comments
r=darin
Attachment #213296 -
Flags: review?(darin) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #213296 -
Flags: approval-branch-1.8.1?(bzbarsky)
Assignee | ||
Comment 21•19 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This may have been responsible for a 20ms Tp jump on btek.
Assignee | ||
Comment 23•19 years ago
|
||
> 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?
No, we probably shouldn't care, but we should give more people a chance to care :-)
Updated•19 years ago
|
Attachment #213296 -
Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Reporter | ||
Updated•18 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•