All users were logged out of Bugzilla on October 13th, 2018

HTTP cache v2: make callbacks iteration in CacheEntry smarter

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cache2][qa-])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
- improve threading
- improve flagging (needs ENTRY_WANTED_COMPLETE, better code for r/o callbacks)
(Assignee)

Updated

5 years ago
Depends on: 917432
(Assignee)

Comment 1

5 years ago
Created attachment 813144 [details] [diff] [review]
v1

This patch is base for two goals:
* fix concurrent read of non-resumable responses (leads to broken pages etc.)
* adaption of wyciwyg channels to the new cache api


api changes:

- new onCacheEntryCheck result: ENTRY_WANTED_COMPLETE, see bellow
- new open flag: nsICacheStorage::CHECK_MULTITHREADED, see bellow


cache2 code changes (disabled by default):

- the callbacks array (only a single one now) in CacheEntry now keeps a helper class that:
  - refers the callback interface
  - records the consumer's demand for read-only open
  - whether it's OK to call OnCacheEntryCheck on any thread (the CHECK_M.T. flag)
  - remember on which thread to invoke OnCacheEntryAvail (and OnCacheEntryCheck when not multithreaded) callback(s) ; this is the thread where AsyncOpenURI has been called
  - whether the consumer wants onAvail be called only after the output stream is closed (the WANT_COMPLETE result of onCheck) ; needed for non-resumable responses to prevent concurrent reading
  - whether the consumer wants the entry at all (NOT_WANTED result)
- mainThreadOnly r/o attribute removed from nsICacheEntryOpenCallback, CHECK_MULTITHREADED does the job now
- CacheFileOutputStream refers a new small helper class CacheOutputCloseListener (that refers the CacheEntry) and notifies this listener when the stream has been closed (removed from CacheFile) ; CacheEntry then invokes callbacks waiting for the complete entry
- small CacheEntry::mFile cleanup
- other very small cleanups
- test for ENTRY_WANTED_COMPLETE


cache1 code changes, need careful review:

- calling asyncOpenURI is now allowed on the main thread and on the cache thread
- when OPEN_TRUNCATE (-> access = WRITE only) we call sync OpenCacheEntry ; this happens only on the cache thread
- when asyncOpenURI is called on the cache thread, processing the open is done synchronously (no repost to the cache thread)


https://tbpl.mozilla.org/?tree=Try&rev=2931fc6bbb9d
Attachment #813144 - Flags: review?(michal.novotny)
(Assignee)

Comment 2

5 years ago
I have an updated patch already.  Will submit tomorrow.
(Assignee)

Comment 3

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #2)
> I have an updated patch already.  Will submit tomorrow.

Hmm.. there are just merge updates.  The patch has not changed.  Feel free to review this version.

(Too tired..)
(Assignee)

Updated

5 years ago
Attachment #813757 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Attachment #813144 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Attachment #813144 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #813757 - Attachment description: v1 [merged, for backup] → v1
(Assignee)

Updated

5 years ago
Blocks: 923688
(Assignee)

Comment 5

5 years ago
Created attachment 827977 [details] [diff] [review]
v1 [merged]

- just remerged to m-c
Attachment #813757 - Attachment is obsolete: true
Attachment #813757 - Flags: review?(michal.novotny)
Attachment #827977 - Flags: review?(michal.novotny)
(Assignee)

Comment 6

5 years ago
Created attachment 829266 [details] [diff] [review]
v1.1

- see comment 1 for overall comments

v1.1 additions:

- when a callback's onCacheEntryCheck result was WANTED_COMPLTETE we call onCacheEntryCheck again after the entry's output has been closed ; this way the callback can check again for partiality of the content and do a partial request (that is setup in onCacheEntryCheck)
- fixed the WANTED_COMPLETE test ; onCacheEntryCheck is not expected being called twice when the old backend is used
Attachment #827977 - Attachment is obsolete: true
Attachment #827977 - Flags: review?(michal.novotny)
Attachment #829266 - Flags: review?(michal.novotny)
Comment on attachment 829266 [details] [diff] [review]
v1.1

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

::: netwerk/cache2/CacheEntry.cpp
@@ +437,2 @@
>  
> +  InvokeCallbacks(true);

Do

if (InvokeCallbacks(false))
  InvokeCallbacks(true);

so that "CacheEntry::InvoikeCallback END" is always logged.

@@ +481,5 @@
> +    mCallbacks.RemoveElementAt(i);
> +
> +    if (NS_SUCCEEDED(rv) && !InvokeCallback(callback)) {
> +      // Callback didn't fire, put it back and go to another one in line
> +      mCallbacks.InsertElementAt(i, callback);

Let's say that we are processing r/w callbacks and one fails. It is inserted back and after finishing other r/w callbacks, we start invoking r/o callbacks even with the one failed r/w callback. Is this what we want to do?

::: netwerk/cache2/OldWrappers.cpp
@@ +527,5 @@
>  
>    // XXX: Start the cache service; otherwise DispatchToCacheIOThread will
>    // fail.
>    nsCOMPtr<nsICacheService> service =
>      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);

With the change above we potentially start the cache service off the main thread.

@@ +530,5 @@
>    nsCOMPtr<nsICacheService> service =
>      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
>  
>    // Ensure the stream transport service gets initialized on the main thread
> +  if (NS_SUCCEEDED(rv) && NS_IsMainThread()) {

The same as above. We cannot guarantee where is STS initialized when the first cache load is invoked from some background thread.

@@ +587,5 @@
>        LOG(("  session->AsyncOpenCacheEntry with access=%d", cacheAccess));
>  
>        bool bypassBusy = mFlags & nsICacheStorage::OPEN_BYPASS_IF_BUSY;
> +
> +      if (cacheAccess == nsICache::ACCESS_WRITE) {

I think there should be also "&& mSync" in the condition.

::: netwerk/cache2/nsICacheStorage.idl
@@ +42,5 @@
>     */
> +  const uint32_t OPEN_BYPASS_IF_BUSY = 1 << 3;
> +
> +  /**
> +   * Do cache check (onCacheEntryCHeck) on any thread for optimal

CHeck -> Check

::: netwerk/test/unit/head_cache2.js
@@ +125,5 @@
>  
> +    if (this.behavior & COMPLETE) {
> +      LOG_C2(this, "onCacheEntryCheck DONE, return ENTRY_WANTED_COMPLETE");
> +      if (newCacheBackEndUsed()) {
> +        this.onCheckPassed = false; // we expect this to get ones more with the new cache on

It is not clear to me why the check should be called twice in case of the new cache.
Attachment #829266 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 8

5 years ago
(In reply to Michal Novotny (:michal) from comment #7)
> Comment on attachment 829266 [details] [diff] [review]
> v1.1

Thanks.  Some of the details discussed privately, here is some other feedback.

> ::: netwerk/cache2/OldWrappers.cpp
> @@ +527,5 @@
> >  
> >    // XXX: Start the cache service; otherwise DispatchToCacheIOThread will
> >    // fail.
> >    nsCOMPtr<nsICacheService> service =
> >      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
> 
> With the change above we potentially start the cache service off the main
> thread.

Yes, but the only consumer that can do that is nsWyciwygChannel.  I'll add an initiation of this service there explicitly to be made on the main thread.

> 
> @@ +530,5 @@
> >    nsCOMPtr<nsICacheService> service =
> >      do_GetService(NS_CACHESERVICE_CONTRACTID, &rv);
> >  
> >    // Ensure the stream transport service gets initialized on the main thread
> > +  if (NS_SUCCEEDED(rv) && NS_IsMainThread()) {
> 
> The same as above. We cannot guarantee where is STS initialized when the
> first cache load is invoked from some background thread.

We guarantee that.  Only the http channel that is using this API fully only on the main thread also needs STS be initialized on the main thread.  So this code is OK.
(Assignee)

Comment 9

5 years ago
Created attachment 830799 [details] [diff] [review]
v1.1 -> 1.2 IDIFF

This is an interdiff on top of the current gum tree.  If necessary, I'll provide a full patch for review.

- taking care of initiating the old cache service and stream trans service on the main thread
- more explanatory comments added
Attachment #830799 - Flags: review?(michal.novotny)
(Assignee)

Comment 10

5 years ago
Created attachment 831067 [details] [diff] [review]
v2

- ENTRY_WANTED_COMPLETE renamed to RECHECK_AFTER_WRITE_FINISHED
- added more explanatory/correct comments
- special initiation of the old cache service and stream trans service on the main thread left to be added to the wyciwyg patch, since it's not needed for this patch be fully functional: wyciwyg is still using the old api and initialize the cache on the main thread (and is not using sts)
- fixed other review comments

cache2 on:  https://tbpl.mozilla.org/?tree=Gum&rev=ae6d25ad7724
cache2 off: https://tbpl.mozilla.org/?tree=Try&rev=dd92a14bd795
            https://tbpl.mozilla.org/?tree=Try&rev=7b66d8eaea88 (fixed head_cache2.js)
Attachment #829266 - Attachment is obsolete: true
Attachment #830799 - Attachment is obsolete: true
Attachment #830799 - Flags: review?(michal.novotny)
Attachment #831067 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Blocks: 938186
(Assignee)

Comment 11

5 years ago
Created attachment 831693 [details] [diff] [review]
v2 -> v2.1 idiff

- actually only updated the idl comments to reflect new changes at all and overall make clear how the callbacks may work

full patch is coming too.
Attachment #831067 - Attachment is obsolete: true
Attachment #831067 - Flags: review?(michal.novotny)
Attachment #831693 - Flags: review?(michal.novotny)
Attachment #831693 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/2b59776ddb86
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [cache2] → [cache2][qa-]
You need to log in before you can comment on or make changes to this bug.