Open Bug 455548 (ST-XPConnect) Opened 11 years ago Updated 11 months ago

Investigate single-threading XPConnect and removing JS_THREADSAFE from the Mozilla

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

People

(Reporter: benjamin, Unassigned)

References

Details

Attachments

(3 obsolete files)

+++ 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.
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.
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.
Attached patch work in progress (obsolete) — Splinter Review
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.
Assignee: benjamin → igor
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/
(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.
Attached patch work in progress (obsolete) — Splinter Review
This is a version of the previous patch that compiles and runs.
Attachment #341456 - Attachment is obsolete: true
Attached patch work in progress (obsolete) — Splinter Review
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.
Attachment #341559 - Attachment is obsolete: true
Alias: ST-XPConnect
Depends on: 716167
Assignee: igor → nobody
bholley, is this bug useful to you or can we just mark it INCOMPLETE?
(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.
Depends on: 755255
Depends on: 766889
Depends on: 770535
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?
Flags: needinfo?(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.
Flags: needinfo?(luke)
(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. :-)
Attachment #342244 - Attachment is obsolete: true
It sounds like we basically need to wait for the EOL of our WinXP support to do this (see bug 773491).
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.
Bug 773491 got duplicated to Bug 956899. Has the situation changed now wrt WinXP support?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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?
(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.
Great to hear, thanks!
Depends on: 956899
You need to log in before you can comment on or make changes to this bug.