455548 - (ST-XPConnect) Investigate single-threading XPConnect and removing JS_THREADSAFE from the Mozilla

Status: NEW

(Core :: XPConnect, defect)

Description

[Benjamin Smedberg]

+++ This bug was initially created as a clone of Bug #454435 +++

I'm splitting this off into a separate bug to discuss single-threading xpconnect.

Comment 1

[Igor Bukanov]

Brendan Eich wrote in bug 454435 comment 24:

> We need a way for such platform customers to cope. One step forward would be to leave JS_THREADSAFE on but still use a runtime per worker. The main thread and any plugin-created threads would share the main (currently only) runtime used by main-thread XPConnect.

Given the already existing code that shares JS things between threads I think the first priority should be creating API that would allow to re-implement the desired functionality with mostly thread-unsafe JS runtime. That is, such library should allow for that code to work with a special build of the browser that would assert on any cross-threading JS object, string or double access.

Comment 2

[Igor Bukanov]

My current thinking is to create a special version of XPConnect just to cover the workers need of accessing their limited API. This should allow to create separated runtimes for each worker removing all lock-contention problems. It would also verify the message-passing approach of the workers API.

Comment 3

[Igor Bukanov]

Attachment: work in progress

The idea behind the patch is to allow to instantiate a thread-private copy of xpconnect. For that the patch moves all static variables in xpconnect into the single class XPCGlobals and changes the code to use special access methods for the class.

This patch does not change yet dom workers to use private xpconnect, that is for the next version.

Comment 4

[Benjamin Smedberg]

I already have a thread-private xpconnect, but that doesn't really help: not only can the "normal" xpconnect be shared across threads, but the DOM workers are managed by a thread pool and don't always run on the same physical thread, so thread-private data structures don't help at all AFAICT.

http://hg.mozilla.org/users/bsmedberg_mozilla.com/xpcthreading/

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> I already have a thread-private xpconnect, but that doesn't really help: not
> only can the "normal" xpconnect be shared across threads,

But such sharing is fine: the idea is to use a private one only with threads that wants one defaulting otherwise to the shared global.

> but the DOM workers
> are managed by a thread pool and don't always run on the same physical thread,
> so thread-private data structures don't help at all AFAICT.

But this is again fine: the idea is to store the private xpconnect structure in the dom worker and associate it with the current thread when it runs.

Comment 6

[Igor Bukanov]

Attachment: work in progress

This is a version of the previous patch that compiles and runs.

Comment 7

[Igor Bukanov]

Attachment: work in progress

The new patch uses separated XPConnect instance per thread worker. It compiles but I have not tested beyond that. The remaining thing is to teach the cycle collector about the possibility of several JS runtimes. Without that the cycles formed through the JS objectes on a thread worker would be alive until the worker will be shut down.

Comment 8

[Benjamin Smedberg]

bholley, is this bug useful to you or can we just mark it INCOMPLETE?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> bholley, is this bug useful to you or can we just mark it INCOMPLETE?

Once the assertion from bug 716167 gets sufficiently far (and once I have a bit more time), I'm going to rip all the thread-safe code out of XPConnect. This bug would work for that, but I can also easily file another one too. Your call.

Comment 10

[Bobby Holley (:bholley)]

So, the locks and most of the other threading goop in XPConnect are basically gone at this point. Is there anything that needs to be done on the JS engine side, or can we basically resolve this?

Comment 11

[Luke Wagner [:luke]]

Everyone would love to get rid of JS_THREADSAFE in the browser (bug 720018), but it is still necessary for people to build the JS shell when they don't have NSPR.  There is bug 773491 to address this (we already have thin wrappers for POSIX, so perhaps all we need is Windows wrappers).  If you can poke people on those bugs, I don't know of any other major impediments for removing JS_THREADSAFE.  We've already switched to building JS_THREADSAFE builds by default recently.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #11)
> Everyone would love to get rid of JS_THREADSAFE in the browser (bug 720018),
> but it is still necessary for people to build the JS shell when they don't
> have NSPR.  There is bug 773491 to address this (we already have thin
> wrappers for POSIX, so perhaps all we need is Windows wrappers).  If you can
> poke people on those bugs

This isn't a project I'm likely to drive. Someone with the interest and the high-level view should probably do the poking. :-)

Comment 13

[Bobby Holley (:bholley)]

It sounds like we basically need to wait for the EOL of our WinXP support to do this (see bug 773491).

Comment 14

[Luke Wagner [:luke]]

Oh great, so never.  I wonder if, with all the recent buildsystem improvements to js/src if it wouldn't be easier to just build NSPR in with the SM build, like we do with mfbt et al.

Comment 15

[Florian Bender]

Bug 773491 got duplicated to Bug 956899. Has the situation changed now wrt WinXP support?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Florian Bender from comment #15)
> Bug 773491 got duplicated to Bug 956899. Has the situation changed now wrt
> WinXP support?

I don't believe we have any plans to remove support for WinXP.

Comment 17

[Florian Bender]

Sorry for the confusion, I didn't intend to inquire about WinXP support but: Is WinXP support still a blocker for this bug with Bug 956899 in the making?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Florian Bender from comment #17)
> Sorry for the confusion, I didn't intend to inquire about WinXP support but:
> Is WinXP support still a blocker for this bug with Bug 956899 in the making?

IIUC, if we do that bug, then we can land this.

Comment 19

[Florian Bender]

Great to hear, thanks!