Avoid large cache evictions as disk fills by smoothing "smart" max cache size

RESOLVED FIXED in mozilla10

Status

()

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

People

(Reporter: jduell, Assigned: michal)

Tracking

unspecified
mozilla10
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(5 attachments, 1 obsolete attachment)

Created attachment 555301 [details]
Graph showing old/new smart cache sizes (Y) vs free disk space (X)

Our current smart cache algorithm has huge jumps in the max cache size--we go from using 1 GB down to 800 MB, and from 800 to 625 MB (and then to 500 MB) as free disk space shrinks.  This means users who hit these cutoffs will need to evict 125 or 175 or 200 MB from the cache at startup, which is likely to take a long time (and may hang the cache in the meantime?).   This is fairly rare, but still nice to avoid.

I've written a patch which avoids these rude large evictions by more smoothly increasing/decreasing the smart cache size.  In order to avoid doing evictions at every single startup (which seems bad for startup benchmarking if nothing else: but that's an important case), it only evicts in 10 MB chunks.  For large disk drives (>7GB free), that will only happen when free disk space has changed by more than 100 MB (or 200 MB for >25GB free) since the last startup (and the cache was full).

Attached are some graphs that show the smart size graphed against free space, using both the old algorithm and my new one.
(Reporter)

Comment 1

6 years ago
Created attachment 555302 [details]
same graph, but log-based X/Y axes (shows small sizes better)
(Reporter)

Comment 2

6 years ago
Created attachment 555304 [details] [diff] [review]
v1: implement smoother smart cache size

Michal: while writing this I noticed that we base the smart size only on remaining amount of disk space on the device.  We don't include the actual size of the cache in this calculation, which is a little silly, in that it means we'll allow the cache to be as big as "40%" of the remaining space (for very small amounts of free space), but then as we actually take up space with the cache, "40% of free space" gets smaller and we actually have to shrink the cache until it's about 29% of free space (see comment in code). 

Do we know the size of the cache this early on in the code? If we do, we could include it in the calculation and get a more stable "free" space value, independent of the cache's size.
Attachment #555304 - Flags: review?(michal.novotny)
(Reporter)

Comment 3

6 years ago
Created attachment 555305 [details] [diff] [review]
version of the patch I used to generate the graphs

This is the same patch, with some code jammed in to generate the raw data I used to generate the graphs, in case that's useful.

There's obviously no easy way to test this code automatedly.  So I figure testing the range of the old/new functions and plotting the results is as good as we need to do here.
(Assignee)

Comment 4

6 years ago
(In reply to Jason Duell (:jduell) from comment #2)
> Do we know the size of the cache this early on in the code? If we do, we
> could include it in the calculation and get a more stable "free" space
> value, independent of the cache's size.

We don't know the size at this point. The total size is stored in cache map header which is read when disk device is initialized.
(Assignee)

Comment 5

6 years ago
Comment on attachment 555304 [details] [diff] [review]
v1: implement smoother smart cache size

> +  // - Would it be better to do the calculation including the cache size? Yes,
> +  //   but also slower. So no :)

It wouldn't be slower since the total size is stored in the header and isn't calculated. We just need to postpone the call of GetSmartCacheSize() after the disk device initialization. Otherwise the patch looks good.
Attachment #555304 - Flags: review?(michal.novotny)
(Assignee)

Comment 6

6 years ago
Created attachment 560490 [details] [diff] [review]
patch v2 - adds size of current disk cache to the free space

Successfully tested on tryserver:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7eda0d31ad59
Assignee: jduell.mcbugs → michal.novotny
Attachment #555304 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #560490 - Flags: review?(bjarne)

Comment 7

6 years ago
Comment on attachment 560490 [details] [diff] [review]
patch v2 - adds size of current disk cache to the free space

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

Could you comment upon how/why current code use the 'firstRun' field/param, the corresponding pref, and how this patch affects this?

::: netwerk/cache/nsCacheService.cpp
@@ -223,5 @@
>          if (NS_FAILED(rv)) 
>              smartSizeEnabled = PR_FALSE;
>          if (smartSizeEnabled) {
>              nsCacheService::SetDiskCacheCapacity(mSmartSize);
> -            // also set on observer, in case mDiskDevice not init'd yet.

Why was this removed?
(Assignee)

Comment 8

6 years ago
(In reply to Bjarne (:bjarne) from comment #7)
> Could you comment upon how/why current code use the 'firstRun' field/param,
> the corresponding pref, and how this patch affects this?

This patch doesn't change it in any way. It just removes the param from nsSetSmartSizeEvent since it was unused.


> >          if (smartSizeEnabled) {
> >              nsCacheService::SetDiskCacheCapacity(mSmartSize);
> > -            // also set on observer, in case mDiskDevice not init'd yet.
> 
> Why was this removed?

Because now it is ensured that this code is called after initialization of mDiskDevice.

Comment 9

6 years ago
(In reply to Michal Novotny (:michal) from comment #8)
> (In reply to Bjarne (:bjarne) from comment #7)
> > Could you comment upon how/why current code use the 'firstRun' field/param,
> > the corresponding pref, and how this patch affects this?
> 
> This patch doesn't change it in any way. It just removes the param from
> nsSetSmartSizeEvent since it was unused.

Is it used anywhere now?

> > Why was this removed?
> 
> Because now it is ensured that this code is called after initialization of
> mDiskDevice.

Ok - fair enough.
(Assignee)

Comment 10

6 years ago
(In reply to Bjarne (:bjarne) from comment #9)
> > This patch doesn't change it in any way. It just removes the param from
> > nsSetSmartSizeEvent since it was unused.
> 
> Is it used anywhere now?

Yes, it is used in nsCacheProfilePrefObserver::PermittedToSmartSize() and in nsCacheProfilePrefObserver::ReadPrefs() - see variable firstSmartSizeRun. It is used to check if this is the first smart size calculation ever. And if it is, we don't allow to override the capacity that was previously set by the user.

Comment 11

6 years ago
Comment on attachment 560490 [details] [diff] [review]
patch v2 - adds size of current disk cache to the free space

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

Ok
Attachment #560490 - Flags: review?(bjarne) → review+
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/604f0f9eabda
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
(Assignee)

Comment 13

6 years ago
Created attachment 568016 [details] [diff] [review]
unbitrotted patch that was checked in
https://hg.mozilla.org/mozilla-central/rev/604f0f9eabda
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Could this be the cause of assertions in bug 697657? They have nsCacheService in most of the stacks
You need to log in before you can comment on or make changes to this bug.