The default bug view has changed. See this FAQ.

Improve disk cache smart sizing for mobile

VERIFIED FIXED in Firefox 14

Status

()

Core
Networking: Cache
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

unspecified
mozilla15
x86
Android
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, firefox16 verified, blocking-fennec1.0 soft)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
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 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.
Attachment #615771 - Flags: feedback+
(Assignee)

Updated

5 years ago
Assignee: nobody → gbrown
(Assignee)

Comment 3

5 years ago
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?
Attachment #615771 - Flags: feedback?(mark.finkle)
Attachment #615771 - Flags: feedback?(blassey.bugs)
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.
Attachment #615771 - Flags: feedback?(mark.finkle) → feedback+
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
Attachment #615771 - Flags: feedback?(blassey.bugs) → feedback+
(Assignee)

Comment 6

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

Comment 7

5 years ago
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
We should probably try to get this change on Nightly and see what affect it has on various devices.
(Assignee)

Comment 9

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

Comment 10

5 years ago
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.
Attachment #615771 - Attachment is obsolete: true
Attachment #628463 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff89c3c866e0
Target Milestone: --- → mozilla15
blocking-fennec1.0: --- → ?
https://hg.mozilla.org/mozilla-central/rev/ff89c3c866e0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Soft - but would like an approval nom once it has baked for a day or two
blocking-fennec1.0: ? → soft
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)
Attachment #628463 - Flags: approval-mozilla-beta?
Attachment #628463 - Flags: approval-mozilla-aurora?
Attachment #628463 - Flags: approval-mozilla-beta?
Attachment #628463 - Flags: approval-mozilla-beta+
Attachment #628463 - Flags: approval-mozilla-aurora?
Attachment #628463 - Flags: approval-mozilla-aurora+
This was already on Aurora. Just landed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/ac6d4a9f432e
status-firefox14: --- → fixed
status-firefox15: --- → fixed
status-firefox16: --- → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
You need to log in before you can comment on or make changes to this bug.