Split CycledCollectedJSContext and XPCJSContext into separate context and runtime classes

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Now that the JS engine supports using multiple JSContexts in different cooperatively scheduled threads, we have to do the same thing in XPConnect and XPCOM. This patch splits CycleCollectedJSContext into CycleCollectedJSContext and CycleCollectedJSRuntime. It does a similar thing for XPCJSContext.

The context classes contain members that are "call stack"-related. The runtime classes contain mostly GC-related stuff.

To reduce churn, I added helper methods to the context classes that just call into the runtime class. For the most part, I think we still want consumers of these classes to use contexts as much as possible. The runtime class should be more of an implementation detail. Eventually I would like to move most of the members of the runtime classes to something like the Zone or TabGroup, so I would like to avoid exposing it too much.

I structured the patch so that it looks like I copied the existing FooContext.cpp file into FooContext.cpp and FooRuntime.cpp and then deleted the unneeded parts from each one. I hope that helps make it easier to see the actual changes.

Olli, who do you think should review this? Can I give it to mccr8?
Flags: needinfo?(bugs)

Comment 1

2 years ago
mccr8 could review, if he has time.
Flags: needinfo?(bugs)
Created attachment 8842236 [details] [diff] [review]
patch
Attachment #8842236 - Flags: review?(continuation)
Duplicate of this bug: 1343395
Sorry, I haven't been following the JSContext splitting work in any detail. How does GC work in this setup?

How is the CC expected to work? The CC is set up per-context, and nsCycleCollector (and the purple buffer) is stored in TLS, so each thread will have its own independently running CC, but then you are moving mJSHolders onto the runtime, so each thread's cycle collector is going to be given roots from every thread.
Flags: needinfo?(wmccloskey)
I think it would be reasonable for all threads to share a single nsCycleCollector and a single purple buffer. This patch doesn't really address that though. It just splits up CycleCollectedJSContext.
Flags: needinfo?(wmccloskey)
Comment on attachment 8842236 [details] [diff] [review]
patch

Review of attachment 8842236 [details] [diff] [review]:
-----------------------------------------------------------------

This is a big change to XPConnect stuff. Maybe an XPConnect peer should at least glance at the patch?

::: js/xpconnect/src/XPCJSContext.cpp
@@ +790,5 @@
>      }
>  
> +    XPCJSRuntime* xpcrt = self->Runtime();
> +    if (self->Context()                          &&
> +        xpcrt->GetMultiCompartmentWrappedJSMap() &&

It seems weird that we're checking that the runtime is initialized every time we create a context. However, in this particular case, I think these maps are all infallibly allocated, so I guess this is fine. I filed bug 1344025 for removing all of these checks.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +652,3 @@
>  {
> +    // For now we only have one context. Eventually we'll need to iterate over
> +    // all contexts.

Like I said elsewhere, it feels it should be on the runtime not the context. That would also make it so that this bit of code won't just weirdly break when somebody adds support for multiple contexts in the future. Failing that, maybe have some kind of XPCJSContext::GetOnly() that provides a central place for any code that will need to be changed in the future to handle multiple contexts?

@@ +2815,5 @@
>  {
>  }
>  
> +/* static */
> +XPCJSRuntime *

please remove the space while you are here

@@ +2926,4 @@
>  }
>  
>  bool
> +XPCJSRuntime::InitializeStrings(JSContext* cx)

This function is goofy, but it looks like a preexisting problem so I filed bug 1343946 about it.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +73,5 @@
>  }
>  
>  nsXPConnect::~nsXPConnect()
>  {
> +    mContext->Runtime()->DeleteSingletonScopes();

It seems like nsXPConnect should have a field for Runtime() and just use that directly.

::: js/xpconnect/src/xpcprivate.h
@@ +400,5 @@
>  private:
>      mozilla::Maybe<StringType> mStrings[2];
>  };
>  
> +class XPCJSRuntime;

You already forward declared this above. Please remove both forward declarations, and add it to XPCForwards.h.

@@ +406,3 @@
>  class XPCJSContext final : public mozilla::CycleCollectedJSContext
>  {
> +    friend class XPCJSRuntime;

You should put the friend classes together, whether it is at the top or the bottom.

@@ +422,5 @@
>      jsid GetResolveName() const {return mResolveName;}
>      jsid SetResolveName(jsid name)
>          {jsid old = mResolveName; mResolveName = name; return old;}
>  
>      XPCWrappedNative* GetResolvingWrapper() const {return mResolvingWrapper;}

I'm not too clear on the split between what stays on the context and what goes to the runtime. Though really I'm not even sure what this thing is.

@@ +436,5 @@
> +    ~XPCJSContext();
> +
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf);
> +
> +    AutoMarkingPtr**  GetAutoRootsAdr() {return &mAutoRoots;}

nit: extra space before Get

@@ +535,5 @@
> +};
> +
> +class XPCJSRuntime final : public mozilla::CycleCollectedJSRuntime
> +{
> +    friend class XPCJSContext;

Please put all of the friend class declarations for this class together.

@@ -590,3 @@
>  
> -    XPCCallContext*          mCallContext;
> -    AutoMarkingPtr*          mAutoRoots;

I think |mAutoRoots| should be on the runtime. It is part of the GC tracing infrastructure.

::: xpcom/base/CycleCollectedJSContext.cpp
@@ +95,5 @@
>    mOwningThread->SetScriptObserver(nullptr);
>    NS_RELEASE(mOwningThread);
> +
> +  delete mRuntime;
> +  mRuntime = nullptr;

Aren't there multiple contexts and one runtime? I'd have thought that we spawn a context from a runtime, but we're destroying the runtime when we destroy the context, and creating a runtime when we create a context. Are you just assuming they are 1:1 for now and some later patch will fix this? (I asked Bill in person, and he said that later patches would fix this.)

::: xpcom/base/CycleCollectedJSContext.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_CycleCollectedJSContext_h
> +#define mozilla_CycleCollectedJSContext_h

Thanks for fixing the trailing underscores.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ -553,5 @@
>    js::SetScriptEnvironmentPreparer(mJSContext, &mEnvironmentPreparer);
>  
> -  JS::SetGetIncumbentGlobalCallback(mJSContext, GetIncumbentGlobalCallback);
> -
> -  JS::SetEnqueuePromiseJobCallback(mJSContext, EnqueuePromiseJobCallback, this);

Like we discussed, it seems like these APIs need to get adjusted on the JS engine side so they are stored per-context, and not per-runtime. It does make sense conceptually that they are per-context, but the way this is now we end up storing a pointer to the CC context on the JS runtime, which makes no sense. It is probably better not to fix it here, but at least file a followup bug for it. (I suppose for SetGetIncumbentGlobalCallback it doesn't matter much.)

@@ +1137,5 @@
>    mPreservedNurseryObjects.Clear();
>  }
>  
>  void
> +CycleCollectedJSRuntime::NurseryWrapperAdded(nsWrapperCache* aCache)

Are nurseries per-context? It might make sense to track these things separately. I guess it can get moved over if it somehow is an issue that shows up in profiles.

::: xpcom/base/CycleCollectedJSRuntime.h
@@ -286,5 @@
>      // now and haven't crashed; the OOM was probably handled correctly".
>      Recovered
>    };
>  
> -private:

Why did you make these public? Doesn't the friend stuff cover calling them from the context, assuming that is what you are doing?

@@ +274,5 @@
>    // whichever was most recent. If there were no such zones, prepare for a
>    // full GC.
>    void PrepareWaitingZonesForGC();
>  
> +  JSContext* MainContext() const

Could you explain here or somewhere what a "main context" is?

@@ -476,5 @@
>    };
>    EnvironmentPreparer mEnvironmentPreparer;
>  };
>  
> -void TraceScriptHolder(nsISupports* aHolder, JSTracer* aTracer);

I think that TraceScriptHolder and AddToCCKind should be in the runtime file, not the context file, because they are more about GC/CC stuff. Maybe also GetBuildId, which feels pretty global. (I have no idea why it is in this file...)
Attachment #8842236 - Flags: review?(continuation) → review+
Comment on attachment 8842236 [details] [diff] [review]
patch

Review of attachment 8842236 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSContext.cpp
@@ -195,5 @@
> -namespace xpc {
> -
> -CompartmentPrivate::CompartmentPrivate(JSCompartment* c)
> -    : wantXrays(false)
> -    , allowWaivers(true)

I don't understand - where does all this stuff move to? I only see it being removed in this patch.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> I don't understand - where does all this stuff move to? I only see it being
> removed in this patch.

The things that are deleted from XPCJSContext.cpp are in XPCJSRuntime.cpp, but not deleted. And vice versa.

copy from js/xpconnect/src/XPCJSContext.cpp
copy to js/xpconnect/src/XPCJSRuntime.cpp
(In reply to Andrew McCreight [:mccr8] from comment #8)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> > I don't understand - where does all this stuff move to? I only see it being
> > removed in this patch.
> 
> The things that are deleted from XPCJSContext.cpp are in XPCJSRuntime.cpp,
> but not deleted. And vice versa.
> 
> copy from js/xpconnect/src/XPCJSContext.cpp
> copy to js/xpconnect/src/XPCJSRuntime.cpp

Oh I see - I guess splinter is just bad at that.
In general it's a bit too bad that we recently renamed all the XPCJSRuntime stuff to XPCJSContext, and now we're going back again. The plus side is that my memory of where things are is accurate again. :-)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> In general it's a bit too bad that we recently renamed all the XPCJSRuntime
> stuff to XPCJSContext, and now we're going back again. The plus side is that
> my memory of where things are is accurate again. :-)

Well, it's not a perfect swap. XPCJSContext didn't exist before. Now we'll have both.
(In reply to Bill McCloskey (:billm) from comment #11)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> > In general it's a bit too bad that we recently renamed all the XPCJSRuntime
> > stuff to XPCJSContext, and now we're going back again. The plus side is that
> > my memory of where things are is accurate again. :-)
> 
> Well, it's not a perfect swap. XPCJSContext didn't exist before. Now we'll
> have both.

Well, there was something called XPCJSContext back when we had multiple JSContexts per runtime. This is basically reintroducing something analogous, right?
Oh, I guess I must have forgotten. You're not thinking of XPCCallContext, are you? That still exists too, but it kind of represents a frame rather than a stack.
No, there was an XPCContext which was the private state that XPConnect stored on a JSContext.

Comment 15

a year ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bdfa44ce843
Split CycledCollectedJSContext and XPCJSContext into separate context and runtime classes (r=mccr8)

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bdfa44ce843
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(boo me. I should have been faster. Need to do tons of merging now to fix microtasks.)
(In reply to Olli Pettay [:smaug] from comment #17)
> (boo me. I should have been faster. Need to do tons of merging now to fix
> microtasks.)

Sorry about that. Can you point me to the bug where you're working on microtasks?
It is bug 1193394. Not much happening there in the bug while I'm fixing tons of tests and browser UI code to follow microtask model.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2a3099466ac32db17260eafff53850a710d9917 is some recent try push, I assume still tens of failures.
You need to log in before you can comment on or make changes to this bug.