Last Comment Bug 105344 - Memory cache pref should be a percentage of physical RAM
: Memory cache pref should be a percentage of physical RAM
Status: VERIFIED FIXED
: topembed+
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.4beta
Assigned To: gordon
:
Mentors:
: 194619 (view as bug list)
Depends on: 194619
Blocks: 71668 109415 192762
  Show dependency treegraph
 
Reported: 2001-10-17 18:37 PDT by David Hyatt
Modified: 2014-04-26 03:30 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rough draft of patch to dynamically determine mem cache capacity (17.14 KB, patch)
2003-04-18 16:14 PDT, gordon
no flags Details | Diff | Splinter Review
updated patch (19.52 KB, patch)
2003-04-22 12:42 PDT, gordon
no flags Details | Diff | Splinter Review
version 3 (24.59 KB, patch)
2003-04-22 15:45 PDT, gordon
no flags Details | Diff | Splinter Review
v4 - should work on winows now... (24.04 KB, patch)
2003-04-22 16:17 PDT, gordon
no flags Details | Diff | Splinter Review
v5 - fixes two minor typos that break windows (24.04 KB, patch)
2003-04-22 18:42 PDT, gordon
gordon: review+
darin.moz: superreview+
Details | Diff | Splinter Review
Patch to fix aix/hpux (683 bytes, patch)
2003-04-23 07:35 PDT, Jim Dunn
mozilla: approval1.4b+
Details | Diff | Splinter Review

Description David Hyatt 2001-10-17 18:37:33 PDT
This bug is designed to work in conjunction with pavlov's improvements to image 
lib.  The memory cache preference should actually be a percentage number that 
represents what percentage of physical RAM should be used.

My proposal is that the default value for this pref be 6.25%, or 1/16 of the 
user's physical memory.  If you examine the physical memory values, you'll see 
how this scales:

PHYSICAL MEMORY        CACHE SIZE
32 megs                2 megs
64 megs                4 megs
128 megs               8 megs
256 megs               16 megs
512 megs               32 megs
1 GB                   64 megs
Comment 1 Matthias Versen [:Matti] 2001-10-17 18:49:11 PDT
Hyatt: 
Mozilla is near unusable on 32Meg and it "works" slooow with 64MB.
Is 4MB Cache for &4MB not a little bit to much ?
I suggest 4% = ~5MB with 128MB RAM (5000KByte is the current default)

 
Comment 2 David Hyatt 2001-10-17 19:00:57 PDT
This chart is actually kinder to 32mb then our current default, and retains the 
same defaults for 64mb.  

The reason I think 6.25% is ok is that with pav's improvements to image lib, a 
certain percentage of this megabyte total will actually reside in the video 
card's memory.

Comment 3 Bradley Baetz (:bbaetz) 2001-10-17 19:32:50 PDT
I like this in theory, however I'm worried that towards the upper end, this may
not be a practical value.

Besides, what is cache hit rate when the cache is that big? How many people
routinely use this much? Remember that our cache currently will save all non-ssl
web pages to the disk cache, not the mem cache - we don't cascade down. Only
decoded images and chrome go into the mem cache. In fact, I've been browsing all
afternoon, and according to about:cache my mem cache has a maximum storage size
of 4194304 Bytes, but only 2750740 Bytes are in use. Most of that is chrome
(including some 0-byte sized ones, for some reason). I don't know how typical
this is, but blake is also seeing the same numbers - noone else currently on irc
has a build which has been running for any length of time.

I have 128M of RAM, so someone would have to almost double the ammount of space
each image takes up before we'd see any affect, and quadruple it before the new
proposed mem cache would be full. Again, I'm not sure what hit rate I would get
over so many images.

If we do change this, how about just changing the default for a new profile? It
really annoys me on the versions of windows where the recycle bin setting was
fixed to an integer %, and you can never set a prcise number.
Comment 4 David Hyatt 2001-10-17 19:44:52 PDT
This is one of those bugs that would substantially improve tests like i-bench
and jrgm's test, but that may not mean very much in real world usage.  

Given that the page load improvement on a machine with a fair bit of RAM (256mb
or more) is on the order of 17%, it seems worth doing, doesn't it?
Comment 5 David Hyatt 2001-10-17 19:50:53 PDT
The other point that should be stressed is that there's a rough correlation
between having a machine loaded with RAM and also having a decent video card. 
With pav's changes the video card can hold a bunch of the images.

For example, with pav's changes, I had my memory cache set to 32mb.  After
running through jrgm's tests 5 times, I ended up using only 5mb of physical
memory for 29mb of images. The video card was holding the other 24mb.  

So keep in mind that this number is a sort of hard maximum including the video
card's memory.
Comment 6 Bradley Baetz (:bbaetz) 2001-10-17 20:01:26 PDT
17%??? Geez.

OK, then, consider me convinced :)
Comment 7 Gagan 2001-10-18 09:57:48 PDT
don't forget that the mem cache has a lower and upper limit. Gordon and I have
talked about this on several occasions before. In general I agree that this is a
good idea but deciding on the percentage maybe tricky. cc'ing Gordon.
Comment 8 David Hyatt 2001-10-18 11:09:51 PDT
Moving out to 0.9.7 to give us time to discuss options.
Comment 9 gordon 2001-10-18 12:08:24 PDT
Another thing to take into account, is when we move to memory-mapped I/O for the
disk cache device, we will be utilizing a larger portion of available memory
because the OS (windows & unix at least) can cache the oft accessed portions of
the mapped files.

What criteria should we use to decide if we want to go with this?  Page-load
performance is certainly one (+17% makes me drool), but are there others?  If we
grab 32 or 64 Mb, are we still "good citizens" or do we even care?
Comment 10 David Hyatt 2001-10-18 12:26:50 PDT
--> gordon
Comment 11 gordon 2001-11-16 18:04:43 PST
Okay, let's have a new pref rather than reinterpreting the old one.  How about 
'browser.cache.memory.percentage'.  Alternative suggestions?

I can do the backend work. Hyatt said he could help with the UI work.
Comment 12 gordon 2001-11-16 18:09:53 PST
There are two transition approaches that make sense to me.

1) Ignore the old pref. Provide a reasonable default for the new one, and let 
people change it.  Con: We might annoy people that have carefully set their mem 
cache size.

2) Old pref takes precedence over new pref, but the new pref is the default.  We 
remove the old default prefs, and the UI for changing them.  Con: Code is 
slightly more complex, and there is added complexity in testing the various old/
new pref interactions.

Feedback?
Comment 13 Bradley Baetz (:bbaetz) 2001-11-16 18:30:01 PST
...and if you do 2, then changing the new pref in the ui will remove the old one
from prefs.js? Hmm

Can we try to get the new pref in the cache code, and if that fails because it
doesn't exist, then set the new pref to <oldPref>/<ram in system> ?
Comment 14 Darin Fisher 2001-11-19 11:24:40 PST
why not introduce a third preference to indicate how to set the memory cache size:
ie. either as a fixed size or as a percentage of available RAM.  the UI could
have some sort of toggle between the two settings.
Comment 15 Stefan Seifert 2001-11-22 14:11:57 PST
I think you don't even need a toggle. Just a slider for percentage and a
textfield where the number in kilobytes is shown and if you adjust the slider
the number is changed and vice versa. This way you can easily change the
percentage while there's no problem if someone wants a concrete number.
Comment 16 Darin Fisher 2001-11-26 11:13:56 PST
excellent suggestion... i very much agree :)
Comment 17 gordon 2002-02-12 14:05:25 PST
Okay, I think I know how to get the amount of physical RAM on Mac and Windows,
but I don't know a good way to do this on Unix-like platforms.

cc'ing some folks that might know, or know who knows.
Comment 18 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-02-12 15:39:37 PST
On Linux, you want to read and parse /proc/meminfo.

I'm not sure how great an idea it is to have a default that's a %age of RAM, but
whatever.
Comment 19 Darin Fisher 2002-02-12 16:37:00 PST
we should probably incorporate upper and lower bounds so as to ensure that the
memory cache size is always reasonable.
Comment 20 scottputterman 2002-03-04 19:13:39 PST
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Comment 21 Matthew Paul Thomas 2002-08-13 08:58:28 PDT
Darin and I discussed this yesterday, considering that the best approach UI-wise
would be to remove the memory cache GUI altogether. If the cache size is a
percentage, any user will be highly unlikely to be both willing and able to work
out a percentage which is more performant than whatever we calculate. Neither
MSIE nor iCab have such a GUI; Opera does, but it's hardly a model of good
design. :-)
Comment 22 Judson Valeski 2003-01-10 16:31:25 PST
perhaps we can separate this bug out into two parts: backend, and UI. It would
be nice to just get the infrastructure (backend prefs/system calls) in place,
then deal w/ how it's presented to the user (if at all) separately.
Comment 23 Boris Zbarsky [:bz] 2003-02-23 13:06:45 PST
We've already removed the UI for memory cache at this point....

And see bug 194619 for a description of other deleterious effects of our present
tiny cache size.
Comment 24 gordon 2003-02-24 18:14:36 PST
*** Bug 194619 has been marked as a duplicate of this bug. ***
Comment 25 Serge Gautherie (:sgautherie) 2003-03-15 11:55:12 PST
Reply to comment 23:

I did not notice (may be I didn't pay attention) any information about the
removal of the M.C. UI;
And this is why I searched and found the current bug.

Then, I'll write one line to be explicit about it:
The removal seem to have happened between Mozilla v1.3a and v1.3b.
Comment 26 gordon 2003-04-18 16:14:16 PDT
Created attachment 121023 [details] [diff] [review]
rough draft of patch to dynamically determine mem cache capacity

I haven't tested the Windows code yet.	We should be using a
GlobalMemoryStatusEx() on Win2000 or WinXP.  Any suggestions/pointers on how to
do that cleanly?

Neither have I tested hpux.

The MacOSX code has been tested in a separate tool, but not as part of mozilla.


Rather than simply take a percentage of physical RAM, this patch uses a formula
that grows less than linearly as follows:

RAM	Cache
---	-----
  32 Mb   2 Mb
  64 Mb   4 Mb
 128 Mb   8 Mb
 256 Mb  14 Mb
 512 Mb  22 Mb
1024 Mb  32 Mb
2048 Mb  44 Mb
4096 Mb  58 Mb
Comment 27 gordon 2003-04-18 16:18:17 PDT
What other versions of Unix do we need to cover?

mkaply, can you provide code for determining physical memory size on OS/2?
Comment 28 Mike Kaply [:mkaply] 2003-04-18 19:58:10 PDT
OS/2 version - physical memory is in bytes

#define INCL_DOSMISC
#include <os2.h>

ULONG ulPhysMem;
DosQuerySysInfo(QSV_TOTPHYSMEM, QSV_TOTPHYSMEM, &ulPhysMem, sizeof(ulPhysMem));
       

Comment 29 gordon 2003-04-20 00:19:25 PDT
Thanks Michael!

I realized this afternoon (as I was building on my machine at home) that this
patch doesn't compile because I didn't include a couple of small changes I made
to files outside the mozilla/netwerk/cache/src directory.  I'll post a more
complete patch when I get to the office.
Comment 30 Darin Fisher 2003-04-20 11:04:37 PDT
Comment on attachment 121023 [details] [diff] [review]
rough draft of patch to dynamically determine mem cache capacity

>Index: nsCacheService.cpp

>+    char * prefList[] = { 
...
>+    };
>+    int listCount = sizeof(prefList)/sizeof(char*);

there's a handy macro for this called NS_ARRAY_LENGTH (see nsMemory.h)


some comments on this algorithm?

>+    double x = log(kbytes)/log(2) - 14;
>+    if (x > 0) {
>+        capacity    = (PRInt32)(x * x - x + 2.001); // add .001 for rounding
>+        capacity   *= 1024;
>+    } else {


i haven't fully reviewed everything...
Comment 31 Darin Fisher 2003-04-20 11:07:53 PDT
>  32 Mb   2 Mb
>  64 Mb   4 Mb
> 128 Mb   8 Mb
> 256 Mb  14 Mb
> 512 Mb  22 Mb
>1024 Mb  32 Mb
>2048 Mb  44 Mb
>4096 Mb  58 Mb

do we want to think about adding a ceiling on the amount of memory usage?  is
there a way for the user to configure this algorithm at all?
Comment 32 Brendan Eich [:brendan] 2003-04-20 12:26:19 PDT
Why not use PR_CeilingLog2 instead of natural logarithms?

/be
Comment 33 gordon 2003-04-21 12:18:18 PDT
Brendan, I'm not using PR_CeilingLog2() because I want a smoother curve; I don't
want limit cache size adjustment to even powers of 2, for example 96 Mb, or 384 Mb.

Darin, the algorithm isn't currently configurable (would that be a reason not to
land this?), however, we still respect the memory cache capacity preference, so
if a user wants a certain amount of cache memory, they can specify that (using a
text  editor, the pref was removed from the UI quite some time ago).

Comment 34 Darin Fisher 2003-04-21 13:24:02 PDT
gordon: yeah, the fact that the existing memory cache capacity pref trumps this
is probably sufficient.  how about adding the table from comment #26 to the code?
Comment 35 gordon 2003-04-21 16:45:43 PDT
Sure.  Good idea.

Hey, can I just use PR_FindSymbolAndLibrary() to determine whether
GlobalMemoryStatusEx() is available or not?


funcPtr = PR_FindSymbolAndLibrary("GlobalMemoryStatusEx", &lib);

or is it better to make two separate calls:

lib = PR_LoadLibrary("Kernel32.lib");
if (lib) {
    funcPtr = PR_FindSymbol(lib, "GlobalMemoryStatusEx");
}
Comment 36 gordon 2003-04-22 12:42:59 PDT
Created attachment 121306 [details] [diff] [review]
updated patch

This patch defines NS_APP_CACHE_PARENT_DIR and removes
browser.cache.memory.capacity from prefs.js.
Comment 37 gordon 2003-04-22 15:45:17 PDT
Created attachment 121333 [details] [diff] [review]
version 3

more tweaks.
Comment 38 gordon 2003-04-22 16:17:00 PDT
Created attachment 121340 [details] [diff] [review]
v4 - should work on winows now...
Comment 39 gordon 2003-04-22 18:42:44 PDT
Created attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows
Comment 40 gordon 2003-04-22 19:13:18 PDT
Comment on attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows

r=saari
Comment 41 gordon 2003-04-22 19:27:53 PDT
I need to add an #include for MacOSX.
Comment 42 Darin Fisher 2003-04-22 19:36:19 PDT
Comment on attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows
Comment 43 Boris Zbarsky [:bz] 2003-04-23 03:00:32 PDT
In case anyone cares, here are some Tp numbers before/after this checkin:

Tbox          Before      After
btek          ~1095       ~1038
luna          ~1160       ~1090
silverstone    646         594
monkey        ~710        ~660
mecca         ~2260       ~2195

So looks like this is working as advertised (a pretty constand 50-70ms speedup
across all tboxes).  ;)  Note that the only one of these machines that has a RAM
amount listed is mecca and that has 128MB.  I suspect they all have at least
128MB of RAM, hence they are all using more memory cache now (our old default
was 4MB).
Comment 44 Jim Dunn 2003-04-23 07:35:18 PDT
Created attachment 121426 [details] [diff] [review]
Patch to fix aix/hpux

This broke AIX tinderbox... (and hp-ux builds)
confusion over which "log" prototype to use.
Just casting to double fixes this, otherwise
I think float is assumed.
Comment 45 Mike Kaply [:mkaply] 2003-04-23 13:28:19 PDT
Comment on attachment 121426 [details] [diff] [review]
Patch to fix aix/hpux

a=mkaply for checkin to 1.4b
Comment 46 benc 2003-04-23 23:12:48 PDT
So if you have an existing memory cache value that is not the default, and you
have moved to a newer version that does not have UI for mem cache, then you will
continue to use the value you picked right?

If so, maybe a relnote reminding people they can clear the value via about:config?

BTW, we now have prefs documentation:

http://www.mozilla.org/quality/networking/docs/netprefs.html#cache.

Let me know how you want that updated once this is fixed.
Comment 47 benc 2003-04-23 23:33:27 PDT
requesting for 1.4b. I'd really like to see this get alot of usage, and beating
the 1.4b milestone is the way to go.
Comment 48 Boris Zbarsky [:bz] 2003-04-24 08:07:42 PDT
Ben, this is already checked in....
Comment 49 benc 2003-04-24 09:00:50 PDT
You mean you did the checkin in #43?

(why is the status ASSIGNED?)
Comment 50 Boris Zbarsky [:bz] 2003-04-24 09:15:57 PDT
Comment 43 was based on the tinderbox numbers after gordon checked it in.

And some people like to keep a bug open till they are sure they won't have to
back it out due to nasty blockers.
Comment 51 gordon 2003-04-24 11:32:40 PDT
Exactly.  Thanks Boris.  I've checked in the version 5 of the patch with the
addition of an #include for MacOSX, as well as Jim Dunn's patch for AIX. 
Nothing horrible seems to have happened in the last day, so I'll mark this FIXED
now.
Comment 52 benc 2003-04-28 10:32:05 PDT
bz: I'm just explaining why I'm confused. I got bugs for about a half dozen
coders, so I try to glance at the summary rather than re-read every bug I look at.

Comment 53 dwitte@gmail.com 2003-05-01 23:52:26 PDT
Comment on attachment 121023 [details] [diff] [review]
rough draft of patch to dynamically determine mem cache capacity

clearing obsolete review request.
Comment 54 Jo Hermans 2003-05-22 04:38:38 PDT
see bug 105344 for some ideas for a refinement of this algorithm
Comment 55 Ashley Bischoff (blog at handcoding.com) 2003-05-22 08:51:00 PDT
Jo: Is that the bug number you really meant to mention? (Bug 105344 is /this bug/)
Comment 56 Jo Hermans 2003-05-22 13:47:37 PDT
Ah you're right. It's bug 204164.
Comment 57 benc 2003-05-30 12:00:57 PDT
VERIFIED: allplats. Mozilla 1.4

browser.cache.memory.capacity is gone.
I'll update the prefs docs to reflect that.

I checked the about:cache on several systems here's what I got:

iMac, 256 MB, 14336k
iBook, 640 MB, 25600k
Dell, 192 MB, 11264k
Dell,  128 MB, 7168 k

Comment 58 gordon 2003-06-03 16:25:41 PDT
Ben, the browser.cache.memory.capacity pref isn't gone, just the default value
in prefs.js.  If the pref is set, we respect it.
Comment 59 benc 2003-06-03 22:22:12 PDT
"gone" meant not in about:config by default. I've describe the behavior of this
pref in the netprefs docs as what you described in this bug.
Comment 60 Jim Booth 2003-06-05 08:38:25 PDT
This fix may be the cause of a SERIOUS new bug 204374 on Windows 9x machines. 
It's setting memory cache so high that too many GDI resources are tied up.  

(My PC has 384MB RAM and the memory cache was over 18K.  Setting
browser.cache.memory.capacity down to 4096 manually seems to fix it.)

It seems that you should take the OS into consideration before setting a large
cache size.
Comment 61 Andrew Hagen 2003-06-06 07:22:27 PDT
The fix for this bug caused a huge regression detailed in bug 204374, affecting
Windows builds, primarily Windows 9x builds. The regression in question leads to
a significant exacerbation of a long standing problem in the memory cache. 

Reopening as this is not acceptable for 1.4. Suggesting that the fix here should
probably be backed out as an emergency measure, and then we can figure out how
to address the long standing problems.
Comment 62 Boris Zbarsky [:bz] 2003-06-06 21:26:43 PDT
> The fix for this bug caused a huge regression detailed in bug 204374

Which is where discussion of the fix should continue (ccing gordon on that bug
would have been a good start).  _If_ this patch is backed out, reopen this bug.
Comment 63 benc 2003-06-08 17:37:37 PDT
VERIFIED/FIEXE:
Please file a new bug, and nominate it to block 1.4.

There are plaform-specific ways of addressing a platform-specific problem.

Comment 64 Stephen Donner [:stephend] 2009-02-13 18:25:45 PST
Manuel: where's the automated testcase, since you marked this as in-testsuite+?  Was that perhaps unintentional?
Comment 65 Stephen Donner [:stephend] 2009-02-18 16:42:02 PST
Clearing in-testsuite+ flag that truquito_2m@hotmail.com has (seemingly erroneously) set.

Note You need to log in before you can comment on or make changes to this bug.