Closed Bug 1279295 Opened 3 years ago Closed 3 years ago

Allocate JSContext when we create the runtime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Flags: needinfo?(jcoppeard)
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.
(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...
Flags: needinfo?(jcoppeard)
(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 :(
(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).
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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8763511 - Flags: review?(luke)
Attachment #8763511 - Flags: review?(luke)
Attached patch PatchSplinter Review
Attachment #8763511 - Attachment is obsolete: true
Attachment #8763577 - Flags: review?(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.
Attachment #8763577 - Flags: review?(luke) → review+
(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..
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca871e39a20
Create the runtime's JSContext when we create the runtime. r=luke
https://hg.mozilla.org/mozilla-central/rev/0ca871e39a20
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1282113
Depends on: 1283169
Duplicate of this bug: 659799
Depends on: 1287395
Depends on: 1287399
Depends on: 1326539
You need to log in before you can comment on or make changes to this bug.