Closed Bug 1206079 Opened 6 years ago Closed 5 years ago

FetchDriver::Fetch() does unnecessary async thread dispatch


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

Not set





(Reporter: bkelly, Assigned: jsnajdr)



(Whiteboard: [good first bug][lang=c++])

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

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.
Nikhil, do you agree with comment 0?
Flags: needinfo?(nsm.nikhil)
See Also: → 1196592
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.
Flags: needinfo?(nsm.nikhil)
Thanks Nikhil.  I think making the non-channel cases callback asynchronously would be preferable.
I'd be happy to test the patch!
Blocks: 1207265
No longer blocks: ServiceWorkers-B2G
Is this still available to work on?
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.

Flags: needinfo?(bkelly)
Thanks bkelly. I'll get started once you summarize.
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:

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:

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?
Flags: needinfo?(bkelly) → needinfo?(stefandye)
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.
Flags: needinfo?(stefandye)
Assignee: nobody → jsnajdr
Blocks: 1278778
Solved as part of bug 1278778.
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1278778
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.