745340 - Improve disk cache smart sizing for mobile

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Geoff Brown [:gbrown]]

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

Attachment: 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]

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

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?)]

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?)]

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

(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]]

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?)]

We should probably try to get this change on Nightly and see what affect it has on various devices.

Comment 9

[Geoff Brown [:gbrown]]

(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]]

Attachment: 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?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff89c3c866e0

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff89c3c866e0

Comment 13

[Johnathan Nightingale [:johnath]]

Soft - but would like an approval nom once it has baked for a day or two

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

This was already on Aurora. Just landed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/ac6d4a9f432e

Comment 16

[Adrian Tamas (:AdrianT)]

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.