Rework cache2 memory pool

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Posted patch v1 (obsolete) — Splinter Review
- 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
Attachment #8394416 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Blocks: 948566
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.
Attachment #8394416 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
Posted patch v1 -> v2 idiff (obsolete) — Splinter Review
- 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)
Attachment #8394416 - Attachment is obsolete: true
Attachment #8398565 - Flags: review?(michal.novotny)
(Assignee)

Comment 5

5 years ago
Posted patch v2 [full patch] (obsolete) — Splinter Review
(Assignee)

Updated

5 years ago
Attachment #8398565 - Attachment description: 975367-memory-pool-rework-IDIFF-R2.patch → v1 -> v2 idiff
Attachment #8398568 - Flags: review+
Attachment #8398565 - Flags: review?(michal.novotny)
Attachment #8400222 - Flags: review?(honzab.moz)
(Assignee)

Comment 7

5 years ago
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).
(Assignee)

Comment 8

5 years ago
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?
Attachment #8400222 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 9

5 years ago
Posted patch v3Splinter Review
- 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
Attachment #8398565 - Attachment is obsolete: true
Attachment #8398568 - Attachment is obsolete: true
Attachment #8401622 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Blocks: 988318
Attachment #8401622 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/34be9ed3d560
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Assignee)

Updated

5 years ago
Depends on: 993649
(Assignee)

Updated

5 years ago
No longer depends on: 993649
(Assignee)

Updated

5 years ago
Depends on: 1025913
You need to log in before you can comment on or make changes to this bug.