913808 - HTTP cache v2: evict disk files when cache limit is reached

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[IU]

Attachment: data is being cached beyond set cache limit

I assume what I'm experiencing, as illustrated in the attache screenshot, will be fixed by this bug.

Comment 2

[Honza Bambas (:mayhemer)]

Yes, and also the displayed 700+ MB of usage might also be wrong, in actuality it can be more.

Deleting the cache manually via Clear Recent History works.

Comment 3

[[e-mail]]

the new cache cost more disk space,since it's one file per entry(small files are taking up 4KB,though most will be small). Is there any plan to change that?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to zhoubcfan from comment #3)
> the new cache cost more disk space,since it's one file per entry(small files
> are taking up 4KB,though most will be small). Is there any plan to change
> that?

So far, no.  We want the file system handle this it self.  E.g. NTFS stores small files into the directory entry.

Comment 5

[Grant]

Hi,

My Firefox wouldn't load any web pages for me today - i checked my cache2 folder and it was around 5GB and had hit exactly 1 million files.

Just giving some feedback on when it does fill up as much as it can.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Grant from comment #5)
> Hi,
> 
> My Firefox wouldn't load any web pages for me today - i checked my cache2
> folder and it was around 5GB and had hit exactly 1 million files.
> 
> Just giving some feedback on when it does fill up as much as it can.

What system are you on?  My cache is at .8G with ~66k files, Win7.  That is quite a different ratio..

Comment 7

[Grant]

Attachment: A 6 day old cache folder

Here's my cache2 folder which is 6 days old - already at over 100,000 items.

I'm on Windows 8.1, NTFS

Comment 8

[John Doe]

Mine grew to 4.8GB but i deleted it yesterday. 
Now it's at 666 MB with 4,944 Files.

Windows 7 64bit

Comment 9

[Michal Novotny [:michal]]

Attachment: WIP patch

known issues
- cache limit is hardcoded (50MB) and not read from preferences
- entries are doomed at CacheFileIOManager level, there is missing a notification to CacheStorageService and CacheEntry is not removed from its hashtables

Comment 10

[Michal Novotny [:michal]]

Attachment: WIP patch - remove keyIsHash option

Comment 11

[Michal Novotny [:michal]]

Attachment: WIP patch - notify CacheServiceStorage about doomed entries

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8355095 [details] [diff] [review]
WIP patch - notify CacheServiceStorage about doomed entries

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

::: netwerk/cache2/CacheEntry.cpp
@@ +814,5 @@
>    // 2. this check is made also only under the service lock
>    return mHandlersCount > 0;
>  }
>  
> +bool CacheEntry::FileIsDoomed()

IsFileDoomed?

::: netwerk/cache2/CacheFile.cpp
@@ +1493,5 @@
> +CacheFile::IsDoomed()
> +{
> +  CacheFileAutoLock lock(this);
> +
> +  if (!mHandle) return false;

return on a new line
shouldn't this return true when there is no handle? (I don't know the code that well, so maybe this is ok).

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +1659,5 @@
>    aHandle->mIsDoomed = true;
> +
> +  if (!aHandle->IsSpecialFile()) {
> +    nsRefPtr<CacheStorageService> css = CacheStorageService::Self();
> +    if (css) {

for ambiguity with CSS please use |service| or |storageService| as local name.

@@ +1665,5 @@
> +      nsCOMPtr<nsILoadContextInfo> info;
> +      rv = CacheFileUtils::ParseKey(aHandle->Key(), getter_AddRefs(info), &url);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));
> +      if (NS_SUCCEEDED(rv)) {
> +        css->CacheFileDoomed(info, url);

isn't this actually calling back to the service where already a new entry can be stored in the hashtables?

I mean in case when we Doom() from the top level, we end up here and call back where the new (replaced) cache entry is already up, or am I wrong?

::: netwerk/cache2/CacheFileUtils.cpp
@@ +78,5 @@
> +  }
> +
> +  if (aInfo) {
> +    *aInfo = GetLoadContextInfo(isPrivate, appId, isInBrowser, isAnonymous);
> +    NS_ADDREF(*aInfo);

can you please go through a nsCOMPtr and forget(aInfo) ?

::: netwerk/cache2/CacheFileUtils.h
@@ +15,5 @@
> +namespace CacheFileUtils {
> +
> +nsresult ParseKey(const nsACString &aKey,
> +                  nsILoadContextInfo **aInfo,
> +                  nsACString *aURL);

The "Create Key" function should be either moved here or this ParseKey function should be close to the creating key routine.

::: netwerk/cache2/CacheStorageService.cpp
@@ +1215,5 @@
> +    if (entryExists && entry->FileIsDoomed()) {
> +      entries->Remove(entryKey);
> +      entry->DoomAlreadyRemoved();
> +      entryExists = false;
> +      entry = nullptr;

Just set aReplace = true, you are merely duplicating the code.  And don't do this check when aReplace is already set, you will save some locking.

@@ +1493,5 @@
> +    RemoveEntryIfFileDoomed(aURL, entries);
> +
> +  AppendMemoryStorageID(contextKey);
> +  if (sGlobalEntryTables->Get(contextKey, &entries))
> +    RemoveEntryIfFileDoomed(aURL, entries);

I'm thinking of using PurgeAndDoom (called out of the service lock) on the entry instead (note: you will always find the entry in sGlobalEntryTables->Get(contextKey) where context key is the non-memory storage id).  PurgeAndDoom takes care for all what is needed and actually duplicates this code.  I just wonder why there's MOZ_ASSERT(CacheStorageService::IsOnManagementThread()); in CacheEntry::PurgeAndDoom().  I think it's not needed since RemoveEntry(this) is thread safe and DoomAlreadyReamoved as well. Probably only a leftover.  Feel free to experiment with removing that assertion.

but globally, I'm not sure we should do this at all!  when the entry is demanded via AddStorageEntry it will remove/replace itself in that method correctly.  see other comments in the manager.  I think this can cause serious problems.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8355094 [details] [diff] [review]
WIP patch - remove keyIsHash option

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

Hmm.. so this effectively breaks the "Clear All" function for the cache... how are you going to alter it?

Comment 14

[Michal Novotny [:michal]]

Attachment: WIP patch - remove keyIsHash option v2

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8355094 [details] [diff] [review]
> WIP patch - remove keyIsHash option
> 
> Review of attachment 8355094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm.. so this effectively breaks the "Clear All" function for the cache...
> how are you going to alter it?

AFAICS, this code was used only by CacheFilesDeletor::DeleteOverLimit() which was never called. The new patch removes the unused code instead of commenting it out.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #14)
> AFAICS, this code was used only by CacheFilesDeletor::DeleteOverLimit()
> which was never called. The new patch removes the unused code instead of
> commenting it out.

In the new patch (remove keyIsHash option v2) it seems correct.

Comment 16

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> @@ +1665,5 @@
> > +      nsCOMPtr<nsILoadContextInfo> info;
> > +      rv = CacheFileUtils::ParseKey(aHandle->Key(), getter_AddRefs(info), &url);
> > +      MOZ_ASSERT(NS_SUCCEEDED(rv));
> > +      if (NS_SUCCEEDED(rv)) {
> > +        css->CacheFileDoomed(info, url);
> 
> isn't this actually calling back to the service where already a new entry
> can be stored in the hashtables?
> 
> I mean in case when we Doom() from the top level, we end up here and call
> back where the new (replaced) cache entry is already up, or am I wrong?

Yes, but we won't do anything with the new entry since CacheFileHandle doesn't exist yet for the new CacheEntry. If it already exists, it isn't doomed. And if it is doomed, it should be removed even if it is a newer CacheEntry that replaced the CacheEntry which used the CacheFileHandle doomed on IO thread. See that CacheStorageService::CacheFileDoomed() removes only entries when CacheEntry::FileIsDoomed() returns true.


> @@ +1493,5 @@
> > +    RemoveEntryIfFileDoomed(aURL, entries);
> > +
> > +  AppendMemoryStorageID(contextKey);
> > +  if (sGlobalEntryTables->Get(contextKey, &entries))
> > +    RemoveEntryIfFileDoomed(aURL, entries);
> 
> I'm thinking of using PurgeAndDoom (called out of the service lock) on the
> entry instead (note: you will always find the entry in
> sGlobalEntryTables->Get(contextKey) where context key is the non-memory
> storage id).  PurgeAndDoom takes care for all what is needed and actually
> duplicates this code.  I just wonder why there's
> MOZ_ASSERT(CacheStorageService::IsOnManagementThread()); in
> CacheEntry::PurgeAndDoom().  I think it's not needed since RemoveEntry(this)
> is thread safe and DoomAlreadyReamoved as well. Probably only a leftover. 
> Feel free to experiment with removing that assertion.
> 
> but globally, I'm not sure we should do this at all!  when the entry is
> demanded via AddStorageEntry it will remove/replace itself in that method
> correctly.  see other comments in the manager.  I think this can cause
> serious problems.

I'm not sure you fully understand the problem that this patch tries to solve. When cache reaches its size limit and we need to evict some entry, CacheIndex provides a hash of some entry. If the entry is unused (there is no CacheFileHandle for this entry) then we can simply delete the entry file. But if it is used, we must doom it. File is moved to doomed directory and won't be deleted until the handle is released (i.e. until corresponding CacheFile as well as CacheEntry is released).

Without this patch, CacheEntry doesn't even know that the entry file was doomed, so asyncOpenURI() would return such entry. The change in CacheStorageService::AddStorageEntry() should solve this problem.

But when nobody tries to open such entry, the CacheEntry stays cached in hashtables. I.e. it still references the CacheFile which still references the CacheFileHandle and the entry file stays on disk in doomed directory. CacheStorageService::CacheFileDoomed() ensures that we don't keep such zombies in the hashtable.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #12)

Got it.  Yes, the entry is removed only when the file is actually doomed.  So it's correct.  Thanks for clearing this.


> I'm thinking of using PurgeAndDoom (called out of the service lock) on the
> entry instead (note: you will always find the entry in
> sGlobalEntryTables->Get(contextKey) where context key is the non-memory
> storage id).  PurgeAndDoom takes care for all what is needed and actually
> duplicates this code.  I just wonder why there's
> MOZ_ASSERT(CacheStorageService::IsOnManagementThread()); in
> CacheEntry::PurgeAndDoom().  I think it's not needed since RemoveEntry(this)
> is thread safe and DoomAlreadyReamoved as well. Probably only a leftover. 
> Feel free to experiment with removing that assertion.

This still applies if you don't see a problem with it.  Of course, PurgeAndDoom call conditioned by entry->IsFileDoomed().

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 8355095 [details] [diff] [review]
WIP patch - notify CacheServiceStorage about doomed entries

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

::: netwerk/cache2/CacheEntry.cpp
@@ +1434,5 @@
> +      nsresult rv = mFile->Doom(mDoomCallback ? this : nullptr);
> +      if (NS_SUCCEEDED(rv)) {
> +        LOG(("  file doomed"));
> +        return;
> +      }

IMO this should be handled in the file.  mFile->Doom() should just return e.g. NS_ERROR_FILE_NOT_FOUND when already doomed.  Or return NS_OK and also NS_OK via the callback.

Comment 19

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #17)
> > I'm thinking of using PurgeAndDoom (called out of the service lock) on the
> > entry instead (note: you will always find the entry in
> > sGlobalEntryTables->Get(contextKey) where context key is the non-memory
> > storage id).  PurgeAndDoom takes care for all what is needed and actually
> > duplicates this code.  I just wonder why there's
> > MOZ_ASSERT(CacheStorageService::IsOnManagementThread()); in
> > CacheEntry::PurgeAndDoom().  I think it's not needed since RemoveEntry(this)
> > is thread safe and DoomAlreadyReamoved as well. Probably only a leftover. 
> > Feel free to experiment with removing that assertion.
> 
> This still applies if you don't see a problem with it.  Of course,
> PurgeAndDoom call conditioned by entry->IsFileDoomed().

OK, so I need to find an entry under the service lock in CacheStorageService::CacheFileDoomed() and check whether entry->IsFileDoomed() == true. Then I need to release the lock and call entry->PurgeAndDoom() which grabs the lock again and searches the hashtables again. Do we really want this?

BTW, there could be a call to CacheStorageService::AddStorageEntry() on other thread after the lock is released in CacheStorageService::CacheFileDoomed() and before the lock is acquired in CacheEntry::PurgeAndDoom(). In this case we would doom a new entry that replaced the one we checked in CacheStorageService::CacheFileDoomed().

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #19)
> (In reply to Honza Bambas (:mayhemer) from comment #17)
> OK, so I need to find an entry under the service lock in
> CacheStorageService::CacheFileDoomed() and check whether
> entry->IsFileDoomed() == true. 

Yes, probably call IsFileDoomed outside the lock

> Then I need to release the lock and call
> entry->PurgeAndDoom() which grabs the lock again and searches the hashtables
> again. Do we really want this?

Yes.

> 
> BTW, there could be a call to CacheStorageService::AddStorageEntry() on
> other thread after the lock is released in
> CacheStorageService::CacheFileDoomed() and before the lock is acquired in
> CacheEntry::PurgeAndDoom(). In this case we would doom a new entry that
> replaced the one we checked in CacheStorageService::CacheFileDoomed().

No.  You have the entry in hands, you don't go via hashtable lookup again, so you are dooming the right entry.

Purpose is to have the code stable.  PurgeAndDoom is the only way to for you to remove an entry correctly.  I don't want it's code duplicated.

Comment 21

[Michal Novotny [:michal]]

Attachment: WIP patch - notify CacheServiceStorage about doomed entries v2

Comment 22

[Michal Novotny [:michal]]

Attachment: WIP patch v2

Fixed FrecencyComparator.

Comment 23

[Michal Novotny [:michal]]

Attachment: patch v1

This is a merged patch from all three WIP patches. All reviewers comments were addressed. It should be applied after patches from bugs 923016 and 913820.

Comment 24

[Michal Novotny [:michal]]

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

Comment 25

[Michal Novotny [:michal]]

Attachment: patch v2

Fixed crash on nullptr in CacheFile::Doom().

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

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8369427 [details] [diff] [review]
patch v1

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

r+ with comments addressed or discussed.

::: netwerk/cache2/CacheEntry.cpp
@@ +836,5 @@
>  }
>  
> +bool CacheEntry::IsFileDoomed()
> +{
> +  mozilla::MutexAutoLock lock(mLock);

don't lock, there is no need

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2147,5 @@
> +  ev = NS_NewRunnableMethod(ioMan,
> +                            &CacheFileIOManager::EvictIfOverLimitInternal);
> +
> +  rv = ioMan->mIOThread->Dispatch(ev, CacheIOThread::EVICT);
> +  NS_ENSURE_SUCCESS(rv, rv);

this will dispatch an event (a lot of allocation and lock acquirement) on every entry update while it will probably do nothing at the end.  Could we check the limit somehow (even imprecisely) here before posting?

I think we could have some uint32_t index->mIndexStats.Size32() accessible completely w/o a lock that will cache last correct value of the int64_t Size().

Like mSize32 = uint32_t(mSize64) always when mSize64 were updated.

This should atomically store the value. That would be much more optimal IMO (zero locking, zero posting).

@@ +2190,5 @@
> +  ev = NS_NewRunnableMethod(this,
> +                            &CacheFileIOManager::OverLimitEvictionInternal);
> +
> +  rv = mIOThread->Dispatch(ev, CacheIOThread::EVICT);
> +  NS_ENSURE_SUCCESS(rv, rv);

This may use some delaying after the IO thread is IDLE for some time, like 5 secs or so, probably handled by the IO thread natively... (followup bug OK).

@@ +2203,5 @@
> +  LOG(("CacheFileIOManager::OverLimitEvictionInternal()"));
> +
> +  nsresult rv;
> +
> +  mOverLimitEvicting = false;

should be at the bottom?  please explain how this flag exactly works, not clear.

@@ +2226,5 @@
> +    LOG(("CacheFileIOManager::OverLimitEvictionInternal() - Cache size over "
> +         "limit. [cacheSize=%u, limit=%u]", cacheUsage, cacheLimit));
> +
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();

I think you can go with NowLoRes()

@@ +2228,5 @@
> +
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();
> +    } else {
> +      TimeDuration elapsed = TimeStamp::Now() - start;

as well here

@@ +2229,5 @@
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();
> +    } else {
> +      TimeDuration elapsed = TimeStamp::Now() - start;
> +      if (elapsed.ToMilliseconds() >= kEvictionLoopLimit) {

as mentioned in the index patch:

static TimeDuration const kEvictionLoopLimit = TimeDuration::FromMilliseconds(...);
if (elapsed >= kEvictionLoopLimit) { ..

@@ +2249,5 @@
> +    }
> +    else if (rv == NS_ERROR_NOT_AVAILABLE) {
> +      LOG(("CacheFileIOManager::OverLimitEvictionInternal() - "
> +           "DoomFileByKeyInternal() failed. [rv=0x%08x]", rv));
> +      // TODO index is outdated, start update

file a bug?

@@ +2255,5 @@
> +      // Make sure index won't return the same entry again
> +      CacheIndex::RemoveEntry(&hash);
> +      consecutiveFailures = 0;
> +    }
> +    else {

} else {

@@ +2265,5 @@
> +      LOG(("CacheFileIOManager::OverLimitEvictionInternal() - "
> +           "DoomFileByKeyInternal() failed. [rv=0x%08x]", rv));
> +
> +      // Mark entry fresh so we can update it.
> +      rv = CacheIndex::EnsureEntryExists(&hash);

Why doesn't it work w/o this step?

@@ +2276,5 @@
> +      rv = CacheIndex::UpdateEntry(&hash, &frecency, &expTime, nullptr);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      consecutiveFailures++;
> +      if (consecutiveFailures >= cnt) {

maybe even sooner? not sure...

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +745,4 @@
>    uint32_t keySize = reinterpret_cast<CacheFileMetadataHeader *>(
>                         mBuf + hdrOffset)->mKeySize;
>  
> +  if (!aHaveKey) {

doesn't mKey.IsEmpty() do the job for you? (not sure).

::: netwerk/cache2/CacheFileUtils.cpp
@@ +84,5 @@
> +    info.forget(aInfo);
> +  }
> +
> +  if (aURL) {
> +    *aURL = Substring(aKey, appIdEndIdx + 1);

This may not be the url, you forgot the key enhance (for posts) and I plan other distinction schemes here or in the storage key itself.

::: netwerk/cache2/CacheIndex.cpp
@@ +1075,5 @@
> +
> +  rv = index->EnsureIndexUsable();
> +  if (NS_FAILED(rv)) return rv;
> +
> +  *_retval = index->mIndexStats.Size();

this should be int64_t result.

@@ +2823,5 @@
>    bool Equals(CacheIndexRecord* a, CacheIndexRecord* b) const {
>      return a->mFrecency == b->mFrecency;
>    }
>    bool LessThan(CacheIndexRecord* a, CacheIndexRecord* b) const {
> +    // Place entries with frecency 0 at the end of the array.

or don't put them there at all?

::: netwerk/cache2/CacheIndex.h
@@ +426,5 @@
>      DO_NOT_KNOW    = 2
>    };
>  
>    static nsresult HasEntry(const nsACString &aKey, EntryStatus *_retval);
> +  static nsresult GetEntryForEviction(SHA1Sum::Hash *aHash, uint32_t *aCnt);

please add comments, by just looking at the arg they don't make much sense.  why returning one hash but also count? is the hash an in-arg or out-arg?

::: netwerk/cache2/CacheStorageService.cpp
@@ +1178,5 @@
>  
>      bool entryExists = entries->Get(entryKey, getter_AddRefs(entry));
>  
> +    // check whether the file is already doomed
> +    if (entryExists && entry->IsFileDoomed() && !aReplace) {

I think  && !aReplace is not needed.

::: netwerk/cache2/CacheStorageService.h
@@ +140,5 @@
> +   * an active entry was removed. This method is called even if the entry
> +   * removal was originated by CacheStorageService.
> +   */
> +  nsresult CacheFileDoomed(nsILoadContextInfo* aLoadContextInfo,
> +                           const nsACString & aURL);

please create a block as following

private:
  friend class CacheFileIOManager;

  /**
   * ...
   */
  nsresult CacheFileDoomed(...)


It's then clear what is exposed to/used by what (even though friend applies to the whole class).

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8370191 [details] [diff] [review]
patch v2

see comment 26.

Comment 28

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #26)
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +2147,5 @@
> > +  ev = NS_NewRunnableMethod(ioMan,
> > +                            &CacheFileIOManager::EvictIfOverLimitInternal);
> > +
> > +  rv = ioMan->mIOThread->Dispatch(ev, CacheIOThread::EVICT);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> this will dispatch an event (a lot of allocation and lock acquirement) on
> every entry update while it will probably do nothing at the end.  Could we
> check the limit somehow (even imprecisely) here before posting?

No, CacheFileIOManager::EvictIfOverLimit() is called only from CacheIndex::ChangeState() which is called rarely.


> @@ +2203,5 @@
> > +  LOG(("CacheFileIOManager::OverLimitEvictionInternal()"));
> > +
> > +  nsresult rv;
> > +
> > +  mOverLimitEvicting = false;
> 
> should be at the bottom?  please explain how this flag exactly works, not
> clear.

mOverLimitEvicting is set only on IO thread, so there is no problem to set false here. When anything goes wrong in this method, we can simply return error and mOverLimitEvicting will have the correct value. It is set to true at the end of this function when we post another evicting event. I'll add a comment.


> @@ +2226,5 @@
> > +    LOG(("CacheFileIOManager::OverLimitEvictionInternal() - Cache size over "
> > +         "limit. [cacheSize=%u, limit=%u]", cacheUsage, cacheLimit));
> > +
> > +    if (start.IsNull()) {
> > +      start = TimeStamp::Now();
> 
> I think you can go with NowLoRes()

The eviction loop limit is 40ms and description of NowLoRes() recommends to use it for timeouts >200ms.


> @@ +2265,5 @@
> > +      LOG(("CacheFileIOManager::OverLimitEvictionInternal() - "
> > +           "DoomFileByKeyInternal() failed. [rv=0x%08x]", rv));
> > +
> > +      // Mark entry fresh so we can update it.
> > +      rv = CacheIndex::EnsureEntryExists(&hash);
> 
> Why doesn't it work w/o this step?

There are various checks in index code to make sure we work with the entries correctly. One of them is that we can update only entry that is fresh (we've either created it, or already opened it within this FF session). In CacheFileIOManager::OverLimitEvictionInternal() we don't know whether the entry is fresh or not, i.e. whether we can call CacheIndex::UpdateEntry() or not.


> @@ +2276,5 @@
> > +      rv = CacheIndex::UpdateEntry(&hash, &frecency, &expTime, nullptr);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      consecutiveFailures++;
> > +      if (consecutiveFailures >= cnt) {
> 
> maybe even sooner? not sure...

Variable cnt contains number of entries in frecency/expiration array. In case of this unexpected error we want to try to remove another entries until the cache is in limit. cnt is IMO a good choice because there is a high probability that we'll try to evict every entry before we give up. But lower value would probably work too, since any further cache activity would start eviction again and we would try to remove another entries.


> ::: netwerk/cache2/CacheFileMetadata.cpp
> @@ +745,4 @@
> >    uint32_t keySize = reinterpret_cast<CacheFileMetadataHeader *>(
> >                         mBuf + hdrOffset)->mKeySize;
> >  
> > +  if (!aHaveKey) {
> 
> doesn't mKey.IsEmpty() do the job for you? (not sure).

It should as long as empty string is an invalid cache key.


> ::: netwerk/cache2/CacheIndex.cpp
> @@ +1075,5 @@
> > +
> > +  rv = index->EnsureIndexUsable();
> > +  if (NS_FAILED(rv)) return rv;
> > +
> > +  *_retval = index->mIndexStats.Size();
> 
> this should be int64_t result.

Hmm, I think CacheIndexStats::Size() should return uint32_t. I defined it internally as int64_t because I probably wanted to have a check in BeforeChange() that mSize don't go below 0.


> @@ +2823,5 @@
> >    bool Equals(CacheIndexRecord* a, CacheIndexRecord* b) const {
> >      return a->mFrecency == b->mFrecency;
> >    }
> >    bool LessThan(CacheIndexRecord* a, CacheIndexRecord* b) const {
> > +    // Place entries with frecency 0 at the end of the array.
> 
> or don't put them there at all?

I think we should be able to evict any entry even if it is quite new. Otherwise it could happen with very low "browser.cache.disk.capacity" that cache size is non-zero but there is nothing to evict.

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #28)
> (In reply to Honza Bambas (:mayhemer) from comment #26)
> > ::: netwerk/cache2/CacheFileIOManager.cpp
> > @@ +2147,5 @@
> > > +  ev = NS_NewRunnableMethod(ioMan,
> > > +                            &CacheFileIOManager::EvictIfOverLimitInternal);
> > > +
> > > +  rv = ioMan->mIOThread->Dispatch(ev, CacheIOThread::EVICT);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > 
> > this will dispatch an event (a lot of allocation and lock acquirement) on
> > every entry update while it will probably do nothing at the end.  Could we
> > check the limit somehow (even imprecisely) here before posting?
> 
> No, CacheFileIOManager::EvictIfOverLimit() is called only from
> CacheIndex::ChangeState() which is called rarely.

Ok, then let's leave it.

> 
> 
> > @@ +2203,5 @@
> > > +  LOG(("CacheFileIOManager::OverLimitEvictionInternal()"));
> > > +
> > > +  nsresult rv;
> > > +
> > > +  mOverLimitEvicting = false;
> > 
> > should be at the bottom?  please explain how this flag exactly works, not
> > clear.
> 
> mOverLimitEvicting is set only on IO thread, so there is no problem to set
> false here. When anything goes wrong in this method, we can simply return
> error and mOverLimitEvicting will have the correct value. It is set to true
> at the end of this function when we post another evicting event. I'll add a
> comment.

I thought the flag prevents posting the same evict event ones more, maybe I just need to read the code ones more.  Comment welcome.

> 
> 
> > @@ +2226,5 @@
> > > +    LOG(("CacheFileIOManager::OverLimitEvictionInternal() - Cache size over "
> > > +         "limit. [cacheSize=%u, limit=%u]", cacheUsage, cacheLimit));
> > > +
> > > +    if (start.IsNull()) {
> > > +      start = TimeStamp::Now();
> > 
> > I think you can go with NowLoRes()
> 
> The eviction loop limit is 40ms and description of NowLoRes() recommends to
> use it for timeouts >200ms.

Yes, I know.  I wrote that comment my self :)

Maybe I should change it, you don't need to be precise here.  on win the timer gives 15.6 ms resolution.  we are going to change this anyway with the yield API, anyway Now() is just wasting here.

> 
> 
> > @@ +2265,5 @@
> > > +      LOG(("CacheFileIOManager::OverLimitEvictionInternal() - "
> > > +           "DoomFileByKeyInternal() failed. [rv=0x%08x]", rv));
> > > +
> > > +      // Mark entry fresh so we can update it.
> > > +      rv = CacheIndex::EnsureEntryExists(&hash);
> > 
> > Why doesn't it work w/o this step?
> 
> There are various checks in index code to make sure we work with the entries
> correctly. One of them is that we can update only entry that is fresh (we've
> either created it, or already opened it within this FF session). In
> CacheFileIOManager::OverLimitEvictionInternal() we don't know whether the
> entry is fresh or not, i.e. whether we can call CacheIndex::UpdateEntry() or
> not.

Aha, so you ensure it's in an expected state to make UpdateEntry pass.  OK, please put some comment there.

> 
> 
> > @@ +2276,5 @@
> > > +      rv = CacheIndex::UpdateEntry(&hash, &frecency, &expTime, nullptr);
> > > +      NS_ENSURE_SUCCESS(rv, rv);
> > > +
> > > +      consecutiveFailures++;
> > > +      if (consecutiveFailures >= cnt) {
> > 
> > maybe even sooner? not sure...
> 
> Variable cnt contains number of entries in frecency/expiration array. In
> case of this unexpected error we want to try to remove another entries until
> the cache is in limit. cnt is IMO a good choice because there is a high
> probability that we'll try to evict every entry before we give up. But lower
> value would probably work too, since any further cache activity would start
> eviction again and we would try to remove another entries.

got it.

> 
> 
> > ::: netwerk/cache2/CacheFileMetadata.cpp
> > @@ +745,4 @@
> > >    uint32_t keySize = reinterpret_cast<CacheFileMetadataHeader *>(
> > >                         mBuf + hdrOffset)->mKeySize;
> > >  
> > > +  if (!aHaveKey) {
> > 
> > doesn't mKey.IsEmpty() do the job for you? (not sure).
> 
> It should as long as empty string is an invalid cache key.

I was just wondering if we could safely get rid if the new argument.

> 
> 
> > ::: netwerk/cache2/CacheIndex.cpp
> > @@ +1075,5 @@
> > > +
> > > +  rv = index->EnsureIndexUsable();
> > > +  if (NS_FAILED(rv)) return rv;
> > > +
> > > +  *_retval = index->mIndexStats.Size();
> > 
> > this should be int64_t result.
> 
> Hmm, I think CacheIndexStats::Size() should return uint32_t. I defined it
> internally as int64_t because I probably wanted to have a check in
> BeforeChange() that mSize don't go below 0.

OK, then you should just explicitly cast int64_t to uint32_t.

> 
> 
> > @@ +2823,5 @@
> > >    bool Equals(CacheIndexRecord* a, CacheIndexRecord* b) const {
> > >      return a->mFrecency == b->mFrecency;
> > >    }
> > >    bool LessThan(CacheIndexRecord* a, CacheIndexRecord* b) const {
> > > +    // Place entries with frecency 0 at the end of the array.
> > 
> > or don't put them there at all?
> 
> I think we should be able to evict any entry even if it is quite new.
> Otherwise it could happen with very low "browser.cache.disk.capacity" that
> cache size is non-zero but there is nothing to evict.

good argument.

Comment 30

[Michal Novotny [:michal]]

Attachment: patch v3 - addressed reviewer's comments

Comment 31

[Michal Novotny [:michal]]

pushed to gum http://hg.mozilla.org/projects/gum/rev/3bbc1af97190

Comment 32

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2275f8be60c9

Comment 33

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f21473b58c6

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f21473b58c6