Memory cache pref should be a percentage of physical RAM

VERIFIED FIXED in mozilla1.4beta

Status

()

Core
Networking: Cache
P1
normal
VERIFIED FIXED
16 years ago
3 years ago

People

(Reporter: David Hyatt, Assigned: gordon)

Tracking

({topembed+})

Trunk
mozilla1.4beta
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 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
(Reporter)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
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)

 
(Reporter)

Comment 2

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

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.
Component: Browser-General → Networking: Cache
QA Contact: doronr → tever
(Reporter)

Comment 4

16 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?
(Reporter)

Comment 5

16 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.
17%??? Geez.

OK, then, consider me convinced :)

Comment 7

16 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.
(Reporter)

Comment 8

16 years ago
Moving out to 0.9.7 to give us time to discuss options.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Comment 9

16 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?
(Reporter)

Comment 10

16 years ago
--> gordon
Assignee: hyatt → gordon
Status: ASSIGNED → NEW
(Assignee)

Updated

16 years ago
Blocks: 71668
(Assignee)

Comment 11

16 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.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

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

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

Comment 15

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

Comment 16

16 years ago
excellent suggestion... i very much agree :)
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Updated

16 years ago
Priority: -- → P2
(Assignee)

Comment 17

16 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.
OS: Windows 2000 → All
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

16 years ago
we should probably incorporate upper and lower bounds so as to ensure that the
memory cache size is always reasonable.
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0

Comment 20

16 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.
Target Milestone: mozilla1.0 → mozilla1.2

Comment 21

15 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. :-)
Hardware: PC → All

Updated

15 years ago
Blocks: 109415

Comment 22

15 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.
Keywords: topembed

Updated

15 years ago
Keywords: topembed → topembed+
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.
Depends on: 194619
(Assignee)

Comment 24

15 years ago
*** Bug 194619 has been marked as a duplicate of this bug. ***
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.
Blocks: 192762
(Assignee)

Updated

14 years ago
Priority: P2 → P1
Target Milestone: mozilla1.2alpha → mozilla1.4beta
(Assignee)

Comment 26

14 years ago
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
(Assignee)

Comment 27

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

Updated

14 years ago
Attachment #121023 - Flags: review?(darin)

Comment 28

14 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));
       

(Assignee)

Comment 29

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

Comment 30

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

Comment 31

14 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?
Why not use PR_CeilingLog2 instead of natural logarithms?

/be
(Assignee)

Comment 33

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

Comment 34

14 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?
(Assignee)

Comment 35

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

Comment 36

14 years ago
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.
Attachment #121023 - Attachment is obsolete: true
(Assignee)

Comment 37

14 years ago
Created attachment 121333 [details] [diff] [review]
version 3

more tweaks.
(Assignee)

Updated

14 years ago
Attachment #121306 - Attachment is obsolete: true
(Assignee)

Comment 38

14 years ago
Created attachment 121340 [details] [diff] [review]
v4 - should work on winows now...
Attachment #121333 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #121340 - Flags: superreview?(darin)
Attachment #121340 - Flags: review?(bryner)
(Assignee)

Comment 39

14 years ago
Created attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows
Attachment #121340 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #121367 - Flags: superreview?(darin)
Attachment #121367 - Flags: review?(bryner)
(Assignee)

Updated

14 years ago
Attachment #121340 - Flags: superreview?(darin)
Attachment #121340 - Flags: review?(bryner)
(Assignee)

Comment 40

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

r=saari
Attachment #121367 - Flags: review?(bryner) → review+
(Assignee)

Comment 41

14 years ago
I need to add an #include for MacOSX.

Comment 42

14 years ago
Comment on attachment 121367 [details] [diff] [review]
v5 - fixes two minor typos that break windows
Attachment #121367 - Flags: superreview?(darin) → superreview+
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

14 years ago
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.

Updated

14 years ago
Attachment #121426 - Flags: approval1.4b?

Comment 45

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

a=mkaply for checkin to 1.4b
Attachment #121426 - Flags: approval1.4b? → approval1.4b+

Comment 46

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

Comment 47

14 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.
Flags: blocking1.4b?
QA Contact: tever → cacheqa
Ben, this is already checked in....

Comment 49

14 years ago
You mean you did the checkin in #43?

(why is the status ASSIGNED?)

Updated

14 years ago
Flags: blocking1.4b?
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.
(Assignee)

Comment 51

14 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 52

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

Comment 53

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

clearing obsolete review request.
Attachment #121023 - Flags: review?(darin)

Comment 54

14 years ago
see bug 105344 for some ideas for a refinement of this algorithm
Jo: Is that the bug number you really meant to mention? (Bug 105344 is /this bug/)

Comment 56

14 years ago
Ah you're right. It's bug 204164.

Comment 57

14 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

Status: RESOLVED → VERIFIED
(Assignee)

Comment 58

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

Comment 59

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

Comment 60

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

Comment 61

14 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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
> 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.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 63

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

Status: RESOLVED → VERIFIED

Updated

9 years ago
Flags: in-testsuite+
Manuel: where's the automated testcase, since you marked this as in-testsuite+?  Was that perhaps unintentional?
Clearing in-testsuite+ flag that truquito_2m@hotmail.com has (seemingly erroneously) set.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.