We've got DISK_CACHE_MAX_ENTRY_SIZE_PREF and MEMORY_CACHE_MAX_ENTRY_SIZE_PREF in nsCacheService.cpp, but don't ever use them. Instead we hard-code the disk cache limit as kMaxDataFileSize in nsDiskCacheMap.cpp, and presumably do something similar for the memory cache somewhere. We should either rip out the prefs, or honor them. (It might be nice to have them: it would make it easier to fiddle around with them--would have been useful for testing cache on mobile).

Now that I think about it, we're almost definitely going to want to allow mobile to set prefs for max cache entry size for both disk and memory cache. Otherwise for the disk cache we'll just default to storing things up to 1/8th of the cache (not sure if the memory cache works the same way), which may be bigger than we want in practice. So this should rise in priority, since it's both simple and something I think mobile may want ASAP (they're not using the disk cache yet, but they are using memory, and tweaking max size might be useful) Here are the (as yet ignored) perf names, BTW: browser.cache.disk.max_entry_size browser.cache.memory.max_entry_size

Ok - I started on this. A few initial design-decisions 1) changing a pref takes effect *after* the change, i.e. we don't purge the cache to enforce a new (lower) limit 2) the existing limits (1/8 of disk-cache and 90% of mem-cache) is applied in addition to the new prefs 3) the value -1 means unlimited (i.e. fallback to original behaviour) Let me know if any of these feels unreasonable.

Those all sound like fine limits, except that we currently (?) allow an object to take up 90% of the RAM cache. That seems way too big. Email me a link to the code where that's set--I may open a separate bug about that.

See nsMemoryCacheDevice::EntryIsTooBig() and how we set mSoftLimit.

This is a first implementation for disk- and memory-cache, including a test. It's not handling the offline-cache at all and it implements the decisions from comment #2. Pushed to tryserver...

Tryserver-run is ok. Awaiting review...

Comment on attachment 529683 [details] [diff] [review] Initial proposal Changing reviewer since Jason seems to be busy.

Comment on attachment 529683 [details] [diff] [review] Initial proposal > +pref("browser.cache.disk.max_entry_size", 20000); ... > +pref("browser.cache.disk.max_entry_size", 5000); This patch should only allow changing the limits. It shouldn't change the default values. Please change them both to 5MB. And the limit is in bytes and not in KiB, so the values here should be 5242880. > +pref("browser.cache.memory.max_entry_size", 5000); KiB instead of bytes again. > +nsCacheService::SetDiskCacheMaxEntrySize(PRInt32 capacity) "maxSize" or "limit" would be probably better name than "capacity" > +nsCacheService::SetMemoryCacheMaxEntrySize(PRInt32 capacity) the same as above > - return entrySize > kMaxDataFileSize > - || entrySize > (static_cast<PRInt64>(mCacheCapacity) * 1024 / 8); Remove definition of kMaxDataFileSize. > + , mMaxEntrySize(-1) // -1 means "no limit" You need to read the pref value somewhere. The code in nsCacheProfilePrefObserver::Observe() sets the values only after a change, so both limits (disk and memory) are always -1 after FF startup. > diff -r 7528b2718827 netwerk/test/unit/test_bug650955.js > --- /dev/null Thu Jan 01 00:00:00 1970 +0000 > +++ b/netwerk/test/unit/test_bug650955.js Tue May 03 13:46:30 2011 +0200 Add the test to the manifest (netwerk/test/unit/xpcshell.ini).

Should be better now. Passes local testing - not sent to tryserver since functionality has not really been changed.

> +pref("browser.cache.disk.max_entry_size", 5000); > This patch should only allow changing the limits. It shouldn't change the > default values. Please change them both to 5MB. And the limit is in bytes and > not in KiB, so the values here should be 5242880. Let's not use bytes for the pref! No one wants that level of control. I'm tempted to use MB, but I guess we should probably use KB instead for now (fennec might want a smaller value than 1 MB). Let's change the names of the prefs to "max_entry_kilobtyes" to reflect that, and maybe change the names of the parameters to SetMemoryCacheMaxEntrySize, etc to "capacityKB""? Let me know if anyone thinks this is a bad idea. But most of our cache prefs are already in KB, right? Seems good to be consistent.

I don't see the big gain in changing all numbers from bytes to to KB, but have no principal problems with doing so - awaiting comments from reviewer. Test must be changed though, but that is handleable.

Comment on attachment 535614 [details] [diff] [review] Updated according to comments from review > + (void) branch->GetIntPref(DISK_CACHE_MAX_ENTRY_SIZE_PREF, > + &mDiskCacheMaxEntrySize) You should check for values below -1, e.g. mDiskCacheMaxEntrySize = PR_MAX(-1, mDiskCacheMaxEntrySize); And of course do the same with mMemoryCacheMaxEntrySize. > PRInt32 mDiskCacheCapacity; // in kilobytes > + PRInt32 mDiskCacheMaxEntrySize; // in kilobytes ... > PRInt32 mMemoryCacheCapacity; // in kilobytes > + PRInt32 mMemoryCacheMaxEntrySize; // in kilobytes Wrong indentation. And the values are in bytes :) So maybe it makes sense to change it to KiB to be more consistent...

Upd according to latest comments. Test had to be changed slightly to use larger data (since we now specify max in kilobytes as opposed to bytes).

Comment on attachment 539788 [details] [diff] [review] Unbitrot and updated to use kilobytes > + mDiskCacheMaxEntrySize = PR_MAX(-1, newMaxSize); All PR_MAX/MIN in this code were replaced with NS_MAX/MIN (see bug #645398). > + if (maxSizeInKilobytes > 0) > + mMaxEntrySize = maxSizeInKilobytes * 1024; > + else > + mMaxEntrySize = -1; This makes -1 from 0. I think there is no reason to not allow setting the limit to 0. > +++ b/netwerk/test/unit/test_bug650955.js Please add a brief description of the test at the beginning of the file.

Comment on attachment 540268 [details] [diff] [review] Fixed nits, requesting check-in Review of attachment 540268 [details] [diff] [review]: -----------------------------------------------------------------