1279295 - Allocate JSContext when we create the runtime

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We can remove JS_NewContext and JS_DestroyContext now if we make JSRuntime responsible for allocating/destroying its JSContext.

A new (infallible) JS_GetContext API will be added to get the runtime's context. This function will probably be temporary: it can be removed once there's a single type exposed to the API.

Comment 1

[Jan de Mooij [:jandem]]

The first issue is that the testOOM jsapi-test will do *much* more work with this patch. maxAllocsPerTest is 100 currently but it has to be > 4000 when we allocate the JSContext under JS_NewRuntime. That's because we initialize the self-hosting compartment, static strings, etc when we create the context.

Bumping maxAllocsPerTest makes the test really slow (from seconds to minutes).

Jon, any ideas?

Comment 2

[Till Schneidereit [:till]]

Would it work to run the test in a worker? Then we could create a parent runtime on the main thread and not do all the costly initialization under the test's JS_NewRuntime.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #2)
> Would it work to run the test in a worker? Then we could create a parent
> runtime on the main thread and not do all the costly initialization under
> the test's JS_NewRuntime.

That could be an option, but it's a bit annoying to set up the thread infrastructure as part of this jsapi-test..

I just realized there's a more serious problem with fusing JS_NewRuntime and JS_NewContext: we currently set certain options (JIT flags, GC parameters, etc) on the runtime before we call JS_NewContext. We could pass these options as arguments to JS_NewRuntime, but that's not great.

We could make the new JS_GetContext function (see comment 0) fallible (it'd initialize the context if needed), but that's not great long-term because we want to merge the two at some point...

Comment 4

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Till Schneidereit [:till] from comment #2)
> > Would it work to run the test in a worker? Then we could create a parent
> > runtime on the main thread and not do all the costly initialization under
> > the test's JS_NewRuntime.
> 
> That could be an option, but it's a bit annoying to set up the thread
> infrastructure as part of this jsapi-test..

Agreed. We could probably move much of EvalInWorker from js.cpp to TestingFunctions.cpp to make that part simpler.
> 
> I just realized there's a more serious problem with fusing JS_NewRuntime and
> JS_NewContext: we currently set certain options (JIT flags, GC parameters,
> etc) on the runtime before we call JS_NewContext. We could pass these
> options as arguments to JS_NewRuntime, but that's not great.

Yeah, that is indeed a more serious problem :(

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Jon, any ideas?

No, apart from parallelising the jsapi tests (not really a serious suggestion).

Comment 6

[Jan de Mooij [:jandem]]

We have a lot of per-runtime options/callbacks and it's not clear which of them *need* to be set before we initialize s-h code. Also, other embedders may use these options/callbacks for other things.

So, thinking about this more, it's probably best (at least for now) to make s-h initialization explicit with something like JS::InitSelfHosting.

That's an extra API call for embedders, but it's something we can MOZ_RELEASE_ASSERT, and it gives embedders most control over what happens in which order.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

This removes the JS_NewContext, JS_DestroyContext{NoGC}, JS_ContextIterator APIs. It also removes the context linked list.

JS::InitSelfHostedCode is used to initialize the self hosted code and is called where we used to create the first context. This avoids all problems mentioned earlier.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8763577 [details] [diff] [review]
Patch

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

Looking good!  I was going to suggest to instead do InitSelfHosting implicitly from within JS_GetContext(), the first time it was called, but since the goal-state is to remove JSRuntime from the JSAPI, then JS_GetContext will go away leaving only the InitSelfHosting() calls which would stay at roughly the same point the are in this patch.  It'd be nice to remove this entirely and have self-hosting get initialized when the JSContext+JSRuntime is first created, but that seems like a future task.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #9)
> I was going to suggest to instead do InitSelfHosting
> implicitly from within JS_GetContext(), the first time it was called, but
> since the goal-state is to remove JSRuntime from the JSAPI, then
> JS_GetContext will go away leaving only the InitSelfHosting() calls which
> would stay at roughly the same point the are in this patch.

Yup, I considered doing it in JS_GetContext, but it's just more work later.

> It'd be nice to
> remove this entirely and have self-hosting get initialized when the
> JSContext+JSRuntime is first created, but that seems like a future task.

Yes, once we have some way to pass all relevant runtime options to JS_NewRuntime..

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca871e39a20
Create the runtime's JSContext when we create the runtime. r=luke

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ca871e39a20