Support max_entry_size prefs for disk & memory cache

RESOLVED FIXED in mozilla7

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jduell, Assigned: bjarne)

Tracking

(Blocks: 1 bug)

unspecified
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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

Comment 1

6 years ago
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
Assignee: nobody → bjarne
Blocks: 645848
Summary: Unused cache settings for object max size → Support max_entry_size prefs for disk & memory cache
(Reporter)

Updated

6 years ago
Blocks: 647391
(Assignee)

Comment 2

6 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 3

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

Comment 4

6 years ago
See nsMemoryCacheDevice::EntryIsTooBig() and how we set mSoftLimit.
(Assignee)

Comment 5

6 years ago
Created attachment 529683 [details] [diff] [review]
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...
Attachment #529683 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 6

6 years ago
Tryserver-run is ok. Awaiting review...
(Assignee)

Comment 7

6 years ago
Comment on attachment 529683 [details] [diff] [review]
Initial proposal

Changing reviewer since Jason seems to be busy.
Attachment #529683 - Flags: review?(jduell.mcbugs) → review?(michal.novotny)
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).
Attachment #529683 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 535614 [details] [diff] [review]
Updated according to comments from review

Should be better now. Passes local testing - not sent to tryserver since functionality has not really been changed.
Attachment #529683 - Attachment is obsolete: true
Attachment #535614 - Flags: review?(michal.novotny)
(Reporter)

Comment 10

6 years ago
> +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.
(Assignee)

Comment 11

6 years ago
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...
Attachment #535614 - Flags: review?(michal.novotny)
(Assignee)

Comment 13

6 years ago
Created attachment 539788 [details] [diff] [review]
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).
Attachment #535614 - Attachment is obsolete: true
Attachment #539788 - Flags: review?(michal.novotny)
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.
Attachment #539788 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 540268 [details] [diff] [review]
Fixed nits, requesting check-in
Attachment #540268 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c828b73a419a
Keywords: checkin-needed
Whiteboard: [inbound]
(Assignee)

Comment 17

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

Review of attachment 540268 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #540268 - Flags: checkin?
http://hg.mozilla.org/mozilla-central/rev/c828b73a419a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Depends on: 667593
Blocks: 715418
You need to log in before you can comment on or make changes to this bug.