Closed
Bug 987829
Opened 10 years ago
Closed 10 years ago
HTTP cache v2: make disk cache smart sizing work with the new backend
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: michal, Assigned: michal)
References
Details
Attachments
(1 file, 4 obsolete files)
14.91 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Blocks: cache2enable
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8406935 -
Flags: review?(honzab.moz)
Comment 2•10 years ago
|
||
Comment on attachment 8406935 [details] [diff] [review] patch v1 Review of attachment 8406935 [details] [diff] [review]: ----------------------------------------------------------------- I didn't check locally and didn't take a look at the math. there are the first comments, please feel free to update in the meantime. ::: netwerk/cache2/CacheFileIOManager.cpp @@ +3547,5 @@ > } > } > > +// Returns default ("smart") size (in KB) of cache, given available disk space > +// (also in KB) but availKB is cachesize + available space @@ +3595,5 @@ > + return std::min<uint32_t>(maxSize, sz10MBs * 10 * 1024); > +} > + > +nsresult > +CacheFileIOManager::UpdateSmartCacheSize() comment what this do and when (who is the caller) assert IO thread (you do IO here) @@ +3612,5 @@ > + > + // Wait at least kSmartSizeUpdateInterval before recomputing smart size. > + if (!mLastSmartSizeTime.IsNull() && > + (TimeStamp::NowLoRes() - mLastSmartSizeTime).ToMilliseconds() < > + kSmartSizeUpdateInterval) { you know the story here.. static TimeDuration const = TimeDuration::FromMilliseconds() @@ +3655,5 @@ > + > +uint32_t > +CacheFileIOManager::DiskCacheCapacity() > +{ > + mozilla::MutexAutoLock lock(mCapacityLock); took a while to understand this code (comments needed! I repeat my self so many times...) mNewCapacity is threadsafe to access (automatically aligned uint32_t). please use that fact and get rid of the lock. also, mNewCapacityUpdatePending could well be stored in CacheObserver instead of having this branching in CacheFileIOManager, would that be possible? Actually it should, I do a similar dance with smart memory cache capacity. @@ +3677,5 @@ > + mNewCapacity = aCapacity; > + > + if (mNewCapacityUpdatePending) { > + // Event was already posted. > + return NS_OK; do you expect this overlap very often? @@ +3714,5 @@ > + // Smart size was disabled before the event was executed. > + LOG(("CacheFileIOManager::SetDiskCacheCapacityInternal() - ignoring call " > + "since smart size is disabled.")); > + return NS_ERROR_NOT_AVAILABLE; > + } why not checking this before posting? @@ +3717,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + > + rv = mozilla::Preferences::SetInt( > + "browser.cache.disk.capacity", mNewCapacity << 10); seems like the locking is there just to have here a consistent mNewCapacity, would be better to pass a method arg to the runnable only? note that mNewCapacity is really not a critical value.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2) > ::: netwerk/cache2/CacheFileIOManager.cpp > @@ +3547,5 @@ > > } > > } > > > > +// Returns default ("smart") size (in KB) of cache, given available disk space > > +// (also in KB) > > but availKB is cachesize + available space That's OK, space occupied by the cache is available for the cache so we should add it to the free space on the disk. > @@ +3595,5 @@ > > + return std::min<uint32_t>(maxSize, sz10MBs * 10 * 1024); > > +} > > + > > +nsresult > > +CacheFileIOManager::UpdateSmartCacheSize() > > comment what this do and when (who is the caller) > > assert IO thread (you do IO here) What's wrong with the assertion that is at the beginning of the method??? > @@ +3655,5 @@ > > + > > +uint32_t > > +CacheFileIOManager::DiskCacheCapacity() > > +{ > > + mozilla::MutexAutoLock lock(mCapacityLock); > > took a while to understand this code (comments needed! I repeat my self so > many times...) > > mNewCapacity is threadsafe to access (automatically aligned uint32_t). > please use that fact and get rid of the lock. > > also, mNewCapacityUpdatePending could well be stored in CacheObserver > instead of having this branching in CacheFileIOManager, would that be > possible? > > Actually it should, I do a similar dance with smart memory cache capacity. I don't think that CacheObserver should do it. Let's rename it to something like CachePreferenceManager and then we can add the logic there. Btw. I don't see any "similar dance" with smart memory cache capacity. > @@ +3677,5 @@ > > + mNewCapacity = aCapacity; > > + > > + if (mNewCapacityUpdatePending) { > > + // Event was already posted. > > + return NS_OK; > > do you expect this overlap very often? No. > @@ +3714,5 @@ > > + // Smart size was disabled before the event was executed. > > + LOG(("CacheFileIOManager::SetDiskCacheCapacityInternal() - ignoring call " > > + "since smart size is disabled.")); > > + return NS_ERROR_NOT_AVAILABLE; > > + } > > why not checking this before posting? It is checked before posting, but the preference can be disabled after posting the event and before the event is executed. > @@ +3717,5 @@ > > + return NS_ERROR_NOT_AVAILABLE; > > + } > > + > > + rv = mozilla::Preferences::SetInt( > > + "browser.cache.disk.capacity", mNewCapacity << 10); > > seems like the locking is there just to have here a consistent mNewCapacity, > would be better to pass a method arg to the runnable only? No, I need to have an access to it in DiskCacheCapacity() method. > note that mNewCapacity is really not a critical value. The lock is there to synchronize access to mNewCapacityUpdatePending so that we know in DiskCacheCapacity() when to return mNewCapacity and when to return value from preferences. I'll change it so that it won't need the lock.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8406935 -
Attachment is obsolete: true
Attachment #8406935 -
Flags: review?(honzab.moz)
Attachment #8407520 -
Flags: review?(honzab.moz)
Comment 5•10 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #3) > (In reply to Honza Bambas (:mayhemer) from comment #2) > > ::: netwerk/cache2/CacheFileIOManager.cpp > > @@ +3547,5 @@ > > > } > > > } > > > > > > +// Returns default ("smart") size (in KB) of cache, given available disk space > > > +// (also in KB) > > > > but availKB is cachesize + available space > > That's OK, space occupied by the cache is available for the cache so we > should add it to the free space on the disk. OK, just update the comment maybe. > > > > @@ +3595,5 @@ > > > + return std::min<uint32_t>(maxSize, sz10MBs * 10 * 1024); > > > +} > > > + > > > +nsresult > > > +CacheFileIOManager::UpdateSmartCacheSize() > > > > comment what this do and when (who is the caller) > > > > assert IO thread (you do IO here) > > What's wrong with the assertion that is at the beginning of the method??? Nothing. My overlook!!! > I don't think that CacheObserver should do it. And I think it should. Then you don't need this unnecessary new lock and another state flags. > Let's rename it to something > like CachePreferenceManager and then we can add the logic there. The name is what prevents adding the logic there? Wow! Honestly I also think of changing the name, it's not perfect. If to change, then to CachePreferences. > Btw. I > don't see any "similar dance" with smart memory cache capacity. And here it is: http://hg.mozilla.org/mozilla-central/annotate/dd50745d7f35/netwerk/cache2/CacheObserver.cpp#l205 There is also the math, up to you to move it there as well or leave in IO man (IMO observer is a better place) > > @@ +3714,5 @@ > > > + // Smart size was disabled before the event was executed. > > > + LOG(("CacheFileIOManager::SetDiskCacheCapacityInternal() - ignoring call " > > > + "since smart size is disabled.")); > > > + return NS_ERROR_NOT_AVAILABLE; > > > + } > > > > why not checking this before posting? > > It is checked before posting, but the preference can be disabled after > posting the event and before the event is executed. I think I understand now. And I don't agree with this overcomplicated approach. > > > > @@ +3717,5 @@ > > > + return NS_ERROR_NOT_AVAILABLE; > > > + } > > > + > > > + rv = mozilla::Preferences::SetInt( > > > + "browser.cache.disk.capacity", mNewCapacity << 10); > > > > seems like the locking is there just to have here a consistent mNewCapacity, > > would be better to pass a method arg to the runnable only? > > No, I need to have an access to it in DiskCacheCapacity() method. > > > > note that mNewCapacity is really not a critical value. > > The lock is there to synchronize access to mNewCapacityUpdatePending so that > we know in DiskCacheCapacity() when to return mNewCapacity and when to > return value from preferences. I'll change it so that it won't need the lock. You don't need mNewCapacityUpdatePending at all when you move it to the observer.
Comment 6•10 years ago
|
||
Comment on attachment 8407520 [details] [diff] [review] patch v2 Review of attachment 8407520 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cache2/CacheFileIOManager.cpp @@ +3624,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + > + uint32_t cacheUsage; > + rv = CacheIndex::GetCacheSize(&cacheUsage); maybe add a comment this is in kB (or rename cacheUsage to something like cacheUsageKB @@ +3693,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsresult rv; > + > + mNewCapacityUpdatePending = false; maybe drop after you actually set the pref? but there are early returns and the flag could be left false, so you need to drop it before every return. hmm.. these are the reasons I am not in favor of flags and trying to avoid them unless really necessary. I think the disk cache limit should be cached in the observer (or preferences) and the observer should also take care to store it back to prefs. also, I'm not happy you directly work with prefs here, it should all be encapsulated in the observer (or CachePreferences - or CachePrefs ?)
Attachment #8407520 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 7•10 years ago
|
||
> ::: netwerk/cache2/CacheFileIOManager.cpp > @@ +3624,5 @@ > > + return NS_ERROR_NOT_AVAILABLE; > > + } > > + > > + uint32_t cacheUsage; > > + rv = CacheIndex::GetCacheSize(&cacheUsage); > > maybe add a comment this is in kB (or rename cacheUsage to something like > cacheUsageKB This is clearly stated in CacheIndex.h and I don't think this comment must be present everywhere we use this method. > @@ +3693,5 @@ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + nsresult rv; > > + > > + mNewCapacityUpdatePending = false; > > maybe drop after you actually set the pref? No, because then it could in theory happen that UpdateSmartCacheSize() is called on a different thread at the same time and the new value would be stored in mNewCapacity after the value is saved to preferences but before mNewCapacityUpdatePending is set to false, so the new event wouldn't be posted. > also, I'm not happy you directly work with prefs here, it should all be > encapsulated in the observer (or CachePreferences - or CachePrefs ?) The difference is that the lock does not need to be used when the pref is set in CacheFileIOManager since we exactly know on what thread we set/get the preference. More generic code in CachePrefs would need to synchronize access, is this OK for you?
Comment 8•10 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #7) > > ::: netwerk/cache2/CacheFileIOManager.cpp > > @@ +3624,5 @@ > > > + return NS_ERROR_NOT_AVAILABLE; > > > + } > > > + > > > + uint32_t cacheUsage; > > > + rv = CacheIndex::GetCacheSize(&cacheUsage); > > > > maybe add a comment this is in kB (or rename cacheUsage to something like > > cacheUsageKB > > This is clearly stated in CacheIndex.h and I don't think this comment must > be present everywhere we use this method. It would help here to more easily follow the code. > > > > @@ +3693,5 @@ > > > + MOZ_ASSERT(NS_IsMainThread()); > > > + > > > + nsresult rv; > > > + > > > + mNewCapacityUpdatePending = false; > > > > maybe drop after you actually set the pref? > > No, because then it could in theory happen that UpdateSmartCacheSize() is > called on a different thread at the same time and the new value would be > stored in mNewCapacity after the value is saved to preferences but before > mNewCapacityUpdatePending is set to false, so the new event wouldn't be > posted. Let me explain my concern: - in CacheFileIOManager::DiskCacheCapacity you return either mNewCapacity or CacheObserver::DiskCacheCapacity() based on the mNewCapacityUpdatePending flag - if I understand correctly, this branching is there because CacheObserver::DiskCacheCapacity() will return the newly determined size only after mozilla::Preferences::SetInt() is called, so you need to return mNewCapacity for the moment before the pref update - so: initially, the pref is at 200MB thread 1 sets mNewCapacity = 123MB thread 1 posts the event to set the prefs on MT thread 1 sets mNewCapacityUpdatePending = true main thread execs the event, sets mNewCapacityUpdatePending = false thread X (it can be 1) calls CacheFileIOManager::DiskCacheCapacity() and gets 200MB main thread sets the pref to 123MB If you just set the new value directly on CacheObserver::sDiskCacheCapacity global var you don't need all this machinery and the code will be reliable and simple, right? Then all you need is to tell the CacheObserver to store sDiskCacheCapacity global back to prefs (when it has to persist). Remember that sDiskCacheCapacity is an aligned global uint32_t whom r/w access is atomic, you can well use this to simplify the code. > > > > also, I'm not happy you directly work with prefs here, it should all be > > encapsulated in the observer (or CachePreferences - or CachePrefs ?) > > The difference is that the lock does not need to be used when the pref is > set in CacheFileIOManager since we exactly know on what thread we set/get > the preference. More generic code in CachePrefs would need to synchronize > access, is this OK for you? You absolutely don't need any locking in CacheObserver for this, why would you?
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8407893 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•10 years ago
|
Attachment #8407520 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
Comment on attachment 8407893 [details] [diff] [review] patch v3 Review of attachment 8407893 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. r=honzab ::: netwerk/cache2/CacheFileIOManager.cpp @@ +51,5 @@ > + > +#ifdef ANDROID > +const uint32_t kMaxCacheSize = 200*1024; // 200 MB > +#else > +const uint32_t kMaxCacheSize = 350*1024; // 350 MB please add KB suffix to kMaxCacheSize to make it clear it's in kB. ::: netwerk/cache2/CacheObserver.h @@ +36,5 @@ > { return sMetadataMemoryLimit << 10; } > static uint32_t const MemoryCacheCapacity(); // result in bytes. > static uint32_t const DiskCacheCapacity() // result in bytes. > { return sDiskCacheCapacity << 10; } > + static void SetDiskCacheCapacity(uint32_t); nit: comment in what units this is expected
Attachment #8407893 -
Flags: review?(honzab.moz) → review+
Comment 11•10 years ago
|
||
Comment on attachment 8407893 [details] [diff] [review] patch v3 Looks like this patch has a problem: https://tbpl.mozilla.org/?tree=Gum&rev=5ca84a3a91b7 INFO - ==2636==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f6a9e0c5f17 sp 0x7f6a890f8930 bp 0x7f6a890f8940 T11) 10:40:37 INFO - #0 0x7f6a9e0c5f16 in operator++ /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsISupportsImpl.h:237 10:40:37 INFO - #1 0x7f6a9e0c5f16 in mozilla::net::CacheObserver::AddRef() /builds/slave/gum-l64-asan-00000000000000000/build/netwerk/cache2/CacheObserver.cpp:74 10:40:37 INFO - #2 0x7f6a9e0c6fd4 in ns_if_addref<mozilla::net::CacheObserver *> /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsISupportsUtils.h:46 10:40:37 INFO - #3 0x7f6a9e0c6fd4 in nsRunnableMethod /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsThreadUtils.h:298 10:40:37 INFO - #4 0x7f6a9e0c6fd4 in operator new /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsThreadUtils.h:383 10:40:37 INFO - #5 0x7f6a9e0c6fd4 in NS_NewRunnableMethod<mozilla::net::CacheObserver *, void (mozilla::net::CacheObserver::*)()> /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsThreadUtils.h:410 10:40:37 INFO - #6 0x7f6a9e0c6fd4 in mozilla::net::CacheObserver::SetDiskCacheCapacity(unsigned int) /builds/slave/gum-l64-asan-00000000000000000/build/netwerk/cache2/CacheObserver.cpp:295 10:40:37 INFO - #7 0x7f6a9e06bfa9 in mozilla::net::CacheFileIOManager::UpdateSmartCacheSize() /builds/slave/gum-l64-asan-00000000000000000/build/netwerk/cache2/CacheFileIOManager.cpp:3652 10:40:37 INFO - #8 0x7f6a9e067e0a in mozilla::net::CacheFileIOManager::EvictIfOverLimitInternal() /builds/slave/gum-l64-asan-00000000000000000/build/netwerk/cache2/CacheFileIOManager.cpp:2389 10:40:37 INFO - #9 0x7f6a9e074a60 in nsRunnableMethodImpl<tag_nsresult (mozilla::net::CacheFileIOManager::*)(), void, true>::Run() /builds/slave/gum-l64-asan-00000000000000000/build/obj-firefox/netwerk/cache2/../../dist/include/nsThreadUtils.h:387 10:40:37 INFO - #10 0x7f6a9e0c59ea in mozilla::net::CacheIOThread::LoopOneLevel(unsigned int) /builds/slave/gum-l64-asan-00000000000000000/build/netwerk/cache2/CacheIOThread.cpp:281 10:40:37
Attachment #8407893 -
Flags: review+ → review-
Comment 12•10 years ago
|
||
This seems like a fix. Pushing to gum. https://tbpl.mozilla.org/?tree=Gum&rev=fcaac8e0137a
Comment 13•10 years ago
|
||
Comment on attachment 8407893 [details] [diff] [review] patch v3 r=honzab with the "threadsafe refcnt fix" patch incorporated.
Attachment #8407893 -
Flags: review- → review+
Updated•10 years ago
|
Attachment #8409063 -
Attachment is patch: true
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7475dc1a27
Attachment #8407893 -
Attachment is obsolete: true
Attachment #8409063 -
Attachment is obsolete: true
Attachment #8410125 -
Flags: review+
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7c7475dc1a27
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•