HTTP cache v2: make disk cache smart sizing work with the new backend

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

5 years ago
No description provided.
Assignee

Updated

5 years ago
Blocks: cache2enable
Assignee

Comment 1

5 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8406935 - Flags: review?(honzab.moz)
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

5 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

5 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #8406935 - Attachment is obsolete: true
Attachment #8406935 - Flags: review?(honzab.moz)
Attachment #8407520 - Flags: review?(honzab.moz)
(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 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

5 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?
(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

5 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #8407893 - Flags: review?(honzab.moz)
Assignee

Updated

5 years ago
Attachment #8407520 - Attachment is obsolete: true
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 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-
Posted patch threadsafe refcnt fix (obsolete) — Splinter Review
This seems like a fix.  Pushing to gum.
https://tbpl.mozilla.org/?tree=Gum&rev=fcaac8e0137a
Comment on attachment 8407893 [details] [diff] [review]
patch v3

r=honzab with the "threadsafe refcnt fix" patch incorporated.
Attachment #8407893 - Flags: review- → review+
Attachment #8409063 - Attachment is patch: true
Assignee

Comment 14

5 years ago
Posted patch patch v4Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/7c7475dc1a27
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.