968106 - HTTP cache v2: implement proper eviction of the whole disk cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

When evicting the whole disk cache we should do:

- doom all active entries
- rename entries directory to some temporary name
- create a new empty entries directory
- start deleting the temporary directory

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

Some changes were needed in CacheIndex for fast CacheIndex::RemoveAll() implementation. Now it is safe to interrupt any pending process at any time using FinishRead(), FinishWrite() or FinishUpdate() method.

Tryserver push: https://tbpl.mozilla.org/?tree=Try&rev=1d004eb6e0b7

Comment 2

[Honza Bambas (:mayhemer)]

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

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

When each of the read, update etc state handling would be in a separate short living class this all code would be much simpler, more transparent and IMO also less fragile.  You are introducing two more state flags and more state checks to the one giant already heavy loaded class.  This is no review requirement, just a comment for inspiration.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2473,5 @@
> +
> +nsresult
> +CacheFileIOManager::EvictAllInternal()
> +{
> +  LOG(("CacheFileIOManager::EvictAllInternal()"));

assert thread

@@ +2478,5 @@
> +
> +  nsresult rv;
> +
> +  nsRefPtr<EvictionNotifierRunnable> r = new EvictionNotifierRunnable();
> +  NS_DispatchToMainThread(r);

Should this be done after we rename the tree at least?  For consistency, if the consumer(s) would want to examine the disk content somehow...  Otherwise when bug 976866 is fixed, we could fire this just instantly from the mainthread call.  But that doesn't seem right to me.  We should actually remove the data or make it inaccessible before this notification is posted.

@@ +2495,5 @@
> +  // Doom all active handles
> +  nsTArray<nsRefPtr<CacheFileHandle> > handles;
> +  mHandles.GetActiveHandles(&handles);
> +
> +  for (uint32_t i=0 ; i<handles.Length() ; i++) {

for (uint32_t i = 0; i < handles.Length(); ++i) {

::: netwerk/cache2/CacheIndex.cpp
@@ +1056,5 @@
> +    if (!index->IsIndexUsable()) {
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +
> +    index->mRemovingAll = true;

use AutoRestore to revert to false.

@@ +1068,5 @@
> +      // it outside the lock. Ignore the result since this is not fatal.
> +      index->GetFile(NS_LITERAL_CSTRING(kIndexName), getter_AddRefs(file));
> +    }
> +
> +    if(index->mJournalHandle) {

if (

@@ +1087,5 @@
> +      case UPDATING:
> +        index->FinishUpdate(false);
> +        break;
> +      default:
> +        MOZ_ASSERT(false, "Unexpected state!");

MOZ_CRASH?

@@ +1535,5 @@
>    mRWHash = nullptr;
>    ReleaseBuffer();
>  
>    if (aSucceeded) {
> +    MOZ_ASSERT(!mIndexFileOpener);

worth a comment

@@ +1542,5 @@
>      mIndexOnDiskIsValid = true;
> +  } else {
> +    if (mIndexFileOpener) {
> +      mIndexFileOpener->Cancel();
> +      mIndexFileOpener = nullptr;

please explain this in a comment.

@@ +2345,5 @@
>    if (index->mState == READY && index->mShuttingDown) {
>      return;
>    }
>  
> +  MOZ_ASSERT(!index->mUpdateEventPending);

why?

@@ +2354,4 @@
>  
>    // We need to redispatch to run with lower priority
>    nsRefPtr<CacheIOThread> ioThread = CacheFileIOManager::IOThread();
>    MOZ_ASSERT(ioThread);

please do a proper null check here, I didn't catch this the last time..

@@ +2622,2 @@
>      return;
>    }

could use some helper method.

@@ +2653,3 @@
>    mDontMarkIndexClean = false;
>  
> +  if (mShuttingDown || mRemovingAll) {

is this under the lock or on a single thread?  or just arbitrarily accessed?

@@ +2876,4 @@
>      NS_WARNING("CacheIndex::UpdateIndex() - Can't dispatch event");
>      LOG(("CacheIndex::UpdateIndex() - Can't dispatch event" ));
>      FinishUpdate(false);
>      return;

no need to return?

@@ +2913,3 @@
>    }
> +
> +  if (mState == UPDATING && aSucceeded) {

|| READY ? (just checking, probably not..)

@@ +2985,5 @@
>      return;
>    }
>  
>    // Try to evict entries over limit everytime we're leaving state READING,
> +  // BUILDING or UPDATING, but not during shutdown and when removing all

not during shutdown OR when removing all

@@ +3167,5 @@
>          WriteRecords();
>        }
>        break;
>      case READING:
> +      if (aOpener == mIndexFileOpener) {

Nice! :)

@@ +3174,3 @@
>          if (NS_SUCCEEDED(aResult)) {
> +          if (aHandle->FileSize() == 0) {
> +            FinishRead(false);

How will StartReadingIndex() behave after we read all other files?  I think we reach that code.

@@ +3226,1 @@
>            break;

you don't need this break.

I'd be a bit more happy if you would remove the else tho

::: netwerk/cache2/CacheIndex.h
@@ +514,5 @@
>                                const uint32_t      *aFrecency,
>                                const uint32_t      *aExpirationTime,
>                                const uint32_t      *aSize);
>  
> +  // Remove all entries from index. Called when clearing the whole cache.

from the index

Comment 3

[Honza Bambas (:mayhemer)]

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

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

::: netwerk/cache2/CacheIOThread.h
@@ +33,5 @@
>      READ,
>      WRITE,
>      MANAGEMENT,
>      CLOSE,
> +    UPDATE_INDEX,

forgot to mention: please name this only INDEX.  no need for a wordy label here.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v2

> @@ +1087,5 @@
> > +      case UPDATING:
> > +        index->FinishUpdate(false);
> > +        break;
> > +      default:
> > +        MOZ_ASSERT(false, "Unexpected state!");
> 
> MOZ_CRASH?

Assert is IMO ok, otherwise it would be necessary to change also other similar asserts in CacheIndex for consistency.


> @@ +2354,4 @@
> >  
> >    // We need to redispatch to run with lower priority
> >    nsRefPtr<CacheIOThread> ioThread = CacheFileIOManager::IOThread();
> >    MOZ_ASSERT(ioThread);
> 
> please do a proper null check here, I didn't catch this the last time..

Why? ioThread must be non-null here.


> @@ +2622,2 @@
> >      return;
> >    }
> 
> could use some helper method.

Helper method for what? Anyway, this code has been removed and CacheIOThread::YieldAndRerun() is used instead.


> @@ +2653,3 @@
> >    mDontMarkIndexClean = false;
> >  
> > +  if (mShuttingDown || mRemovingAll) {
> 
> is this under the lock or on a single thread?  or just arbitrarily accessed?

It is under the lock. I added an assert.


> @@ +2913,3 @@
> >    }
> > +
> > +  if (mState == UPDATING && aSucceeded) {
> 
> || READY ? (just checking, probably not..)

mState cannot be READY here, see the assert at the beginning of this method.


> @@ +3174,3 @@
> >          if (NS_SUCCEEDED(aResult)) {
> > +          if (aHandle->FileSize() == 0) {
> > +            FinishRead(false);
> 
> How will StartReadingIndex() behave after we read all other files?  I think
> we reach that code.

I don't understand. Do you mean "after we opened all other files" instead? FinishRead(false) cancels all remaining openers, so OpenFileOpenedInternal() is not called anymore.


> @@ +3226,1 @@
> >            break;
> 
> you don't need this break.

I know, but every FinishRead(false) in this method is followed by break to make sure we step out of the switch-statement immediately. This one is not needed but I'm afraid that it is easy to forget to add the break there if someone ever writes some additional logic.

> I'd be a bit more happy if you would remove the else tho

Why? The current if-statement IMO corresponds to the logic:

do we have journal file?
  yes -> rename it
  no  -> start reading index

Comment 5

[Michal Novotny [:michal]]

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

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

Comment 6

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> @@ +2478,5 @@
> > +
> > +  nsresult rv;
> > +
> > +  nsRefPtr<EvictionNotifierRunnable> r = new EvictionNotifierRunnable();
> > +  NS_DispatchToMainThread(r);
> 
> Should this be done after we rename the tree at least?  For consistency, if
> the consumer(s) would want to examine the disk content somehow...  Otherwise
> when bug 976866 is fixed, we could fire this just instantly from the
> mainthread call.  But that doesn't seem right to me.  We should actually
> remove the data or make it inaccessible before this notification is posted.

Test test_private_channel.js fails (times out) after this change, because CreateCacheTree() in CacheFileIOManager::EvictAllInternal() fails because there is no mCacheDirectory and no notification is sent. Should we also send the notification in case of failure (or just some specific failure), or should we just say that the test is badly written?

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #4)
> > please do a proper null check here, I didn't catch this the last time..
> 
> Why? ioThread must be non-null here.

I think you are too trustworthy, but OK, if you trust this API in your code, let it be so.

> > @@ +3174,3 @@
> > >          if (NS_SUCCEEDED(aResult)) {
> > > +          if (aHandle->FileSize() == 0) {
> > > +            FinishRead(false);
> > 
> > How will StartReadingIndex() behave after we read all other files?  I think
> > we reach that code.
> 
> I don't understand. Do you mean "after we opened all other files" instead?

Yes.

> FinishRead(false) cancels all remaining openers, so OpenFileOpenedInternal()
> is not called anymore.

OK.

> > I'd be a bit more happy if you would remove the else tho
> 
> Why? The current if-statement IMO corresponds to the logic:
> 
> do we have journal file?
>   yes -> rename it
>   no  -> start reading index

Ah, overlook, sorry.  This is OK.

(In reply to Michal Novotny (:michal) from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > @@ +2478,5 @@
> > > +
> > > +  nsresult rv;
> > > +
> > > +  nsRefPtr<EvictionNotifierRunnable> r = new EvictionNotifierRunnable();
> > > +  NS_DispatchToMainThread(r);
> > 
> > Should this be done after we rename the tree at least?  For consistency, if
> > the consumer(s) would want to examine the disk content somehow...  Otherwise
> > when bug 976866 is fixed, we could fire this just instantly from the
> > mainthread call.  But that doesn't seem right to me.  We should actually
> > remove the data or make it inaccessible before this notification is posted.
> 
> Test test_private_channel.js fails (times out) after this change, because
> CreateCacheTree() in CacheFileIOManager::EvictAllInternal() fails because
> there is no mCacheDirectory and no notification is sent. Should we also send
> the notification in case of failure (or just some specific failure), or
> should we just say that the test is badly written?

I think we should send a notification whenever we are confident the disk is deleted, so in case there is no cache, we should notify as well.


Can you please land the patch on gum?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Can you please land the patch on gum?

Ah, there is a try run.  That is better.

On what position in the pending queue you want to land this bug?  To reland the r+'ed stuff currently pending on gum we need this patch (according your words).  So it should go right after the last r+'ed patch which is 964039-memeory-reporting.  I think it's better to land by parts, so if you can - if it's not an overkill, please remerge on top of that patch.  Also this is somewhat dependent on bug 976866.  But that would just potentially make tests unstable with cache2, so just on gum.  Not critical for landing on m-c.

Comment 9

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> On what position in the pending queue you want to land this bug?  To reland
> the r+'ed stuff currently pending on gum we need this patch (according your
> words).  So it should go right after the last r+'ed patch which is
> 964039-memeory-reporting.  I think it's better to land by parts, so if you
> can - if it's not an overkill, please remerge on top of that patch.

The patch collides quite a lot with 958317-yield-even-simpler.patch so I would rather push it on the top of this patch.

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v3

Just a small change in CacheFileIOManager::EvictAllInternal() to make test_private_channel.js working.

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 8387481 [details] [diff] [review]
patch v3

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

Comment 12

[Michal Novotny [:michal]]

Landed on gum: http://hg.mozilla.org/projects/gum/rev/3b4f8fdd57f6

Comment 13

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d8474e3042

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5d8474e3042