Closed Bug 1359245 Opened 8 years ago Closed 8 years ago

Support multiple {CycleCollectedJS,XPCJS}Contexts per {CycleCollectedJS,XPCJS}Runtime

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(11 files, 1 obsolete file)

10.44 KB, patch
till
: review+
Details | Diff | Splinter Review
3.00 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.61 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
7.13 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
14.02 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
34.25 KB, patch
mccr8
: review+
sfink
: review+
Details | Diff | Splinter Review
12.25 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.75 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
17.84 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
1.60 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
40.74 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
Bug 1343396 split apart the context and runtime parts of XPCJSContext and CycleCollectedJSContext. However, I left in a good amount of code that assumed that there would be only one context per runtime. I'll be fixing those places in this bug, as well as adding the ability to have multiple contexts per runtime.
The promise callbacks are intended to run on the XPCJSContext, so the data that is associated with them should be per-context. We might as well make the callbacks themselves per-context as well.
Attachment #8861229 - Flags: review?(till)
Once we have multiple XPCJSContext's, we may have to figure out which one we're using with TLS. A later patch tries to remove as many of these TLS lookups as possible so that performance doesn't suffer.
Attachment #8861230 - Flags: review?(continuation)
The next couple patches remove various fields and methods for getting from a runtime to a (single) context.
Attachment #8861231 - Flags: review?(continuation)
This patch removes the last reference from runtime to context. To do this, I had to add some JS engine functionality that works directly on a runtime (rather than requiring a context). The alternative to doing this is to pass a JSContext through all the cycle collector tracing callbacks, which would require a ton of code changes. I also considered invoking the JSAPI functions here using a "main" context (which would likely be the first one created, and the last one destroyed). However, it would be a bug to pass such a context to Rooted, so that seems inherently dangerous to me. Steve, can you review the JS engine bits and Andrew the rest?
Attachment #8861242 - Flags: review?(sphink)
Attachment #8861242 - Flags: review?(continuation)
This patch reduces the number of TLS lookups we need to do by eliminating calls to XPCJSContext::Get(). That should help performance.
Attachment #8861246 - Flags: review?(continuation)
This patch adds initial support for cooperatively scheduled CycleCollectedJSContexts.
Attachment #8861247 - Flags: review?(continuation)
This patch keeps track of all a runtime's contexts using a linked list. It uses this list in a couple places where we need to iterate over all contexts during GC.
Attachment #8861248 - Flags: review?(continuation)
Comment on attachment 8861229 [details] [diff] [review] Move JS engine promise callbacks from JSRuntime to JSContext Review of attachment 8861229 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense, r=me
Attachment #8861229 - Flags: review?(till) → review+
Comment on attachment 8861230 [details] [diff] [review] Use TLS to return XPCJSContext::Get() Review of attachment 8861230 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSContext.cpp @@ +618,5 @@ > mTimeoutAccumulated(false), > mPendingResult(NS_OK) > { > + MOZ_RELEASE_ASSERT(!gTlsContext.get()); > + gTlsContext.set(this); gTlsContext should be cleared in ~XPCJSContext. ::: js/xpconnect/src/xpcprivate.h @@ +403,5 @@ > { > public: > static XPCJSContext* newXPCJSContext(); > + static XPCJSContext* Get(); > + static XPCJSContext* GetOnly() { /*return nsXPConnect::XPConnect()->GetContext();*/ return nullptr; } I guess this goes away in a later patch? Otherwise you should delete the comment and change this to a release assert.
Attachment #8861230 - Flags: review?(continuation) → review+
Comment on attachment 8861231 [details] [diff] [review] Eliminate nsXPConnect::GetContextInstance() Review of attachment 8861231 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsXPConnect.cpp @@ -158,5 @@ > -XPCJSContext* > -nsXPConnect::GetContextInstance() > -{ > - nsXPConnect* xpc = XPConnect(); > - return xpc->GetContext(); I guess you could have changed this to use XPCJSContext::Get(), but it is nice to eliminate one of the too many ways XPConnect has to access its top level data structures.
Attachment #8861231 - Flags: review?(continuation) → review+
Comment on attachment 8861234 [details] [diff] [review] Eliminate nsXPConnect::mContext Review of attachment 8861234 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsXPConnect.cpp @@ +98,5 @@ > > // shutdown the logging system > XPC_LOG_FINISH(); > > + delete XPCJSContext::Get(); I guess later patches deal with possible ways to destroy other XPCJSContexts. @@ +154,5 @@ > // static > XPCJSRuntime* > nsXPConnect::GetRuntimeInstance() > { > + MOZ_ASSERT(NS_IsMainThread()); RELEASE_ASSERT please, to match the old code path.
Attachment #8861234 - Flags: review?(continuation) → review+
Comment on attachment 8861236 [details] [diff] [review] Get rid of CycleCollectedJSRuntime::MainContext Review of attachment 8861236 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +1066,5 @@ > > #define JS_OPTIONS_DOT_STR "javascript.options." > > static void > ReloadPrefsCallback(const char* pref, void* data) |data| is now unused. Please change the two places that pass in |this| for |data| at the end of XPCJSRuntime::Initialize() to pass in null instead. @@ +2946,5 @@ > return nsXPConnect::GetRuntimeInstance(); > } > > void > +XPCJSRuntime::Initialize(JSContext* cx) Can't you pull |cx| out of TLS like you do in InitSingletonScopes()? Aren't we always initializing whatever the active thread is? Likewise in Shutdown. @@ -3096,5 @@ > #ifdef DEBUG > depth--; > XPC_LOG_ALWAYS(("XPCJSRuntime @ %p", this)); > XPC_LOG_INDENT(); > - XPC_LOG_ALWAYS(("mJSContext @ %p", MainContext())); I'd gripe more about the erosion of this method if I thought it was widely used. ;) ::: js/xpconnect/src/XPCWrappedJS.cpp @@ +468,5 @@ > XPCJSRuntime::AssertInvalidWrappedJSNotInTable(nsXPCWrappedJS* wrapper) const > { > #ifdef DEBUG > if (!wrapper->IsValid()) { > + JSContext* cx = XPCJSContext::Get()->Context(); Please move this declaration next to the use (unless you are going to use it sooner in some later patch...).
Attachment #8861236 - Flags: review?(continuation) → review+
The cycle collector also keeps a couple of pointers to CCJSContexts. Is that going to have to be changed to runtimes at some point, or is there going to be one CC per main thread?
Flags: needinfo?(wmccloskey)
Comment on attachment 8861242 [details] [diff] [review] Remove CycleCollectedJSRuntime::mJSContext Review of attachment 8861242 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the non-JS parts ::: js/src/jsapi.cpp @@ +566,5 @@ > return cx->runtime()->parentRuntime ? cx->runtime()->parentRuntime : cx->runtime(); > } > > +JS_PUBLIC_API(JSRuntime*) > +JS_GetRuntime(JSContext* cx) I guess this isn't called enough to warrant putting it on RootingContext along with an inlinable getter? ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +505,3 @@ > : mGCThingCycleCollectorGlobal(sGCThingCycleCollectorGlobal) > , mJSZoneCycleCollectorGlobal(sJSZoneCycleCollectorGlobal) > + , mJSRuntime(JS_GetRuntime(aCx)) You should probably assert that mJSRuntime is non-null.
Attachment #8861242 - Flags: review?(continuation) → review+
Comment on attachment 8861246 [details] [diff] [review] Eliminate some XPCJSContext::Get() usage Review of attachment 8861246 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsXPConnect.cpp @@ +397,5 @@ > > NS_IMETHODIMP > nsXPConnect::GarbageCollect(uint32_t reason) > { > + XPCJSRuntime::Get()->GarbageCollect(reason); This could just be mRuntime->GarbageCollect. ::: js/xpconnect/src/xpcprivate.h @@ +2504,2 @@ > #ifdef DEBUG > + jsid old = DebugOnly<jsid> instead of the guard while you are here?
Attachment #8861246 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #15) > The cycle collector also keeps a couple of pointers to CCJSContexts. Is that > going to have to be changed to runtimes at some point, or is there going to > be one CC per main thread? Oh I see this happens in a later patch.
Flags: needinfo?(wmccloskey)
Comment on attachment 8861242 [details] [diff] [review] Remove CycleCollectedJSRuntime::mJSContext Review of attachment 8861242 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/RootingAPI.h @@ +1056,5 @@ > AddPersistentRoot(RootingContext* cx, RootKind kind, PersistentRooted<void*>* root); > > +JS_PUBLIC_API(void) > +AddPersistentRoot(JSRuntime* rt, RootKind kind, PersistentRooted<void*>* root); > + Whoa. I guess with the cooperative threading stuff, we lost this? Ugh. @@ +1110,5 @@ > + void registerWithRootLists(JSRuntime* rt) { > + MOZ_ASSERT(!initialized()); > + JS::RootKind kind = JS::MapTypeToRootKind<T>::kind; > + AddPersistentRoot(rt, kind, reinterpret_cast<JS::PersistentRooted<void*>*>(this)); > + } I'm not thrilled with the duplication, but it's fine. template <class RootHolder> void registerWithRootLists(RootHolder* holder) { ... } is probably overkill (and worse for compile errors). ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +561,3 @@ > MOZ_ASSERT(!mDeferredFinalizerTable.Count()); > > + mJSRuntime = nullptr; What's the point setting a field to nullptr in a destructor? Please don't tell me something does ccrt->~CycleCollectedJSRuntime(); ccrt->reportShutdown(); or something else awful.
Attachment #8861242 - Flags: review?(sphink) → review+
Comment on attachment 8861247 [details] [diff] [review] Initial support for cooperative contexts Review of attachment 8861247 [details] [diff] [review]: ----------------------------------------------------------------- This mostly looks good. I just have a few quibbles about comments and variable names I'd like to see addressed in an updated patch, and I'd like to know a little more about how the CC is supposed to work in this context, as indicated below. ::: js/xpconnect/src/nsXPConnect.cpp @@ +1409,5 @@ > + > +void > +xpc::CreateCooperativeContext() > +{ > + XPCJSContext::NewXPCJSContext(gMainThreadContext); Maybe assert !gMainThreadContext here? @@ +1415,5 @@ > + > +void > +xpc::DestroyCooperativeContext() > +{ > + delete XPCJSContext::Get(); I think this have an assertion that XPCJSContext::Get() != gMainThreadContext. ::: js/xpconnect/src/xpcpublic.h @@ +624,5 @@ > +void > +DestroyCooperativeContext(); > + > +void > +YieldCooperativeContext(); For these methods, please add a comment referring to the relevant method in jsapi.h, like "See JS_YieldCooperativeContext in jsapi.h" to save people some indirection when trying to figure out what in the world these methods are supposed to do. ::: xpcom/base/CycleCollectedJSContext.cpp @@ +102,5 @@ > > mOwningThread->SetScriptObserver(nullptr); > NS_RELEASE(mOwningThread); > > + if (mIsCooperativeContext) { Shouldn't this be !mIsCooperativeContext? Only the uncooperative context creates a runtime. Also, please add a comment to mRuntime that says that this is a strong reference for the uncooperative context and weak for the cooperative contexts. Assuming that is what is happening. ::: xpcom/base/CycleCollectedJSContext.h @@ +73,5 @@ > CycleCollectedJSContext(); > virtual ~CycleCollectedJSContext(); > > MOZ_IS_CLASS_INIT > + void InitializeCommon(); This should be private. @@ +81,5 @@ > uint32_t aMaxBytes, > uint32_t aMaxNurseryBytes); > > + MOZ_IS_CLASS_INIT > + nsresult InitializeCooperative(CycleCollectedJSContext* aSiblingContext); You really need some description somewhere of what a cooperative CCJSContext is or refer to some comment in the JS engine. Why do we have these? What is a sibling context? Are all contexts siblings? Does more than one run at a time? The word "sibling" does not seem appropriate, as this relationship is neither symmetric nor transitive. Maybe "parent" or "primary"? Is this favored sibling different from the others in any way besides that it happened to be created first? ("first among equals") @@ +254,5 @@ > > private: > CycleCollectedJSRuntime* mRuntime; > > + bool mIsCooperativeContext; Maybe this is what the JS engine is doing already, but the terminology for this seems a little off to me as well. If I'm reading this correctly, the "main" context for the main thread will have false for this field? Isn't it cooperating with the other lesser contexts (ie they are all cooperative contexts)? Maybe you could reverse the polarity on this and call it "mIsPrimaryContext" or something? ::: xpcom/base/nsCycleCollector.cpp @@ +4055,5 @@ > > return sCollectorData.init(); > } > > +static nsCycleCollector* gMainThreadCollector; It would be good to initialize this to null. @@ +4071,5 @@ > > sCollectorData.set(data); > + > + if (NS_IsMainThread()) { > + gMainThreadCollector = data->mCollector; If you initialize this to null, then you could assert that it is null here. @@ +4087,5 @@ > + > + CollectorData* data = new CollectorData; > + > + data->mCollector = gMainThreadCollector; > + data->mContext = aCx; I don't understand from this patch how the CC is supposed to work in this setting. Is the CC only going to be run from the primary thread? I guess mContext needs to be set on other threads for CycleCollectedJSContext::Get(), and mCollector needs to be set for Suspect. If the CC is only run from the primary context, can it trace into JS objects from other contexts? @@ +4103,5 @@ > + // And we shouldn't have already forgotten our context. > + MOZ_ASSERT(data->mContext); > + // We should not have shut down the cycle collector yet. > + MOZ_ASSERT(data->mCollector); > + I guess there's no way to check if this is the "real" main thread or not.
Attachment #8861247 - Flags: review?(continuation) → review-
Comment on attachment 8861230 [details] [diff] [review] Use TLS to return XPCJSContext::Get() Review of attachment 8861230 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/xpcprivate.h @@ +403,5 @@ > { > public: > static XPCJSContext* newXPCJSContext(); > + static XPCJSContext* Get(); > + static XPCJSContext* GetOnly() { /*return nsXPConnect::XPConnect()->GetContext();*/ return nullptr; } Actually, looking over the later patches, I'd just leave GetOnly() alone until you get rid of it in the last patch. Otherwise, all of the intermediate patches will crash needlessly. It shouldn't be too hard to fix this to work when there's only a single context. If you don't want to deal with that, I'd just delete the method here, so the build will break, rather than crashing at the first GC or whatever.
Comment on attachment 8861248 [details] [diff] [review] Keep a linked list of CycleCollectedJSContexts in the runtime Review of attachment 8861248 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +661,5 @@ > } > > void XPCJSRuntime::TraceNativeBlackRoots(JSTracer* trc) > { > + for (const CycleCollectedJSContext* ccx = Contexts().getFirst(); ccx; I guess it clashes a little with the horrible janky iteration that XPConnect normally uses, but this might be nicer if you used range-based iteration. ::: xpcom/base/CycleCollectedJSContext.cpp @@ +141,5 @@ > return NS_ERROR_OUT_OF_MEMORY; > } > > mRuntime = CreateRuntime(mJSContext); > + mRuntime->AddContext(this); This should be in InitializeCommon(), it looks like. Or in XPCJSContext::Initialize(). ;) ::: xpcom/base/CycleCollectedJSContext.h @@ +66,5 @@ > uint32_t mNumSlices; > }; > > class CycleCollectedJSContext > + : public LinkedListElement<CycleCollectedJSContext> Could you do this for XPCJSContext instead of CCJSContext? (Including moving mContexts and the accessors down there.) I can't imagine that we'll actually want to support multiple JS contexts for DOM workers for quite some time, and it would let you avoid the cast when using mContexts.
Attachment #8861248 - Flags: review?(continuation) → review+
Just a few notes: - gMainThreadCollector is automatically initialized to null since it's a static variable. - You're right, I forgot about the mJSContext field in nsCycleCollector. I'll post a patch tomorrow to fix that. It looks pretty straightforward. I think this is reviewable without that though.
Attachment #8861247 - Attachment is obsolete: true
Attachment #8861685 - Flags: review?(continuation)
I discovered this problem after applying your suggestion to null out gTlsContext on shutdown. When we just had CycleCollectedJSContext (and no CycleCollectedJSRuntime) a weird thing happened at shutdown: 1. We would call JS_DestroyContext from ~CycleCollectedJSContext. By that time, the ~XPCJSContext destructor had already finished. 2. Destroying the context runs a final GC. That GC would call back into various GC callbacks, such as TraceBlackJS and TraceGrayJS. 3. These callbacks would do a virtual method call: http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/xpcom/base/CycleCollectedJSRuntime.cpp#791 4. Normally this method call would call into the XPCContext::TraceNativeBlackRoots. However, C++ changes the vtable for an object during destruction. So we would only call CycleCollectedJSContext's version of TraceNativeBlackRoots, which is empty. So we never traced anything. When I moved this code into the runtime, we actually do call into XPCJSRuntime::TraceNativeBlackRoots at that time. So the behavior changed, and that was causing crashes once I nulled out the TLS as you asked. So I removed these callbacks for the last GC.
Attachment #8861686 - Flags: review?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #22) > Could you do this for XPCJSContext instead of CCJSContext? (Including moving > mContexts and the accessors down there.) I can't imagine that we'll actually > want to support multiple JS contexts for DOM workers for quite some time, > and it would let you avoid the cast when using mContexts. I debated this, but it seems like we might want to know all the possible contexts from within CCJSContext. Just for GC purposes or something. We don't now, so it's not really important. I can do whatever you want.
Comment on attachment 8861686 [details] [diff] [review] Remove tracers on shutdown Review of attachment 8861686 [details] [diff] [review]: ----------------------------------------------------------------- This seems a little questionable, but I have the impression the JS engine isn't marking late in shutdown anyways, so hopefully it will be okay. I guess it isn't making anything worse.
Attachment #8861686 - Flags: review?(continuation) → review+
Comment on attachment 8861685 [details] [diff] [review] Initial support for cooperative contexts, v2 Review of attachment 8861685 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: js/xpconnect/src/XPCJSContext.cpp @@ +811,4 @@ > > +// static > +XPCJSContext* > +XPCJSContext::NewXPCJSContext(XPCJSContext* aSiblingContext) nit: aPrimaryContext ::: js/xpconnect/src/xpcprivate.h @@ +400,5 @@ > class XPCJSContext final : public mozilla::CycleCollectedJSContext > { > public: > + static void InitTLS(); > + static XPCJSContext* NewXPCJSContext(XPCJSContext* aSiblingContext); nit: aPrimaryContext @@ +488,5 @@ > private: > XPCJSContext(); > > MOZ_IS_CLASS_INIT > + nsresult Initialize(XPCJSContext* aSiblingContext); nit: aPrimaryContext
Attachment #8861685 - Flags: review?(continuation) → review+
(In reply to Bill McCloskey (:billm) from comment #25) > I debated this, but it seems like we might want to know all the possible > contexts from within CCJSContext. Just for GC purposes or something. We > don't now, so it's not really important. I can do whatever you want. If you think it could be useful at some point, that's fine.
This patch removes mJSContext from the cycle collector. I also ended up removing the pass-through functionality from context to runtime since it's not used as much now.
Attachment #8862617 - Flags: review?(continuation)
Comment on attachment 8862617 [details] [diff] [review] Remove context references from cycle collector Review of attachment 8862617 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for cleaning up the vestigial methods on context. ::: xpcom/base/nsCycleCollector.cpp @@ +1268,5 @@ > bool mScanInProgress; > CycleCollectorResults mResults; > TimeStamp mCollectionStart; > > + CycleCollectedJSRuntime* mCCRuntime; Please leave the JS in the name. Like mJSRuntime or mCCJSRuntime if you prefer. From the cycle collector's perspective, the distinctive thing about this is that it is the interface to the JS engine. @@ +1299,5 @@ > > public: > nsCycleCollector(); > > + void SetCCRuntime(CycleCollectedJSRuntime* aCCRuntime); Keep JS in the name of these two methods please.
Attachment #8862617 - Flags: review?(continuation) → review+
Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/90fccc166e44 Move JS engine promise callbacks from JSRuntime to JSContext (r=till) https://hg.mozilla.org/integration/mozilla-inbound/rev/12cbd8e08e62 Use TLS to return XPCJSContext::Get() (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/10cc100a7ee0 Eliminate nsXPConnect::GetContextInstance() (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/15304c93da0e Eliminate some XPCJSContext::Get() usage (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/b3be9a308906 Eliminate nsXPConnect::mContext (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ad80b18a0b Get rid of CycleCollectedJSRuntime::MainContext (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/fd29fdf5c245 Remove CycleCollectedJSRuntime::mJSContext (r=mccr8,sfink) https://hg.mozilla.org/integration/mozilla-inbound/rev/22e729c4596e Initial support for cooperative contexts (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/6a3c4b906eef Keep a linked list of CycleCollectedJSContexts in the runtime (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/92d90ef2a945 Remove some tracing callbacks at shutdown (r=mccr8) https://hg.mozilla.org/integration/mozilla-inbound/rev/665e26ed4e4d Remove references to context from the cycle collector (r=mccr8)
Depends on: 1373004
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: