650995 - Support max_entry_size prefs for disk & memory cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

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

Comment 1

[Jason Duell]

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

Comment 2

[Bjarne (:bjarne)]

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.

Comment 3

[Jason Duell]

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.

Comment 4

[Bjarne (:bjarne)]

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

Comment 5

[Bjarne (:bjarne)]

Attachment: Initial proposal

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

Comment 6

[Bjarne (:bjarne)]

Tryserver-run is ok. Awaiting review...

Comment 7

[Bjarne (:bjarne)]

Comment on attachment 529683 [details] [diff] [review]
Initial proposal

Changing reviewer since Jason seems to be busy.

Comment 8

[Michal Novotny [:michal]]

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

Comment 9

[Bjarne (:bjarne)]

Attachment: Updated according to comments from review

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

Comment 10

[Jason Duell]

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

Comment 11

[Bjarne (:bjarne)]

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 12

[Michal Novotny [:michal]]

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

Comment 13

[Bjarne (:bjarne)]

Attachment: Unbitrot and updated to use kilobytes

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 14

[Michal Novotny [:michal]]

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 15

[Bjarne (:bjarne)]

Attachment: Fixed nits, requesting check-in

Comment 16

[Bjarne (:bjarne)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c828b73a419a

Comment 17

[Bjarne (:bjarne)]

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

Review of attachment 540268 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c828b73a419a