413774 - Investigate whether XPConnect needs to suspend requests during calls to native methods.

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Johnny Stenback (:jst)]

Attachment: Move suspend/resume request code into thread context stack push/pop.

This is something that came up when profiling code and specifically looking at the time spent in XPConnect when calling from JS into C++. The testcase in question was a simple loop like this:

  var body = document.body;
  for (var i = 0; i < 1000000; i++) {
    var t = body.nodeType;
  }

looking at where the time is spent running that JS it showed that ~20% of the time is spent suspending and resuming JS requests (AutoJSSuspendRequest). In thinking about this and talking to brendan it's not obvious that we really need this. The point of suspending a request is to avoid deadlocks if we're calling into code that will run JS on other threads and waits for that JS to run to completion. If the JS on the other thread gets stuck waiting for the context on which we called into the other thread. I don't think there's a single case in Firefox where we'd do that.

So rather than paying for this for *every* call from JS into C++, I'd like to see whether we could not suspend requests around calls from JS into C++ and instead do it whenever C++ code does things that might take a long time (i.e. modal dialogs, etc). There may very well be cases in an extension or other embeddings where a JS request needs to be suspended, but ideally the code that needs it would deal with this rather than expecting XPConnect to do so for them.

I'm attaching a patch that removes the suspend/resume around calls from JS into C++ and makes the context stack's push/pop do that instead. That way if Firefox ever opens modal dialogs, does synchronous XHRs, or does a call from JS into C++ into JS on another context we'll suspend/resume the contexts.

If this is too big of an axe to drop at this point, we could also consider doing this only when called on the main thread, and only for objects that claim they're DOM objects through their class info, or something along these lines.

If anyone has thoughs on this, please speak up...

Comment 1

[Johnny Stenback (:jst)]

Plusing for now to keep this on the radar.

Comment 2

[John Gaunt (redfive)]

Songbird does do a lot of stuff off the main thread. I'll be happy to test out any patches on our stuff to work with you on this one.

Comment 3

[Johnny Stenback (:jst)]

That'd be great, John. Any chance you could give the attached patch a spin in Songbird?

Comment 4

[David Bradley]

I'm hoping to have some time this weekend to sit down and mentally walk some paths to see if there might be any potential problems. 

Comment 5

[John Gaunt (redfive)]

I built Songbird on Linux 64 with a XULRunner with the patch attached. No hangs, no lags. Ran all night no problem. Doesn't appear to have had any detrimental effects.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Same thing, with more consistent styling.

As decided in todays Gecko 1.9 meeting, we should get this in for further evaluation of whether we'll ship final with this or not.

Comment 7

[Brendan Eich [:brendan]]

In Gecko, with the DOM setting a GC callback to veto GC attempts by non-main threads, the only hazard here is taking too long within a request, possibly an indefinite amount of time (flying a dialog). But even if background threads want to GC, their failed attempts won't wait for the main thread to end its request, so there is no deadlock hazard to do with GC scheduling, AFAICT.

If there is an object exclusively owned (ownercx) by the main thread, and another thread wants to lock it, then that thread could have to wait an indefinite amount of time -- and if it held resources needed by the main thread, then you could get a deadlock. So there's an object-locking hazard. But again the main thread objects are mostly unshared (e.g., DOM).

Shared nothing is great, let's stick to it ;-).

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 300164 [details] [diff] [review]
Same thing, with more consistent styling.

And indeed, the context stack handles the dialog case nicely. This should be good all around. sr+'ing in advance of shaver's r+.

/be

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 300164 [details] [diff] [review]
Same thing, with more consistent styling.

r=shaver

Comment 10

[Johnny Stenback (:jst)]

Fix landed on trunk. Marking FIXED.