Closed Bug 958317 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking: Cache, enhancement)

Other Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 957707
No longer blocks: cache2enable
Attached patch v1 (obsolete) — Splinter Review
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...
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8375875 - Flags: review?(michal.novotny)
Comment on attachment 8375875 [details] [diff] [review]
v1

Already working on a better version, realized I can test with the purging cycle.
Attachment #8375875 - Flags: review?(michal.novotny)
Attached patch v2 (obsolete) — Splinter Review
I think the patch is self explanatory.  Otherwise please ask questions.
Attachment #8375875 - Attachment is obsolete: true
Attachment #8379399 - Flags: review?(michal.novotny)
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
Attachment #8379399 - Flags: review?(michal.novotny) → review+
(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?
(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.
(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 ;)
(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 :)
Attached patch v3Splinter Review
- 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.
Attachment #8379399 - Attachment is obsolete: true
Attachment #8379784 - Flags: review+
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.
Flags: in-testsuite-
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.
(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..)
https://hg.mozilla.org/mozilla-central/rev/a0b80ff2d638
Status: ASSIGNED → 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.