GC only on the main thread
Benjamin Smedberg
our XPCOM+XPConnect platform that I've committed a change in bug 409433 so
that xpconnect prevents spidermonkey GC from occurrin on any thread other
than the main thread by default. I did this because cycle collection
requires the GC callbacks to happen on the main thread, and I couldn't see a
reason to leave this as a configurable option.
Gecko (the DOM engine) already set this option, so this change would only
affect clients who use xpconnect but not the DOM.
--BDS
Jeff Walden
> Not sure there's a good group for this, but I'm writing to inform clients of
> our XPCOM+XPConnect platform that I've committed a change in bug 409433 so
> that xpconnect prevents spidermonkey GC from occurrin on any thread other
> than the main thread by default. I did this because cycle collection
> requires the GC callbacks to happen on the main thread, and I couldn't see a
> reason to leave this as a configurable option.
Doesn't this prevent using single-threaded XPCOM components in multi-threaded JS? Sounds like the component and wrapper would be created on the non-main thread, but destruction would happen on the main thread and produce unavoidable (!) threadsafety assertions.
Jeff
Dave Townsend
Jeff Walden
> This would be what I hit with
> https://bugzilla.mozilla.org/show_bug.cgi?id=337648 no?
That's what inspired the question, but the current situation is just XPConnect not doing things as correctly as it could with changes which would fix that bug -- what's proposed here is institutionalizing that broken behavior, with the twist that GC would be limited to the main thread, not any arbitrary thread.
Jeff
Benjamin Smedberg
I don't think it is ever safe to use single-threaded objects from off the
main thread in xpconnect. This change really doesn't make it any worse,
because previously GC could be on any thread... now it has to be on the main
thread, but it's unlikely that it was ever going to happen on the thread the
wrapper existed on.
--BDS
Dave Townsend
So basically you are saying that you should never run js on a background
thread unless every single object you use is thread safe?
Dave
Benjamin Smedberg
> So basically you are saying that you should never run js on a background
> thread unless every single object you use is thread safe?
Well, "thread safe" means many different things. What I mean is, if you
touch an object from xpconnect, you must:
* Make sure that that the methods you call are safe to call from the thread
you're calling them from
* It's safe to perform the final release of the object from the main thread
Note that you don't have to make every method of the object threadsafe, and
you don't even have to have threadsafe QI.
--BDS
Jeff Walden
> * Make sure that that the methods you call are safe to call from the thread
> you're calling them from
> * It's safe to perform the final release of the object from the main thread
>
> Note that you don't have to make every method of the object threadsafe, and
> you don't even have to have threadsafe QI.
That could be reasonable, actually, on second thought -- so long as we re-jigger the NS_IMPL_RELEASE* macros to only do the owning-thread assert if the reference count hasn't dropt to zero, which I hadn't previously thought was doable. With that change I can't think of any other problems we'd hit.
Actually, is there a good reason not change the macro on trunk? I think this change would eliminate the only assertions I'm aware of when using single-threaded components on a single thread in JS when the owning-thread check happens on final release of the wrapper during GC, which would make it possible to actually use threads in JS in code where we'd want to treat assertions as fatal (e.g. the HTTP server as used in the xpcshell tests, where assertions are fatal errors).
Jeff
Boris Zbarsky
> That could be reasonable, actually, on second thought -- so long as we
> re-jigger the NS_IMPL_RELEASE* macros to only do the owning-thread
> assert if the reference count hasn't dropt to zero
I'm not sure I follow. The assert should really be that references to
the object are only held on a single thread (because the whole point is
that the code is not doing atomic decrements when releasing here).
Basically, anything used through JS on a background thread needs to have
threadsafe refcounting. This falls under the "Make sure that that the
methods you call are safe to call from the thread you're calling them
from" item of Benjamin's 2-item list.
-Boris
Jeff Walden
The problem is that sometimes you'd like to use something that doesn't have threadsafe refcounting in JS on a background thread. You can do this in C++, no problems, if you're careful. However, you can't in JS even if you are careful, because the final release will happen from whatever thread GC runs on, even a thread other than the one where you created/used the object. It's completely out of your control, no matter how careful you are to only keep a reference on a single thread, if GC might ever run on another thread. However, if you could somehow ensure all references but the last one are released from the original thread, it wouldn't matter where the last one's released, because it can't race another release.
That said, what I suggested doesn't seem to be enough, at least in the following two cases where I get the threadsafety assertion twice (!) when I only expected it once:
(function() { var q = Components.classes["@mozilla.org/supports-string;1"].createInstance(Components.interfaces.nsISupportsString) })()
t = Components.classes["@mozilla.org/thread-manager;1"].getService()
th = t.newThread(0).dispatch({run: function() { gc(); }}, 0)
while (t.currentThread.hasPendingEvents()) t.currentThread.processNextEvent(true);
// ... repeat until the assertions fire
var r = (function() {
var q = Components.classes["@mozilla.org/supports-string;1"].createInstance(Components.interfaces.nsISupportsString);
q = null;
});
t = Components.classes["@mozilla.org/thread-manager;1"].getService()
th = t.newThread(0)
th.dispatch({run: r}, 0);
while (t.currentThread.hasPendingEvents()) t.currentThread.processNextEvent(true);
// ...five or six times so the event is run on the new thread
gc()
If my suggestion doesn't actually help in either of these cases, I suspect something I don't know about in XPConnect would make it fail on real-world cases as well, in which case the suggestion is useless.
Jeff
Boris Zbarsky
> The problem is that sometimes you'd like to use something that doesn't
> have threadsafe refcounting in JS on a background thread.
The problem is that then you have to ensure it never gets refcounted on any
other thread. JS can't make that guarantee. Most simply, the cycle collector
can refcount your object, and that runs on the main thread.
> You can do this in C++, no problems, if you're careful.
Right, because there are no algorithms outside your control (like GC and cycle
collection in the JS case) that traverse the entire object graph on threads
other than your one chosen thread.
> However, you can't in JS
> even if you are careful, because the final release will happen from
> whatever thread GC runs on
It's not just a problem of final release. If your addref/release are not
threadsafe, then you can't ever do refcounting on more than one thread "at
once". That's very difficult to guarantee with an object reflected into JS
that's refcounted by someone from off the main thread.
> It's completely out of your control, no matter
> how careful you are to only keep a reference on a single thread, if GC
> might ever run on another thread.
Right.
> However, if you could somehow ensure
> all references but the last one are released from the original thread,
And that all addrefs are performed on the original thread, right?
> it wouldn't matter where the last one's released, because it can't race
> another release.
It can race an addref, no?
-Boris