Last Comment Bug 745340 - Improve disk cache smart sizing for mobile
: Improve disk cache smart sizing for mobile
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86 Android
: -- normal (vote)
: mozilla15
Assigned To: Geoff Brown [:gbrown] (pto May 28-June 13)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 14:41 PDT by Geoff Brown [:gbrown] (pto May 28-June 13)
Modified: 2012-07-09 02:52 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
soft


Attachments
a first patch, for discussion (2.83 KB, patch)
2012-04-17 10:25 PDT, Geoff Brown [:gbrown] (pto May 28-June 13)
jduell.mcbugs: feedback+
mark.finkle: feedback+
blassey.bugs: feedback+
Details | Diff | Review
enable disk cache smart sizing for Android (2.89 KB, patch)
2012-05-30 13:57 PDT, Geoff Brown [:gbrown] (pto May 28-June 13)
gbrown: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Geoff Brown [:gbrown] (pto May 28-June 13) 2012-04-13 14:41:22 PDT
Storage capacity varies widely across the set of devices supported by Fennec, making it difficult to determine an appropriate value for pref browser.cache.disk.capacity. (See discussion in bug 742560.)

Smart sizing - setting disk cache capacity based on available storage - seems like a natural solution. However, the current smart sizing algorithm concentrates mostly on GB of storage, while many older devices may have less than 100 MB of storage available; for these, the smart size minimum of 50 MB seems troublesome!
Comment 1 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-04-17 10:25:41 PDT
Created attachment 615771 [details] [diff] [review]
a first patch, for discussion

Here is a quick patch to get us thinking about what we want. The existing smart size algorithm is unchanged for desktop. For Android, note these changes:
 - smart sizing enabled
 - max size of 200 MB
 - min size of 10 MB
 - use 20% of the first 500 MB

For example...

50 MB or less available -> 10 MB cache
100 MB available        -> 20 MB cache
200 MB available        -> 40 MB cache
500 MB available        -> 100 MB cache
1 GB available          -> 125 MB cache
2 GB available          -> 175 MB cache
2.5 GB or more available-> 200 MB cache

Thoughts?
Comment 2 Jason Duell [:jduell] (needinfo? me) 2012-04-17 15:10:04 PDT
Comment on attachment 615771 [details] [diff] [review]
a first patch, for discussion

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

Codewise looks fine to me.  Policy-wise I don't know if 20% of the 1st 500 MB is too greedy or not, but it's definitely a good idea to lower it from desktop's 40% :)   I'll let the mobile team make that call: feel free to mark +r and land once % y'all decide on that number.
Comment 3 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-04-18 14:24:11 PDT
Comment on attachment 615771 [details] [diff] [review]
a first patch, for discussion

No rush here -- comment when you have the time. 

How do you feel about the numbers in comment #1?
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-18 14:36:29 PDT
Comment on attachment 615771 [details] [diff] [review]
a first patch, for discussion

This seems fine to me. The only part I was thinking about was the low end. Is 10MB a good low or should we use 6MB. I decided that 10MB is fine for the low end.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-04-19 08:45:25 PDT
Comment on attachment 615771 [details] [diff] [review]
a first patch, for discussion

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

::: netwerk/cache/nsCacheService.cpp
@@ +142,1 @@
>  const PRInt32 MAX_CACHE_SIZE = 1024 * 1024;     //   1 GB

why would we need to decrease the max? We should in the common case be restricted by the size of disk. But if someone makes an android device with a 20Gb disk, why not use it?

@@ +635,5 @@
> +    // device owners may be sensitive to storage footprint: Use a smaller
> +    // percentage of available space and a smaller minimum.
> +
> +    // 20% of space up to 500 MB (10 MB min)
> +    sz10MBs += NS_MAX<PRUint32>(1, avail10MBs * .2);

let's make these values constants and ifdef around the constants
Comment 6 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-04-19 11:24:34 PDT
(In reply to Brad Lassey [:blassey] from comment #5)
> why would we need to decrease the max? We should in the common case be
> restricted by the size of disk. But if someone makes an android device with
> a 20Gb disk, why not use it?

I am just being cautious: I figure there's less chance of a performance problem cropping up with 200 MB vs 1 GB -- but I haven't tested either yet. I'll get on that...
Comment 7 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-05-22 09:07:58 PDT
Some observations in light of bug 754575 and bug 757385:
 - there are space-challenged devices out there; smart-sizing might provide a better cache size for them
 - mobile devices probably have a greater likelihood of getting an invalid cache (killed due to low memory), and may take longer to delete the Cache.Trash
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-22 09:46:17 PDT
We should probably try to get this change on Nightly and see what affect it has on various devices.
Comment 9 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-05-23 12:17:44 PDT
(In reply to Geoff Brown [:gbrown] from comment #6)
> (In reply to Brad Lassey [:blassey] from comment #5)
> > why would we need to decrease the max? We should in the common case be
> > restricted by the size of disk. But if someone makes an android device with
> > a 20Gb disk, why not use it?
> 
> I am just being cautious: I figure there's less chance of a performance
> problem cropping up with 200 MB vs 1 GB -- but I haven't tested either yet.
> I'll get on that...

My biggest concern with using very-large disk caches is deleting a full cache when it is invalid. Invalid caches occur if Fennec crashes or is killed due to low memory; in these cases, the next run will detect the "dirty" cache and move it to a Cache.Trash folder; 90 seconds later, the Cache.Trash folder will be deleted on a background thread. All of that handling seems solid, but I think we still want to try to avoid very-long-running disk access.

Testing shows a tremendous range of performance across devices:

Disk cache                Time to delete
  size         Galaxy S     Galaxy Nexus    Galaxy Tab
20 MB             33 s          <1 s            <1 s
200 MB           157 s          11 s            12 s
1000 MB       18 minutes        52 s            55 s

The Galaxy S / 1000 MB case makes me nervous; I think a 200 MB max is a reasonable compromise.
Comment 10 Geoff Brown [:gbrown] (pto May 28-June 13) 2012-05-30 13:57:07 PDT
Created attachment 628463 [details] [diff] [review]
enable disk cache smart sizing for Android

Updated for bitrot only.

Android-only sizing strategy changes as shown in Comment 1.

r=jduell -- see Comment 2.

I would like to see this on Nightly, only, for now.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2012-05-30 20:01:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff89c3c866e0
Comment 12 Ed Morley [:emorley] 2012-05-31 05:53:54 PDT
https://hg.mozilla.org/mozilla-central/rev/ff89c3c866e0
Comment 13 Johnathan Nightingale [:johnath] 2012-05-31 11:34:14 PDT
Soft - but would like an approval nom once it has baked for a day or two
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-11 07:36:54 PDT
Comment on attachment 628463 [details] [diff] [review]
enable disk cache smart sizing for Android

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not really a regression, but this is a followup to bug 742560
User impact if declined: Larger than needed disk cache on some phones
Testing completed (on m-c, etc.): ~11 days on m-c
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none

We have got a few reports of large profiles. This patch could help reduce profile size a little. We would change from a fixed 25MB cache to a 10MB cache at a minimum (growing with freespace)
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-11 13:59:24 PDT
This was already on Aurora. Just landed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/ac6d4a9f432e
Comment 16 Adrian Tamas (:AdrianT) 2012-07-09 02:52:24 PDT
Verified fixed on:
Aurora 15.0a2 2012-07-08/Nightly 16.0a1 2012-07-08/Firefox Mobile 14.0b11
HTC Desire/Samsung Galaxy Nexus
Android 2.2.2/ Android 4.0.2

For HTC Desire the cache size is 10240kb while for the Google Nexus the cache size is 204800kb according to about:cache.

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