Closed
Bug 645848
Opened 14 years ago
Closed 13 years ago
Enable disk cache in Fennec and set reasonable size and location
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 10
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: mfinkle, Assigned: u408661)
References
(Blocks 1 open bug)
Details
(Keywords: mobile, perf)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
We should make sure all disk cache IO is off the main thread (which I don't think is currently the case).
Reporter | ||
Comment 2•14 years ago
|
||
(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•14 years ago
|
||
(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.
Updated•14 years ago
|
Reporter | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
mfinkle on IRC said that on some/many devices, we would be limited to 6-8MB for the cache.
Comment 7•14 years ago
|
||
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•14 years ago
|
||
SD cards can be noticeably slower than internal flash memory on most devices, so that would probably reduce the performance benefit.
Reporter | ||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
Updated•14 years ago
|
Blocks: http_cache
Reporter | ||
Updated•14 years ago
|
Whiteboard: [fennec-6]
Updated•14 years ago
|
tracking-fennec: --- → 6+
Whiteboard: [fennec-6]
Reporter | ||
Updated•14 years ago
|
tracking-fennec: 6+ → 7+
Reporter | ||
Comment 12•13 years ago
|
||
Jason - Can we assign this to someone on necko?
tracking-fennec: 7+ → 8+
Reporter | ||
Updated•13 years ago
|
tracking-fennec: 8+ → +
![]() |
||
Comment 13•13 years ago
|
||
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•13 years ago
|
||
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...)
Assignee | ||
Comment 15•13 years ago
|
||
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!)
Assignee | ||
Comment 16•13 years ago
|
||
Enable disk cache and limit to 10MB
Attachment #571462 -
Flags: review?(doug.turner)
Comment 17•13 years ago
|
||
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).
Attachment #571462 -
Flags: review?(doug.turner) → review+
Updated•13 years ago
|
Keywords: checkin-needed
Comment 18•13 years ago
|
||
(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. :)
Assignee | ||
Comment 19•13 years ago
|
||
(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.
Assignee | ||
Comment 20•13 years ago
|
||
Update the commit message to have r=dougt in it. Carry forward r+
Attachment #571462 -
Attachment is obsolete: true
Attachment #571642 -
Flags: review+
Assignee | ||
Comment 21•13 years ago
|
||
(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•13 years ago
|
||
pushed to mozilla-inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/b3699e59cfd3
Comment 23•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3699e59cfd3
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•13 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•