645848 - Enable disk cache in Fennec and set reasonable size and location

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We should consider using the disk cache to improve page loading. We should also think about how big to make the cache and where (SD card) the cache should live.

Comment 1

[Alon Zakai (:azakai)]

We should make sure all disk cache IO is off the main thread (which I don't think is currently the case).

Comment 2

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

(In reply to comment #1)
> We should make sure all disk cache IO is off the main thread (which I don't
> think is currently the case).

I thought bug 513008 fixed that

Comment 3

[Alon Zakai (:azakai)]

(In reply to comment #2)
> (In reply to comment #1)
> > We should make sure all disk cache IO is off the main thread (which I don't
> > think is currently the case).
> 
> I thought bug 513008 fixed that

For reads and writes yes, but I think there are places where files are Open()ed on the main thread, from a quick look through the code. I am not sure yet though.

Comment 4

[Doug Turner (:dougt)]

bug 549767 still lives.

Comment 5

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

I did some initial Tp tests with a 50MB disk cache stored with the profile, not sdcard:

Nexus One:
  wifi:             3687ms
  wifi with cache:  3591ms

  3g:               5188ms
  3g with cache:    4484ms

Galaxy Tab:
  wifi:             3775ms
  wifi with cache:  4800ms

  3g:               5245ms
  3g with cache:    5907ms

The Nexus One shows a nice improvement, especially for 3g, while the Galaxy Tab is always a regression with cache. The Galaxy Tab has a horrible file system, so the results are not surprising.

If we decide to move forward with disk cache, we'll likely have to black list some phones at runtime.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

mfinkle on IRC said that on some/many devices, we would be limited to 6-8MB for the cache.

Comment 7

[Doug Turner (:dougt)]

more so, we'd probably only want to ever cache when there is a sdcard present and available (we can enable and set parent directory at runtime)

Comment 8

[Matt Brubeck (:mbrubeck)]

SD cards can be noticeably slower than internal flash memory on most devices, so that would probably reduce the performance benefit.

Comment 9

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

I can do some testing on smaller (6-8 MB) caches and see if we still get the Tp win. I'd be OK with using a 6-8 MB cache in the main memory if we still had decent wins.

Comment 10

[Doug Turner (:dougt)]

matt - reading from the sdcard is going to be faster than a network request over edge.  but, yes... slower media will decrease the performance improvement.

mark - a 6mb cache is going to be blown away very very quick.

Comment 11

[Jason Duell]

Adding a couple bugs as dependencies that may make things better on mobile.  None of them would actually block turning the cache on, though.

Also cc-ing Bjarne and Michal, who are the people we've assigned to making sure HTTP caching works as well as it can for mobile.

Comment 12

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

Jason - Can we assign this to someone on necko?

Comment 13

[Geoff Brown [:gbrown] (pto Jul 1-19)]

I talked with Josh about this bug at the All Hands. As I understand it, he thinks a solution is now 4 - 6 weeks away.

Comment 14

[Bjarne (:bjarne)]

Results from the cache-microbenchmarks (https://wiki.mozilla.org/Necko/MobileCache/MicroBenchmarks) as well as running Tp4M with a realistic cache-config (https://wiki.mozilla.org/Necko/MobileCache/Tp4m) seems to indicate that disk-caching is not a problem for Fennec anymore.

Mark/Doug: In your opinion, what is left to figure out/decide/evaluate before we can just enable disk-caching in its existing form? I have no clue about which cache-sizes we can afford on mobile, so somebody must provide guidelines here. I have also no clue about SD cards vs flash (or other media for that matter) and their locations, so I'll need guidelines here as well. (The most efficient way to move forward might be to re-assign this to someone in the mobile team to wrap up these details, since they are pretty specific to the mobile platform anyway...)

Comment 15

[u408661]

FWIW, my tests seem to indicate that microsd vs. flash isn't a big enough difference to matter. I'm talking with the mobile guys on Friday, so we can probably discuss the other things then. All told, though, it looks like we're getting close to having something that's viable for mobile (hooray!)

Comment 16

[u408661]

Attachment: patch

Enable disk cache and limit to 10MB

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 571462 [details] [diff] [review]
patch

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

please file follow up to add telemetry data.

From email:

I've got the recommendations for the size of the disk cache for fennec, I'm sticking with my 10MB suggestion from our meeting last week. It seems to get us at least a slight performance bump over the 6MB size in most of my tests, and is comparable with the competition (it's what Opera Mobile uses).

You had asked about memory overhead of the disk cache, as well. These are calculations based on the data structures mapped out at https://wiki.mozilla.org/images/3/3e/Disk_cache_memory.png

Minimum overhead (up to 512 entries in cache, none in use): 50k
Maximum overhead (none in use, I didn't bother calculating the number of entries): 32MB (this number increases with the number of possible entries in the disk cache)

Before you get afraid of that 32MB number, in reality this is significantly lower. On all my desktop machines, I have 1GB disk caches that are running at capacity. The number of entries I have in the cache makes the actual overhead between 512k and 1M (up to approximately 32000 entries can be done in 512k, and up to approximately 64000 entries in 1M). I haven't looked at the numbers on mobile, but a 10MB disk cache isn't going to hold nearly that many entries (based on it taking between 20000 and 45000 entries to fill a 1G disk cache), so we should be fine on the memory overhead there.

In addition to the pure overhead above, each in use entry (either for reading or writing) will take approximately 250B + metadata size (which varies), and possibly + data size (if it's in a block file instead of a standalone file).

Comment 18

[Bjarne (:bjarne)]

(In reply to Doug Turner (:dougt) from comment #17)
> please file follow up to add telemetry data.

Bug #697074 and bug #697072 may be of interest.

> Minimum overhead (up to 512 entries in cache, none in use): 50k
> Maximum overhead (none in use, I didn't bother calculating the number of
> entries): 32MB (this number increases with the number of possible entries in
> the disk cache)

I'm a little confused by your numbers. Assuming we talk about the disk-cache map (which is in memory even if no entries are used) my understanding is as follows (Michal or anyone else, please correct me if any of this is wrong):

- Each map-entry is a nsDiskCacheRecord with 4 integers. See http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheMap.h#118  The datastructures organizing them ("buckets") are simple arrays and adds very little overhead. I.e. the map uses 16 bytes for each resource in the disk-cache (I assume an integer is also 4 bytes on mobile?).

- The disk-cache works with entries rounded up to nearest Kb and stores data and metadata for a resource as separate entries in the cache. This means that for any resource in the disk-cache, we use at least 2Kb of cache-space: 1Kb for the data + 1Kb for the metadata. (Note that I'm not defending this policy but rather states how it works.)

From the above, a 10Mb disk-cache holds maximum 5k resources, which needs 80Kb for the cache-map. With a 1Gb disk-cache we can store maximum 512K resources, giving us a max memory-usage of 8Mb for the cache-map.

IMO the conclusion to not worry about memory overhead for the cache-map is fine, but the numbers were a little confusing...  The size of the disk-cache may even be increased if memory-overhead is a reason for keeping it small. :)

Comment 19

[u408661]

(In reply to Bjarne (:bjarne) from comment #18)
> (In reply to Doug Turner (:dougt) from comment #17)
> > please file follow up to add telemetry data.
> 
> Bug #697074 and bug #697072 may be of interest.
> 
> > Minimum overhead (up to 512 entries in cache, none in use): 50k
> > Maximum overhead (none in use, I didn't bother calculating the number of
> > entries): 32MB (this number increases with the number of possible entries in
> > the disk cache)
> 
> I'm a little confused by your numbers. Assuming we talk about the disk-cache
> map (which is in memory even if no entries are used) my understanding is as
> follows (Michal or anyone else, please correct me if any of this is wrong):

I was using the records for my calculation, and was pretty sure I'd overestimated, but that I still had an upper bound. If you look at nsDiskCacheMap::NotifyCapacityChange, you'll see that we bound the number of records to 32MB worth of memory. Thanks for the extended details, though, it makes me (and likely others) feel even better about the memory usage.

Comment 20

[u408661]

Attachment: patch w/updated commit message

Update the commit message to have r=dougt in it. Carry forward r+

Comment 21

[u408661]

(In reply to Doug Turner (:dougt) from comment #17)
> please file follow up to add telemetry data.

Bug 699409 and bug 699410 have been filed

Comment 22

[Josh Aas]

pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/b3699e59cfd3

Comment 23

[Ed Morley [:emorley]]

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