958317 - HTTP cache v2: allow yield to more priority levels in IOThread

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[Honza Bambas (:mayhemer)]

Perf related, blocking c2e.

API suggestions:
bool CacheFileIOManager::YieldIOThread()
 { return gInstance->IOThread()->HasMorePriorityPending(); }

HasMorePriorityPending {
  return HasPendingEvents(mCurrentRunningLevel);
}

During loops on lower levels of the IO thread that might take longer time (purging, eviction) and might thus block priority operations like reading/opening/processing callbacks, the loop has to check for this condition and return from (+ repost if needed) the event's Run() method.

There could be an API or new nsresult value to repost the event automatically.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

So far the API is untested.  Michal, feel free to take the patch (merge if needed) and check with your patches that may use it.

I rather abandoned the idea of return a special error from Run().  I was not able to write a satisfying comment for it...

Comment 2

[Honza Bambas (:mayhemer)]

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

Already working on a better version, realized I can test with the purging cycle.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v2

I think the patch is self explanatory.  Otherwise please ask questions.

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8379399 [details] [diff] [review]
v2

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

::: netwerk/cache2/CacheFileIOManager.cpp
@@ -2340,5 @@
> -    if (start.IsNull()) {
> -      start = TimeStamp::NowLoRes();
> -    } else {
> -      TimeDuration elapsed = TimeStamp::NowLoRes() - start;
> -      if (elapsed.ToMilliseconds() >= kEvictionLoopLimit) {

Defined limits are no longer needed, so remove kEvictionLoopLimit and kRemoveTrashLoopLimit.

@@ +2403,1 @@
>    return NS_OK;

Add NS_NOTREACHED here since after this change, we should never get to this place.

@@ +2653,1 @@
>    return NS_OK;

NS_NOTREACHED

::: netwerk/cache2/CacheIOThread.h
@@ +65,5 @@
> +   *    will not execute automatically again.
> +   */
> +  enum ERepost {
> +    REPOST,
> +    ABANDONE

ABANDONE -> ABANDON

Anyway, I don't think there is a need to call Yield() with ABANDON argument, so why not to always re-post the event?

@@ +68,5 @@
> +    REPOST,
> +    ABANDONE
> +  };
> +
> +  static bool Yield(ERepost aRepost = ABANDONE)

Even if we want to keep the argument, couldn't it be simple bool aRepost?

::: netwerk/cache2/CacheIndex.cpp
@@ -2374,5 @@
> -    if (start.IsNull()) {
> -      start = TimeStamp::NowLoRes();
> -    } else {
> -      static TimeDuration const kLimit = TimeDuration::FromMilliseconds(
> -                                           kBuildIndexLoopLimit);

Remove kBuildIndexLoopLimit and kUpdateIndexLoopLimit defines.

@@ +2470,5 @@
>        LOG(("CacheIndex::BuildIndex() - Added entry to index. [hash=%s]",
>             leaf.get()));
>        entry->Log();
>      }
>    }

NS_NOTREACHED

@@ +2735,5 @@
>        LOG(("CacheIndex::UpdateIndex() - Added/updated entry to/in index. "
>             "[hash=%s]", leaf.get()));
>        entry->Log();
>      }
>    }

NS_NOTREACHED

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #4)
> Comment on attachment 8379399 [details] [diff] [review]
> v2
> 
> Review of attachment 8379399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks.

> @@ +68,5 @@
> > +    REPOST,
> > +    ABANDONE
> > +  };
> > +
> > +  static bool Yield(ERepost aRepost = ABANDONE)
> 
> Even if we want to keep the argument, couldn't it be simple bool aRepost?

I wanted to make clear what the argument is doing by just looking at the call statement.  I'm actually enemy of bool args you need to examine what they mean.  Other option for me was to have two methods: Yield() and YieldAndRepost().  That would be even more descriptive.

What do you think?

Comment 6

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> I wanted to make clear what the argument is doing by just looking at the
> call statement.  I'm actually enemy of bool args you need to examine what
> they mean.  Other option for me was to have two methods: Yield() and
> YieldAndRepost().  That would be even more descriptive.
> 
> What do you think?

As I wrote I don't think we really need the ABANDON functionality, so implementing just YieldAndRepost() would be sufficient. Btw, the word "repost" evokes in me that the event will be placed as the last event in the given level queue, but we don't do it.


I found a comment in the patch that is unclear to me:

> +      // Release outside the lock.
>        events[index] = nullptr;

This code is in the block where we hold the lock.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > I wanted to make clear what the argument is doing by just looking at the
> > call statement.  I'm actually enemy of bool args you need to examine what
> > they mean.  Other option for me was to have two methods: Yield() and
> > YieldAndRepost().  That would be even more descriptive.
> > 
> > What do you think?
> 
> As I wrote I don't think we really need the ABANDON functionality, 

True is that on all places I use this method it only makes sense to let the event be executed again.  Ok, let's simplify the API then.  When needed, it will be simple to introduce it.

> implementing just YieldAndRepost() would be sufficient. Btw, the word
> "repost" evokes in me that the event will be placed as the last event in the
> given level queue, but we don't do it.

Good point.  Events posted later to the same level will not execute sooner.  Tho, I don't have an idea of a better naming at the moment...

YieldAndReinvoke() ?

> 
> 
> I found a comment in the patch that is unclear to me:
> 
> > +      // Release outside the lock.
> >        events[index] = nullptr;
> 
> This code is in the block where we hold the lock.

It's unlock ;)

Comment 8

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Good point.  Events posted later to the same level will not execute sooner. 
> Tho, I don't have an idea of a better naming at the moment...
> 
> YieldAndReinvoke() ?

YieldAndReinvoke() or YieldAndRerun() will be OK.


> > This code is in the block where we hold the lock.
> 
> It's unlock ;)

Ah! So it's clear to me now :)

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v3

- bool YieldAndRerun(void)
- review comments
- removed the MOZ_CRASH when some xpcom-dispatched event calls this api, since I don't think (now when simplified and also because we return false) is a coding fatal mistake, it's just simply a no-op

Michal, feel free to check ones more on the patch, I will not land it sooner then next week.

Comment 10

[Honza Bambas (:mayhemer)]

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

Comment 11

[Honza Bambas (:mayhemer)]

It is very hard to build an automated test for this feature.  It would have to be a C++ test and would be dependent on timing that is not easily controllable.

Comment 12

[Honza Bambas (:mayhemer)]

There is a bug in CacheStorageService::PurgeOverMemoryLimit().  mPurging = CacheIOThread::YieldAndRerun() is wrong, this will make the event rerun everytime there is an even on a higher level.  I will update the patch.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> There is a bug in CacheStorageService::PurgeOverMemoryLimit().  mPurging =
> CacheIOThread::YieldAndRerun() is wrong, this will make the event rerun
> everytime there is an even on a higher level.  I will update the patch.

Hmm.. maybe this is alright (getting tired today..)

Comment 14

[Michal Novotny [:michal]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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