David Hyatt Reporter
	Description • 23 years ago

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

	Matthias Versen [:Matti]
	Comment 1 • 23 years ago

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)

 

	David Hyatt Reporter
	Comment 2 • 23 years ago

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.

	Bradley Baetz (:bbaetz)
	Comment 3 • 23 years ago

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.

	David Hyatt Reporter
	Comment 4 • 23 years ago

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?

	David Hyatt Reporter
	Comment 5 • 23 years ago

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.

	Bradley Baetz (:bbaetz)
	Comment 6 • 23 years ago

17%??? Geez.

OK, then, consider me convinced :)

	Gagan
	Comment 7 • 23 years ago

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.

	David Hyatt Reporter
	Comment 8 • 23 years ago

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

	gordon Assignee
	Comment 9 • 23 years ago

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?

	David Hyatt Reporter
	Comment 10 • 23 years ago

--> gordon

	gordon Assignee
	Comment 11 • 23 years ago

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.

	gordon Assignee
	Comment 12 • 23 years ago

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?

	Bradley Baetz (:bbaetz)
	Comment 13 • 23 years ago

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

	Darin Fisher
	Comment 14 • 23 years ago

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.

	Stefan Seifert
	Comment 15 • 23 years ago

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.

	Darin Fisher
	Comment 16 • 23 years ago

excellent suggestion... i very much agree :)

	gordon Assignee
	Comment 17 • 23 years ago

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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 18 • 23 years ago

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.

	Darin Fisher
	Comment 19 • 23 years ago

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

	scottputterman
	Comment 20 • 23 years ago

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.

	Matthew T (active 1999-2002)
	Comment 21 • 22 years ago

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. :-)

	Judson Valeski
	Comment 22 • 22 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 22 years ago

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.

	gordon Assignee
	Comment 24 • 22 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 25 • 22 years ago

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.

	gordon Assignee
	Comment 26 • 22 years ago

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

	gordon Assignee
	Comment 27 • 22 years ago

What other versions of Unix do we need to cover?

mkaply, can you provide code for determining physical memory size on OS/2?

	Mike Kaply [:mkaply]
	Comment 28 • 22 years ago

OS/2 version - physical memory is in bytes

#define INCL_DOSMISC
#include <os2.h>

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

	gordon Assignee
	Comment 29 • 22 years ago

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.

	Darin Fisher
	Comment 30 • 22 years ago

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

	Darin Fisher
	Comment 31 • 22 years ago

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

	Brendan Eich [:brendan]
	Comment 32 • 22 years ago

Why not use PR_CeilingLog2 instead of natural logarithms?

/be

	gordon Assignee
	Comment 33 • 22 years ago

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

	Darin Fisher
	Comment 34 • 22 years ago

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?

	gordon Assignee
	Comment 35 • 22 years ago

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");
}

	gordon Assignee
	Comment 36 • 22 years ago

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

	gordon Assignee
	Comment 37 • 22 years ago

more tweaks.

	gordon Assignee
	Comment 40 • 22 years ago

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

r=saari

	gordon Assignee
	Comment 41 • 22 years ago

I need to add an #include for MacOSX.

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 22 years ago

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

	Jim Dunn
	Comment 44 • 22 years ago

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.

	Mike Kaply [:mkaply]
	Comment 45 • 22 years ago

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

a=mkaply for checkin to 1.4b

	benc
	Comment 46 • 22 years ago

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.

	benc
	Comment 47 • 22 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 48 • 22 years ago

Ben, this is already checked in....

	benc
	Comment 49 • 22 years ago

You mean you did the checkin in #43?

(why is the status ASSIGNED?)

	Boris Zbarsky [:bzbarsky]
	Comment 50 • 22 years ago

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.

	gordon Assignee
	Comment 51 • 22 years ago

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.

	benc
	Comment 52 • 22 years ago

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.

	dwitte@gmail.com
	Comment 53 • 22 years ago

Comment on attachment 121023 [details] [diff] [review]
rough draft of patch to dynamically determine mem cache capacity

clearing obsolete review request.

	Jo Hermans
	Comment 54 • 21 years ago

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

	Ashley Bischoff (blog at handcoding.com)
	Comment 55 • 21 years ago

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

	Jo Hermans
	Comment 56 • 21 years ago

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

	benc
	Comment 57 • 21 years ago

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

	gordon Assignee
	Comment 58 • 21 years ago

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.

	benc
	Comment 59 • 21 years ago

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

	Jim Booth
	Comment 60 • 21 years ago

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.

	Andrew Hagen
	Comment 61 • 21 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 62 • 21 years ago

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

	benc
	Comment 63 • 21 years ago

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.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 64 • 16 years ago

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

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 65 • 16 years ago

Clearing in-testsuite+ flag that truquito_2m@hotmail.com has (seemingly erroneously) set.