Closed Bug 968106 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Summary: HTTP cache v2: implement proper eviction of the while disk cache → HTTP cache v2: implement proper eviction of the whole disk cache
Blocks: 965089
Attached patch patch v1 (obsolete) — Splinter Review
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
Attachment #8381637 - Flags: review?(honzab.moz)
Depends on: 976866
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
Attachment #8381637 - Flags: review?(honzab.moz) → review+
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.
Blocks: 966438
Blocks: 920573
Attached patch patch v2 (obsolete) — Splinter Review
> @@ +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
Attachment #8381637 - Attachment is obsolete: true
Attachment #8386622 - Flags: review+
(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?
(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?
(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.
(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.
Attached patch patch v3Splinter Review
Just a small change in CacheFileIOManager::EvictAllInternal() to make test_private_channel.js working.
Attachment #8386622 - Attachment is obsolete: true
Attachment #8387481 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b5d8474e3042
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.