1206079 - FetchDriver::Fetch() does unnecessary async thread dispatch

Status: RESOLVED DUPLICATE of bug 1278778

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently FetchDriver::Fetch() dispatches to the current thread immediately:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#89

This is unnecessary because ContinueFetch() is already async due to using nsIChannel::AsyncOpen().

I think we should change FetchDriver::Fetch() so that the steps in ContinueFetch() happen directly in Fetch().  We should remove the InternalRequest::IsSynchronous() flag since we don't support it.

I think this could explain some of the slowdown in fetch() under load noted in bug 1196592.  Therefore blocking v2 for b2g perf issue.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Nikhil, do you agree with comment 0?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

While I agree for anything using channels, there are cases like BasicFetch() for about/data/blob URIs where we create the response and call the observer synchronously and this will become fully synchronous if we make the call to ContinueFetch() synchronous. Our current fetch() API callers will be fine with this since they just delegate to promises, but do update the (non-existent) FetchObserver docs to point out that the observer must be prepared for both cases. Another option is to make those individual cases async.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Thanks Nikhil.  I think making the non-channel cases callback asynchronously would be preferable.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'd be happy to test the patch!

Comment 5

[Stefan Dye]

Is this still available to work on?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

It is still available.  Need info'ing myself to look and see what needs to be done here now.  I think some things have changed since this was written.  I'll try to summarize the work needed on Monday.

Thanks.

Comment 7

[Stefan Dye]

Thanks bkelly. I'll get started once you summarize.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the delay here.  Its been quite hectic trying to get some things fixed before the holiday.

So the main issue here is this runnable dispatch:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#84

Instead of creating the runnable and calling NS_DispatchToCurrentThread(), I think we should just call ContinueFetch().

However, there are certain paths triggered by ContinueFetch() which can call back into the FetchDriverObserver synchronously.  We need to avoid doing that since I think the WorkerFetchResolver may not expect it.

So in the FetchDriver where we call out to mObserver:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#398
  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#409
  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#670

We need to check to see if we are in a synchronous path with the original Fetch call or not.

To do this I would add an mSync bool member variable to FetchDriver.  You can then do something like this at the top of FetchDriver::Fetch():

  AutoRestore<bool> syncRestore(mSync);
  mSync = true;

Then, before calling out to mObserver check to see if mSync is true.  If it is, then you need to dispatch a runnable to the current thread to call the mObserver method.

Does that make sense?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Alternatively, we could put the sync check and conditional dispatch in WorkerFetchResolver itself.  That might be easier to do.

The other implementation, MainThreadFetchResolver should be able to handle synchronous callbacks without problem.  Its just posting the result to a promise which does its own async handling.

If you go this route, I would just add a comment to FetchDriver.h indicating that Fetch() can sometimes call back to itself synchronously.

Comment 10

[Jarda Snajdr [:jsnajdr]]

Solved as part of bug 1278778.