Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 709297 - Limit disk cache size until/unless "clear recent data" can be done async
: Limit disk cache size until/unless "clear recent data" can be done async
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Nicholas Hurley [:nwgh][:hurley]
: Patrick McManus [:mcmanus]
: 800186 (view as bug list)
Depends on: 783755
Blocks: 780922
  Show dependency treegraph
Reported: 2011-12-09 14:08 PST by Jason Duell [:jduell] (needinfo me)
Modified: 2012-10-17 18:02 PDT (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (22.17 KB, patch)
2012-08-06 13:02 PDT, Nicholas Hurley [:nwgh][:hurley]
jduell.mcbugs: feedback+
Details | Diff | Splinter Review
patch (version for checkin) (21.28 KB, patch)
2012-08-07 15:20 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
cbiesinger: superreview+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2011-12-09 14:08:35 PST
Unless we can fix bug 648232 before the next code fork, I propose we limit the disk cache for now to something smaller than 1 GB, say 400 MB.

The issue is that we're blocking while we evict all entries instead of using nsDeleteDir.

We don't have great data on how long this is blocking the browser.  We've got no direct telemetry on it (let's add some telemetry to EvictEntries() as part of this bug?).  The closest thing we have are the stats for NETWORK_DISK_CACHE_DELETEDIR (which measures how long nsDeleteDir takes to delete all the files in the cache: I'm not sure how that compares to eviction, but I'm guessing eviction can only take longer as it has bookkeeping overhead too), and we foolishly capped those at 10 sec max (I'm changing those in bug 7092862).
Shows that deleting the disk cache takes >10s around 20% of the time. Users who've selected "clear my last hour of browsing history" shouldn't have it take that long (and it's probably much longer in some cases).
Comment 1 Jason Duell [:jduell] (needinfo me) 2012-07-24 10:33:06 PDT
Note that this post from a Chrome dev also suggests that we may want to limit cache size because at some point, lookup speed deteriorates:

"beyond 320MB, the cache hit rate improvements were negligible, but the long tail of disk lookups started increasing, so performance actually degraded, and it was actually better to simply load the resource from the network instead."

We should get some measurements and consider pushing out A|B tests to see if a smaller cache size might actually be better.
Comment 2 Nicholas Hurley [:nwgh][:hurley] 2012-08-06 13:02:10 PDT
Created attachment 649369 [details] [diff] [review]

Here's a first run at taking the approach that was discussed on dev-tech-network. Try looks good; a few android tests failed unexpectedly, but those seem to be infrastructure issues (I'm re-running the failed tests, just to make sure).
Comment 3 Jason Duell [:jduell] (needinfo me) 2012-08-06 15:44:00 PDT
Comment on attachment 649369 [details] [diff] [review]

Review of attachment 649369 [details] [diff] [review]:

+r with nits fixed (and tryserver OK).

::: netwerk/cache/nsCacheService.cpp
@@ +1594,5 @@
>      return NS_OK;
>  }
> +// Runnable sent from cache thread to main thread
> +class nsSetUseOldMaxSmartSizePrefEvent: public nsRunnable

Since this event only ever turns off the old size, it might read better as "nsDisableOldMaxSmartSizePrefEvent".  Just a thought.

::: netwerk/cache/nsDiskCacheDevice.cpp
@@ +1004,5 @@
>              exists = false;
>          } else {
>              // don't gather telemetry for "corrupt cache" for new profile
>              // where cache doesn't exist (most likely case if we're here).
> +            nsCacheService::MarkStartingFresh();

Hmm, I found a bug here while looking at this: bug 780750.

Once that's fixed we can remove two out of the three MarkStarting calls here, and just use the last one (under if !exists).

I probably should have based that patch on top on this one.  But just remove the other two calls already and things will work well enough--the return rv case here is rare (and the disk cache doesn't get rebuilt, so you actually don't want to MarkFresh anyway), and the ERROR_CORRUPTED call just winds up resulting in calling MarkFresh twice.
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2012-08-06 16:37:16 PDT
Here's the try run for this patch combined with the one from bug 780750 -
Comment 5 Nicholas Hurley [:nwgh][:hurley] 2012-08-07 15:20:28 PDT
Created attachment 649832 [details] [diff] [review]
patch (version for checkin)

Here's the patch with Jason's nits fixed, and rebased on top of his patch in bug 780750, carrying forward r+

Try looks good
Comment 6 Nicholas Hurley [:nwgh][:hurley] 2012-08-07 15:50:26 PDT
Comment 7 Ed Morley [:emorley] 2012-08-08 09:31:58 PDT
Comment 8 Nicholas Hurley [:nwgh][:hurley] 2012-08-13 14:28:11 PDT
Comment on attachment 649832 [details] [diff] [review]
patch (version for checkin)

Christian, Josh mentioned I should get sr from you for this, given the scope of its change. It's already landed, but can pretty easily be backed out if you find some problem with it.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-21 18:39:45 PDT
Comment on attachment 649832 [details] [diff] [review]
patch (version for checkin)

+    bool            UseOldMaxSmartSize()        { return mUseOldMaxSmartSize; }
+    void            SetUseNewMaxSmartSize()     { mUseOldMaxSmartSize = false; }

Two thoughts on this:
- UseOldMaxSmartSize reads ambiguously, maybe rename to ShouldUseOldMaxSmartSize?
- SetUseNewMaxSmartSize - maybe give it a bool argument to allow setting it either way
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-21 18:48:38 PDT
I'm not entirely clear on why we want to use the bigger size when we are creating a new cache, but not otherwise? Or am I misreading the patch? Maybe you can point me to the mentioned dev-tech-network discussion
Comment 11 Nicholas Hurley [:nwgh][:hurley] 2012-08-22 10:06:46 PDT
The changes from comment 9 sound good, I'll take care of them in the followup to this (bug 783755).

As for your question in comment 10, I think you're misreading the patch (or I missed something glaringly obvious). Any time we start with a fresh cache (whether it's because we crashed, are being started for the very first time, the user cleared history, or something else) we will start using the 350MB cache. We're doing the resize then so we don't go thrashing people's disks when they upgrade to Firefox 17 by deleting 650MB of data (a whole CD ROM worth!). Our hope is that this will, over the course of a few releases, get us to the point where most people are using the 350MB size (due to our abysmal crash rate), and then we can just bite the bullet for the last small group of people and make the change permanent. The dev-tech-network discussion is at
Comment 12 Marc42410 2012-09-17 11:26:32 PDT
Just out of curiosity,
What happened to the hard limit of not caching a file larger than 1/8 of the cache size? 50MB was the value set for the pref(browser.cache.disk.max_entry_size), which requires the cache size to be more than 400mb. 350MB means the pref must be lowered, if I'm guessing right.
Comment 13 Jason Duell [:jduell] (needinfo me) 2012-10-11 11:47:46 PDT
*** Bug 800186 has been marked as a duplicate of this bug. ***
Comment 14 Will Fiveash 2012-10-16 17:05:53 PDT
I've limited my cache size to 350MB but when I run 'du -sh .' in the cache dir on OS X
$ du -sh .
449M	.

I find this surprising.  Note, I'm running Firefox 16.0.1.

Also, the about:cache states:

Number of entries: 	88384
Maximum storage size: 	358400 KiB
Storage in use: 	358399 KiB
Cache Directory: 	/Users/willf/Library/Caches/Firefox/Profiles/dp2ght2h.Firefox7/Cache
Comment 15 Michal Novotny (:michal) 2012-10-17 18:02:02 PDT
We round the data size up to the nearest kB, but FS blocks are usually 4kB or more, hence the difference.

Note You need to log in before you can comment on or make changes to this bug.