Closed Bug 650361 Opened 14 years ago Closed 8 years ago

Refactor JSContext/JSRuntime

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: luke, Assigned: jandem)

References

Details

With single-thread JSRuntime (bug 649537), removing cx->globalObject (bug 604813), the JSContext-flag-removing work cdleary has done and has in patches, JSContext has very little meaning other than "a thread-like thing".  Furthermore, because there is no way to set aside or suspend an active C stack, the only form of switching between threads is synchronous reentrence.  Thus, there should be little loss of embedder flexibility to removing JSContext entirely and having a single implicit (accessed via TLS (bug 650357)) stack corresponding to JSThreadData.  This would certainly simplify some engine logic but I think there is a bigger win in removing what is now a widespread hazy and confusing-to-newcomers JS concept.
Blocks: 650357
Summary: perhaps merge JSContext into JSThreadData? → rm JSContext
Very V8-like. I mean bug 650357, I guess!

A big change for embedders, though. Server-side and other embeddings want shared Runtime (process wide, Unix sense), JSThreadData (lock-free and OS thread-local), and JSContext (stack or execution environment recursive list).

Truly we have broken the "coroutined" use of multiple contexts on one stack if they must share the same thread-local JS stack in a LIFO fashion. No hope there for the patches from Joachim Kuebert to make us "stackless"?

There is a loss in flexibility here: if you have only one implicit TLS-mapped data structure, you can't nest on the native (OS thread) stack using different contexts, which can be done today (correct me if I'm wrong).

Having disjoint stack backtraces even on the same OS thread, when nesting flows on two or more contexts, can be important. Or have we broken that?

/be
Probably we should talk in m.d.t.js-engine or dev-tech-js-engine-internals first.

/be
(Joachim, apologies for misspelling your last name in comment 1!)
Our server side embedding uses shared runtime and potentially multiple JSContext per OS thread. Broken would be an understatement.  It would leave us with no where to go.  No upgrade path.
Why does this bug block bug 604813 and bug 649537 (and bug 650357 for that matter)? Maybe it's a preference to sort this out and avoid longer-path rework, but those seem fixable without rm'ing JSContext.

/be
(In reply to comment #4)
> Our server side embedding uses shared runtime and potentially multiple
> JSContext per OS thread.

The question is whether you really need many cx'es per OS thread -- do you nest control flows on them purely last-in-first-out, no "coroutining"?

What do you use multiple cx's for, could you remind me? Thanks.

/be
Here's one example.  (We have several scenarios but I can't remember them all without more digging)

A server side JS script is running in a web server on context A.
The script needs to check security for something by polling "decision voters".
who get to say nay or yay to the request.

The top level script suspends its request and then calls a security manager which fires pre-compiled function on a security global to "vote".  The security manager has its own global and can either pull a new context from a pool or use one it already has to run the js function.  For security reasons we cannot share a global or context with the top level script.
The security manager & its global with compiled functions lives as long as the "session" with the web server (which has its own global too, outside the current request global)

Let me know if this makes sense.
I know we have more complex examples but this one is east to grasp.
We also have a case where we have a webserver hosting a debugger which can debug another web servers scripts.  In this case we have mulitple runtimes and multiple contexts and pass control back and forth between the two when stepping through code.
(In reply to comment #7)
> Here's one example.  (We have several scenarios but I can't remember them all
> without more digging)
> 
> A server side JS script is running in a web server on context A.
> The script needs to check security for something by polling "decision voters".
> who get to say nay or yay to the request.
> 
> The top level script suspends its request and then calls a security manager
> which fires pre-compiled function on a security global to "vote".  The security
> manager has its own global and can either pull a new context from a pool or use
> one it already has to run the js function. 

But you could make this IPC, no?  Whether to a separate thread or process.
Sure, if I wanted it to be really really slow, needlessly complex and hard to debug. We measure response time in microseconds.

Why would I want the overhead of another thread to start or IPC mechanism?  Let alone another process.
Can Luke or somebody explain what the overall goal is?  I'm not sure I understand why this is being proposed.  I think V8 is moving away from TLS for speed reasons from what I've been able to glean...
Oops, I swapped 'blocks' with 'depends on' :-/

(In reply to comment #1)
> There is a loss in flexibility here: if you have only one implicit TLS-mapped
> data structure, you can't nest on the native (OS thread) stack using different
> contexts, which can be done today (correct me if I'm wrong).

The gist behind referencing cx->globalObject/cx->options-removal is that, with
these and a few other key bits removed as persistent state of the JSContext
(and instead managed LIFO using guards like AutoEnterCompartment) we can
achieve the *effect* of multiple nested JSContext by instead using LIFO stack
guards.

> Having disjoint stack backtraces even on the same OS thread, when nesting flows
> on two or more contexts, can be important. Or have we broken that?

It mostly (modulo JS_SaveFrameChain) works now: JS_CallFunction(cx) will link
the new frame to cx->fp().  However, this is easy enough to simulate if we want
multiple interleaved backtraces: give JS_CallFunction a "down" frame (just like
JS_Execute has now) and let the embedding weave whatever backtraces they need. 
Again, this makes explicit something that was implicit with JSContext.

MikeM:
It sounds like one of your objections is against the single-thread-ification of
JSRuntime; that is not the subject of this bug, see bug 649537.  In fact, now
that I think about it, I don't think this bug actually depends on that one.

Your second objection is against losing multiple JSContext's per OS thread. 
Now, what you've described is like what Mozilla does now.  Hopefully my comment
to Brendan above gives some assurance that you will be able to continue to
achieve the same effect via stack guards.  In particular, with bug 650353, all
you need is to switch compartments when you switch from unprivileged to
privileged code.  Note that compartments give you a much stronger security
boundary (transitive closure) than what you get by changing JSContext+global.

To restate: unless I'm missing a key usecase (which I'm happy to continue to
listen for) I think/hope there should be no real functionality loss, but rather
a reduction in number of concepts needed to do the same thing using JSAPI.
No longer blocks: 604813, new-web-workers, 650357
Depends on: 604813, 650357
(Although the topic of this bug is not single-threaded JSRuntime: note that you don't need IPC: you can have two JSRuntimes in two different threads in the same process that communicate using class hooks or proxies.  Again, with web workers Mozilla has the same challenge; that is exactly part of what bug 649537 is doing.  Granted, web workers are (very much by design) not trying to share globals or otherwise alias normal JS objects across threads.)
> It sounds like one of your objections is against the single-thread-ification of
> JSRuntime; that is not the subject of this bug, see bug 649537.  
Yeah, that would be hard to swallow.

> Your second objection is against losing multiple JSContext's per OS thread. 
> Now, what you've described is like what Mozilla does now.  Hopefully my comment
>to Brendan above gives some assurance that you will be able to continue to
> achieve the same effect via stack guards.  In particular, with bug 650353, all
> you need is to switch compartments when you switch from unprivileged to
> privileged code.  Note that compartments give you a much stronger security
> boundary (transitive closure) than what you get by changing JSContext+global.

Some of that was over my head. Can you explain the actual JS API calls I would make to switch to a different context/global in the same thread?
(In reply to comment #14)
> Some of that was over my head. Can you explain the actual JS API calls I would
> make to switch to a different context/global in the same thread?

Sorry about that.  So, first, my thesis is that 'context' has no meaning outside of a few bits of state; mostly JSOPTION_* flags that have been set and its "global" object (JS_{Set,Get}GlobalObject).  Thus, assuming you don't set JSOPTION_* different per-context, the goal isn't to "change context/context" but just to "change "global".  (If you do fiddle with options, there is/would-be an analogous stack guard as shown below.)

So let's pretend there is no more JSContext and bug 650353 has landed. Then you would write:

JSBool call_with_global(JSObject *newGlobal, JSFunction *fun, ...)
{
  JSAutoEnterCompartment ac(newGlobal);
  if (!JS_CallFunctionValue(this, fun, ...))
    return JS_FALSE;
  /* ~JSAutoEnterCompartment changes back to original global */
}

Note that JSAutoEnterCompartment is currently in JSAPI but doesn't always change globals (only when they are in different compartments, hence bug 650353).

On last comment: I was filing this to collect comments and discuss future direction; its not an immediate priority.
Oops, pretend I wrote "return JS_CallFunctionValue"... I was about to write something else but forgot decided against :-/
Luke,
I'm not up to speed on compartments...sorry.

So in your example where is the compartment?
I see the global coming in...newGlobal, but not the compartment.

What's the purpose of "this" being passed to JS_CallFunctionValue()?
Mike: I was doubly-sloppy, I'm sorry.  I used JS_CallFunctionValue but the scope chain used when running a function called via JS_CallFunctionValue is the scope chain when the closure was *created*, not called.  Second, I was also throwing in a Mozilla-ism where we have to explicitly enter compartments before running code (for various arcane hook-related reasons), which we can also ignore here.

Thus, since JS_ExecuteScript takes the scope chain in via the 'obj' parameter, I believe you only have to write:

  JS_ExecuteScript(newGlobal, scriptObj, &rval);
There's one set of DOM consumers of separate JSContexts per window: it allows us to keep track of which window script was entered in: it's the JSContext script is "running on" right now.  This is needed for web compat; some web APIs use that script-entry window for things like base URIs.

I suppose we could manually track that in the DOM somehow, but we'll definitely need a way to track it.
Lets track that separately. I want to kill the context stack. Its dog slow.
Boris: let's say we had bug 650353, is there any good associativity between cx->compartment->global and this window-per-JSContext?  I was vaguely under the impression that the window-per-JSContext might be the outer window of cx->compartment->global, but that might be totally wrong.
> I want to kill the context stack

I'm not talking about the context stack...

> is there any good associativity between cx->compartment->global and this
> window-per-JSContext?

Not sure.  Right now we basically rely on the fact that certain JSAPI entrypoints in Gecko always use a JSContext associated with the entrypoint itself.  I suppose if we had compartment-per-page instead of compartment-per-origin we could use the compartment itself to carry that information...
If so, then that would be like reason #8 to do bug 650353 :)
irc follow-up: Don't forget to add a way for embedders to get the current JSRuntime pointer from inside a JSNative etc.
Update:
With bug 650411 and bug 650353 likely to happen, it seems that this bug can not only remove JSContext from JSAPI, but, at the same time, anything to do with requests and "entering"/"leaving" compartments.  In particular, since bug 625199 makes entering/leaving a compartment super cheap (as long as you don't have an exception pending) we can simply say that *any* JSAPI function that takes a GC thing enters the compartment of that GC thing and leaves on return.  It is still necessary to wrap values that transfer between compartments(=globals), but this can be simplified to take an explicit destination compartment:
  JSBool JS_WrapValueIntoCompartmentOf(JSObject *gcthing, Value *vp);
instead of having the destination implicitly be cx->compartment.
Sounds awesome. Can't wait for these changes to happen Luke.
Depends on: 767938
Depends on: 981188
Assignee: general → nobody
Depends on: 742497
We're really close so we should start thinking what we want to do here after bz lands the final patches to use 1 JSContext per runtime in Gecko. Here's one proposal:

* JSContext and JSRuntime will always be 1:1. JS_NewContext and related APIs go away and we simply create the context in JS_NewRuntime. rt->mainThreadContext() asserts we're on the main thread and then gives us the runtime's JSContext.

* In this scheme, JSContext is just a token that means "We're on the runtime's main thread", whereas ExclusiveContext and JSRuntime can be accessed on background threads.

* We could then go a step further and use JSContext for stuff that's only accessed on the runtime's main thread, like the Activation list. In this world, JSRuntime only contains data that has to be available on background threads.

Curious to hear your thoughts!
Flags: needinfo?(luke)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
I think you're right that we should continue to use the static type system to segregate main thread from background thread.  I also like the idea of (completely counter to the previous trend :) pushing as much as we can from JSRuntime into JSContext to statically represent main-thread-only stuff.

I wonder: if we do this to the fullest extent such that the only stuff remaining in JSRuntime is that which is shared and has a thread-safe public interface (that performs or asserts locking, etc), then maybe we don't need ExclusiveContext to do all this runtime-hiding business; rather, ExclusiveContext would just publicly expose JSRuntime and just add the additional per-thread data.  But we already have PerThreadData for this purpose.  So what if we had:
 - PerThreadData: per-thread stuff, public pointer to JSRuntime; maybe rename to PerThreadContext?
 - JSContext: *derives* JSRuntime *and* PerThread
 - JSRuntime: shared stuff; has assert-checked downcast to JSContext

The derivation in JSContext isn't trying to be cute; it'd have a few advantages:
 - avoid indirection when accessing any runtime or per-thread field given 'cx'; it's all static offsets.
 - implicit derived-to-base casts reflect what you can do safely
 - avoid cognitive burden of having to remember what is where (when it doesn't matter for safety as checked by static types)
 - JSRuntime can have protected fields that are used by JSContext methods and/or protected methods that are made public by JSContext (`using JSRuntime::foo;`)

Lastly, for JSAPI, I'd suggest we simply remove public JSContext altogether and have all interfaces deal with JSRuntime.  (JSAPI functions then use the asserted downcast to call an internal JSContext function.)

Hope I didn't forget any critical architectural constraints and this is bogus; it's like being asked what you want for your birthday ;)
Flags: needinfo?(luke)
For my birthday I'd like API functions to take JSRuntime& instead of JSRuntime* to make non-nullness obvious...
(In reply to Luke Wagner [:luke] from comment #28)
> Lastly, for JSAPI, I'd suggest we simply remove public JSContext altogether
> and have all interfaces deal with JSRuntime.  (JSAPI functions then use the
> asserted downcast to call an internal JSContext function.)

Will natives also have this JSRuntime argument? I'm afraid that will require a lot of downcasts (especially inside the engine, where natives will often want to call cx-taking functions). What if we make JSContext the public type (maybe after renaming it)?

I like the inheritance idea.

(In reply to Nicholas Nethercote [:njn] from comment #29)
> For my birthday I'd like API functions to take JSRuntime& instead of
> JSRuntime* to make non-nullness obvious...

We have functions that take an optional cx (maybecx). Using a pointer instead of a reference for these would make that more obvious too.
If we're going to significantly change what a JSContext is, perhaps we should consider changing the name (though I don't suggest bikeshedding in this bug, necesarily).
(In reply to Jan de Mooij [:jandem] from comment #30)
Good point, I hadn't thought about the callbacks.  So that would mean instead removing JSRuntime from the JSAPI (b/c it does seem better to have only 1 thing).
(In reply to Luke Wagner [:luke] from comment #32)
> (In reply to Jan de Mooij [:jandem] from comment #30)
> Good point, I hadn't thought about the callbacks.  So that would mean
> instead removing JSRuntime from the JSAPI (b/c it does seem better to have
> only 1 thing).

OK. I'll have to check but I think almost all APIs/callbacks are main-thread only and will be happy with the most derived type. I am a bit worried about things like Class hooks and JSFreeOp (currently has a |runtime()| accessor) that might be used off-thread.

If we make JSContext the public type, we could rename JSRuntime -> js::SharedRuntime, JSContext -> JSRuntime (JS::Runtime?), etc.
I had been thinking along the same lines, regarding naming, but then it started to seem like the current names (modulo JS* vs. JS::*/js::*) are actually appropriate:
 - a "runtime" is a thing that contains all relevant VM state, which is what a JSRuntime is
 - a "context" is a thing that allows you to run something, which is what a JSContext is

Also, over time, (and after the above dehoisting of state into JSContext) I have a feeling that the push to share more and more between JSContexts (to continue to make workers lighter weight) may actually lead to JSRuntime being shared by multiple JSContexts (again, but this time totally different ;)
(In reply to Luke Wagner [:luke] from comment #34)
> Also, over time, (and after the above dehoisting of state into JSContext) I
> have a feeling that the push to share more and more between JSContexts (to
> continue to make workers lighter weight) may actually lead to JSRuntime
> being shared by multiple JSContexts (again, but this time totally different
> ;)

Note that we already have a ParentRuntime since bug 1211723, so isn't that the thing we would/should be sharing?
Ah, apologies, it seems I misunderstood that bug. The parentRuntime field is simply another JSRuntime.
Egad, this thread is a mess of cross-chatter.  :-(

(In reply to Jan de Mooij [:jandem] from comment #27)
> * JSContext and JSRuntime will always be 1:1. JS_NewContext and related APIs
> go away and we simply create the context in JS_NewRuntime.
> rt->mainThreadContext() asserts we're on the main thread and then gives us
> the runtime's JSContext.

JSContext shouldn't exist at all -- internally or externally.  The concept that is the unification of JSContext and JSRuntime, which we can tentatively name JSRuntime as the name of the current closest thing to it, is the only thing that should exist.  (To the extent what you're saying is exactly the same as what I'm saying, just with JSContext as the name we use, I suppose I don't really care.  Except that we've been saying for years we wanted to remove *JSContext*, so it seems a little strange to not adhere to that claim.)  There should be no need to explicitly create "the context" in JS_NewRuntime.

> * In this scheme, JSContext is just a token that means "We're on the
> runtime's main thread", whereas ExclusiveContext and JSRuntime can be
> accessed on background threads.

To the extent such a token is valuable (and I assume it is, because we have it now for contexts), we can do equally well by having this inheritance hierarchy:

JSRuntime -> ExclusiveRuntime -> '()

and then have background threads, and threads not doing things with cross-thread consequences, use only ExclusiveRuntime.

If it happens that everything in JSRuntime right now, is accessed by design, locking-safely, across all threads, then we should just fold everything in JSRuntime into ExclusiveContext, s/ExclusiveContext/ExclusiveRuntime/g, s/JSRuntime/ExclusiveRuntime/g and s/JSContext/JSRuntime/g.  Or introduce another level into the inheritance chain:

JSRuntime (you're on the runtime's thread)
\__> ExclusiveRuntime (any thread: you've got a lock on whole-runtime stuff)
     \_> ProtectedRuntime (any thread, but no runtime lock acquired)
         \_> '()

> * We could then go a step further and use JSContext for stuff that's only
> accessed on the runtime's main thread, like the Activation list. In this
> world, JSRuntime only contains data that has to be available on background
> threads.

Okay, so this is suggesting JSRuntime as it exists now has stuff in it that's accessed only on the main thread.  Fine -- that stuff wouldn't move into ExclusiveRuntime.

So I guess we're more or less on the same page, just with different names for what the final concepts should be?


(In reply to Luke Wagner [:luke] from comment #28)
> I also like the idea of
> (completely counter to the previous trend :) pushing as much as we can from
> JSRuntime into JSContext to statically represent main-thread-only stuff.

I think we're in agreement on this, modulo names.

> So what if we had:
>  - PerThreadData: per-thread stuff, public pointer to JSRuntime; maybe
> rename to PerThreadContext?

Seems right modulo naming.

>  - JSContext: *derives* JSRuntime *and* PerThread

Having per-thread stuff accessible to background threads via implicit cast of the JSRuntime* they store, with no checking of thread, sounds a little risky to me.  I think I'd make it an explicit downcast method plus private inheritance, maybe?

>  - JSRuntime: shared stuff; has assert-checked downcast to JSContext

I'm 100% confused why JSRuntime *and* JSContext both have to exist after this.


(In reply to Nicholas Nethercote [:njn] from comment #29)
> For my birthday I'd like API functions to take JSRuntime& instead of
> JSRuntime* to make non-nullness obvious...

If we were going for druthers, all the functions taking JSRuntime* would be member functions on a JS::Runtime structure, or whatever.  Runtime creation would be via a |UniquePtr<JS::Runtime> MakeRuntime()| function.  If you had a pointer/reference to the JSRuntime, self-evidently you could do stuff with it, and the |this| inside the functions would clearly be non-null.

But none of this is stuff we should do now, versus the minimum signature changes necessary for context/runtime unification.


(In reply to Luke Wagner [:luke] from comment #34)
> I had been thinking along the same lines, regarding naming, but then it
> started to seem like the current names (modulo JS* vs. JS::*/js::*) are
> actually appropriate:
>  - a "runtime" is a thing that contains all relevant VM state, which is what
> a JSRuntime is
>  - a "context" is a thing that allows you to run something, which is what a
> JSContext is

Ugh, no!  Our entire thrust has been toward unifying these, because having the two separate, and requiring both to exist, makes for a mess of information sprawled across multiple data structures, yet relevant to everything using any of them.  There's no reason we need two concepts for this at all.
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> Having per-thread stuff accessible to background threads via implicit cast
> of the JSRuntime* they store, with no checking of thread, sounds a little
> risky to me.  I think I'd make it an explicit downcast method plus private
> inheritance, maybe?

I think you're misunderstanding the proposal; maybe give it another read?

> >  - JSRuntime: shared stuff; has assert-checked downcast to JSContext
> 
> I'm 100% confused why JSRuntime *and* JSContext both have to exist after
> this.

JSContext is the static type "token" that says you're on the main thread.  We can also use the two types to segregate fields/methods that are main-thread-only vs. main-thread-and-background-threads.  Dumping them both into a single class feels like a missed opportunity to leverage static typing.

> (In reply to Luke Wagner [:luke] from comment #34)
> Ugh, no!  Our entire thrust has been toward unifying these, because having
> the two separate, and requiring both to exist, makes for a mess of
> information sprawled across multiple data structures, yet relevant to
> everything using any of them.  There's no reason we need two concepts for
> this at all.

The "entire thrust" has been avoiding the 1:N relationship between JSRuntime and JSContext.  Once there is a 1:1 relationship (particularly, when the two are glued together in one object via inheritance) it will be quite easy to move fields/methods between the two.  Given that most things aren't shared, I would expect JSContext to end up with most of the methods/fields.  I wouldn't get too hung up on the literal mantra "rm JSContext".
> The "entire thrust" has been avoiding the 1:N relationship between JSRuntime and JSContext.

Well, the obvious thing is to work incrementally -- do this and see where we stand.

(Now that we're up close to it, I actually feel a little pessimistic about our chances of simplifying things much here without a lot of hard work.)
Flags: needinfo?(jorendorff)
Depends on: 1278223
Other than the naming issue, it seems there's agreement to move to a single main-thread type with inheritance for the background thread. To do that as incrementally as possible, I think this could work:

Step 1: Make JSContext (not ExclusiveContext!) inherit from JSRuntime, but keep cx->runtime().

Step 2: Get rid of cx->runtime() calls and use cx directly.

Step 3: Incrementally push main-thread-only fields/methods down from JSRuntime into JSContext.

Step 4: Now it should be safe to make JSRuntime a base class of ExclusiveContext instead of JSContext.

Step 5: This will probably be the right time to rename things. Personally I like this:

    JSRuntime -> js::SharedRuntime
    ^
    js::ExclusiveContext -> js::ExclusiveRuntime
    ^
    JSContext -> JS::Runtime

But this can wait for now and I don't think a decision here has to hold up steps 1-4.

Unfortunately there will be churn for embedders for a while, but that's unavoidable no matter what we do here and I think the end result with be worth it (1 external type instead of 2).
(In reply to Jan de Mooij [:jandem] from comment #40)
> Step 4: Now it should be safe to make JSRuntime a base class of
> ExclusiveContext instead of JSContext.

Oops, Luke points out this is bogus, of course. ExclusiveContext will still need a pointer to JSRuntime instead of inheriting from it.
Depends on: 1278947
Depends on: 1279295
Depends on: 1281529
Depends on: 1283855
Depends on: 1284808
Depends on: 1285134
Depends on: 1286159
Depends on: 1286795
Depends on: 1292892
Depends on: 1294404
Depends on: 1302448
I think we can close this now. JSContext is still there, but JSContext now inherits from JSRuntime and we no longer expose JSRuntime to JSAPI.
Assignee: nobody → jdemooij
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: rm JSContext → Refactor JSContext/JSRuntime
Great work on this Jan!  I've benefited from the 1:1 association several times already.
You need to log in before you can comment on or make changes to this bug.