986179 - Rework cache2 memory pool

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

- instead of a single pool we now have two
- one for memory cache, controlled by browser.cache.memory.capacity (by default autodetected)
- other for disk entries, controlled by a new browser.cache.disk.metadata_memory_limit, defaults at 250kB
- chunks belonging to disk entries are not memory-reported ; we only count metadata size for disk entries
- disk entry's cached chunks are purged when no one keeps a ref to such an entry
- purging is now scheduled with 1000ms timer ; hence, the mPurging flag is gone
- ~CacheMemoryConsumer automatically reports 0 now
- CacheObserver::MemoryCacheCapacity() copied from netwerk/cache, just minor code fits were made

Comment 1

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=d6fab046931f

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8394416 [details] [diff] [review]
v1

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

::: netwerk/cache2/CacheEntry.cpp
@@ +809,5 @@
>  
> +  return Persistent();
> +}
> +
> +bool CacheEntry::Persistent() const

IsPersistent?

::: netwerk/cache2/CacheEntry.h
@@ +218,5 @@
>    void OnHandleClosed(CacheEntryHandle const* aHandle);
>  
>  private:
> +  friend class CacheEntryHandle;
> +  // Increments the number of handlers keeping this entry.

These are handles, not handlers. There should be: AddHandleRef, mHandlesCount, ...

::: netwerk/cache2/CacheFile.cpp
@@ +210,5 @@
>  
>    if (mMemoryOnly) {
>      MOZ_ASSERT(!aCallback);
>  
> +    mMetadata = new CacheFileMetadata(true, mKey);

Pass mMemoryOnly as the first argument.

::: netwerk/cache2/CacheStorageService.cpp
@@ +831,5 @@
>  
>    // Exchange saved size with current one.
>    aConsumer->mReportedMemoryConsumption = aCurrentMemoryConsumption;
>  
> +  bool usingDisk = !(aConsumer->mFlags & CacheMemoryConsumer::MEMORY_ONLY);

This won't work. If the CacheFile was initialized as disk file and it ended up in memory due to some IO error, it will report to a wrong pool. Or more precisely, it will report to a correct pool, but it will be registered in a wrong pool.

::: netwerk/cache2/CacheStorageService.h
@@ +53,5 @@
> +    // which of the two disk and memory pools count this consumption at.
> +    MEMORY_ONLY = 1 << 0,
> +    // Prevent reports of this consumer at all, used for disk data chunks since
> +    // we throw them away ASAP and don't want to wipe the whole pool out during
> +    // their short life.

As far as I understand this patch, this is not true. We don't release the chunks ASAP. We release all chunks from a CacheFile at once when nobody uses the CacheFile anymore, instead of releasing a particular chunk when it was written to disk and/or no stream wants to read from it anymore.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #2)
> Comment on attachment 8394416 [details] [diff] [review]
> v1
> 
> Review of attachment 8394416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +809,5 @@
> >  
> > +  return Persistent();
> > +}
> > +
> > +bool CacheEntry::Persistent() const
> 
> IsPersistent?

Maybe the naming should change for the UsingDisk method too...  That one is purposed for use by the cache service only and under its lock.

> 
> ::: netwerk/cache2/CacheEntry.h
> @@ +218,5 @@
> >    void OnHandleClosed(CacheEntryHandle const* aHandle);
> >  
> >  private:
> > +  friend class CacheEntryHandle;
> > +  // Increments the number of handlers keeping this entry.
> 
> These are handles, not handlers. There should be: AddHandleRef,
> mHandlesCount, ...

Actually best word is "consumers".

> 
> ::: netwerk/cache2/CacheFile.cpp
> @@ +210,5 @@
> >  
> >    if (mMemoryOnly) {
> >      MOZ_ASSERT(!aCallback);
> >  
> > +    mMetadata = new CacheFileMetadata(true, mKey);
> 
> Pass mMemoryOnly as the first argument.

It's true here, but as you wish.

> 
> ::: netwerk/cache2/CacheStorageService.cpp
> @@ +831,5 @@
> >  
> >    // Exchange saved size with current one.
> >    aConsumer->mReportedMemoryConsumption = aCurrentMemoryConsumption;
> >  
> > +  bool usingDisk = !(aConsumer->mFlags & CacheMemoryConsumer::MEMORY_ONLY);
> 
> This won't work. If the CacheFile was initialized as disk file and it ended
> up in memory due to some IO error, it will report to a wrong pool. Or more
> precisely, it will report to a correct pool, but it will be registered in a
> wrong pool.

I know and somewhat don't care.

To make this right is however not that hard, just means to have a new flag on CacheFile remembering the original open flag.  Would you be OK with that?

> 
> ::: netwerk/cache2/CacheStorageService.h
> @@ +53,5 @@
> > +    // which of the two disk and memory pools count this consumption at.
> > +    MEMORY_ONLY = 1 << 0,
> > +    // Prevent reports of this consumer at all, used for disk data chunks since
> > +    // we throw them away ASAP and don't want to wipe the whole pool out during
> > +    // their short life.
> 
> As far as I understand this patch, this is not true. We don't release the
> chunks ASAP. We release all chunks from a CacheFile at once when nobody uses
> the CacheFile anymore, instead of releasing a particular chunk when it was
> written to disk and/or no stream wants to read from it anymore.

You understand well.  You are concerned about the comment?  I can change ASAP for "immediately as the cache entry is not held by any consumer."  OK?

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1 -> v2 idiff

- s/handlers/handles/
- name for UsingDisk() and Persistent() adjusted
- new bool CacheFile::mOpenAsMemoryOnly flag
- fixed race condition on setting mPurgeTimer (I think this has appeared on try as a crash/assert in nsTimerImpl)

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v2 [full patch]

Comment 6

[Michal Novotny [:michal]]

Attachment: release chunks ASAP

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8400222 [details] [diff] [review]
release chunks ASAP

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

::: netwerk/cache2/CacheFile.cpp
@@ +1179,5 @@
>  
>      MOZ_ASSERT(mReady);
> +    MOZ_ASSERT((mHandle && !mMemoryOnly && !mOpeningFile) ||
> +               (!mHandle && mMemoryOnly && !mOpeningFile) ||
> +               (!mHandle && !mMemoryOnly && mOpeningFile));

some explanation would be good (up to you to add it or not).

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8400222 [details] [diff] [review]
release chunks ASAP

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

::: netwerk/cache2/CacheFile.cpp
@@ +14,5 @@
>  #include <algorithm>
>  #include "nsComponentManagerUtils.h"
>  #include "nsProxyRelease.h"
>  
> +// When CACHE_CHUNKS is defined we always cache unused chunks in mCacheChunks.

mCachedChunks

@@ +18,5 @@
> +// When CACHE_CHUNKS is defined we always cache unused chunks in mCacheChunks.
> +// When it is not defined, we always release the chunks ASAP, i.e. we cache
> +// unused chunks only when:
> +//  - CacheFile is memory-only
> +//  - CacheFile is still waiting for the handle

the file handle

@@ +794,1 @@
>      // entries from being purged.

Michal, I think this old comment of mine is now wrong.  Can you please (or me, when incorporating this patch) remove it when we are here?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v3

- incorporates the "release chunks ASAP" patch
- AddHandleRef/ReleaseHandleRef methods left
- PurgeEntryData et al removed
- MANAGEMENT level put before write to let pool purging happen before writes (i.e. not be blocked by slow pending writes)
- CacheEntry cannot be purged when its CacheFile indicates there may be some unwritten data, see CacheFile::IsWriteInProgress() and please check that I list all flags and states that may indicate any in-progress writes

Comment 10

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34be9ed3d560

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34be9ed3d560