105344 - Memory cache pref should be a percentage of physical RAM

Status: VERIFIED FIXED

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

Description

[David Hyatt]

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]]

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]

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)]

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]

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]

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)]

17%??? Geez.

OK, then, consider me convinced :)

Comment 7

[Gagan]

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]

Moving out to 0.9.7 to give us time to discuss options.

Comment 9

[gordon]

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]

--> gordon

Comment 11

[gordon]

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]

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)]

...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]

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]

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]

excellent suggestion... i very much agree :)

Comment 17

[gordon]

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 emeritus)]

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]

we should probably incorporate upper and lower bounds so as to ensure that the
memory cache size is always reasonable.

Comment 20

[scottputterman]

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 [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 21

[Matthew T (active 1999-2002)]

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]

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 [:bzbarsky]]

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]

*** Bug 194619 has been marked as a duplicate of this bug. ***

Comment 25

[Serge Gautherie (:sgautherie)]

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]

Attachment: 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]

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]]

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]

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]

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]

>  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]]

Why not use PR_CeilingLog2 instead of natural logarithms?

/be

Comment 33

[gordon]

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]

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]

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]

Attachment: updated patch

This patch defines NS_APP_CACHE_PARENT_DIR and removes
browser.cache.memory.capacity from prefs.js.

Comment 37

[gordon]

Attachment: version 3

more tweaks.

Comment 38

[gordon]

Attachment: v4 - should work on winows now...

Comment 39

[gordon]

Attachment: v5 - fixes two minor typos that break windows

Comment 40

[gordon]

Comment on attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows

r=saari

Comment 41

[gordon]

I need to add an #include for MacOSX.

Comment 43

[Boris Zbarsky [:bzbarsky]]

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]

Attachment: 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]]

Comment on attachment 121426 [details] [diff] [review]
Patch to fix aix/hpux

a=mkaply for checkin to 1.4b

Comment 46

[benc]

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]

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 [:bzbarsky]]

Ben, this is already checked in....

Comment 49

[benc]

You mean you did the checkin in #43?

(why is the status ASSIGNED?)

Comment 50

[Boris Zbarsky [:bzbarsky]]

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]

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]

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

[[e-mail]]

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]

see bug 105344 for some ideas for a refinement of this algorithm

Comment 55

[Ashley Bischoff (blog at handcoding.com)]

Jo: Is that the bug number you really meant to mention? (Bug 105344 is /this bug/)

Comment 56

[Jo Hermans]

Ah you're right. It's bug 204164.

Comment 57

[benc]

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]

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]

"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]

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]

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 [:bzbarsky]]

> 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]

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] Not actively reading bugmail]

Manuel: where's the automated testcase, since you marked this as in-testsuite+?  Was that perhaps unintentional?

Comment 65

[Stephen Donner [:stephend] Not actively reading bugmail]

Clearing in-testsuite+ flag that [e-mail] has (seemingly erroneously) set.