730281 - remove cx argument from GC and compartment related functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

With the switch to single-threaded runtime many compartment and GC functions take a cx argument that is either unused or only used to access JSRuntime. We should remove that or replace it with JSRuntime * to eliminate unnecessary cx references.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch removes the cx argument from a few GC and compartment functions and propagates the cx removal to the callers when feasible.

The patch also replaces ThreadContextRange and js_ContextIter with single ContextIter class.

In the patch I moved the autorroters and Root stacks to JSRuntime. Strictly speaking this is not required for this bug but it allows to minimize changes for the bug 723286.

Comment 2

[Igor Bukanov]

Attachment: v2

In the previous patch I have missed few more cases where JSContext *cx argument can be eliminated.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm a little worried about moving rooters to the runtime. We want pushing and popping a rooter to be as fast as possible, and this adds an extra load to that path. How important is it to do this?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 600403 [details] [diff] [review]
v2

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

I'm still concerned about moving the rooting stuff from JSContext to JSRuntime. However, the rest of this patch is fantastic. Why do you need to move the rooters?

::: js/src/jsgc.cpp
@@ +3280,4 @@
>      ~AutoHeapSession();
>  
>    protected:
> +    JSRuntime *rt;

For the most part we've adhered to the convention that field names should be spelled out. The one exception is JSCompartment::rt, which has always bugged me. Can be call this runtime instead of rt?

::: js/src/vm/Stack.h
@@ +1566,5 @@
>  
>  class ContextStack
>  {
>      StackSegment *seg_;
> +    StackSpace *const space_;

What's this about?

Comment 5

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #4)
> 
> I'm still concerned about moving the rooting stuff from JSContext to
> JSRuntime. However, the rest of this patch is fantastic. Why do you need to
> move the rooters?

First some history - the only reasons the rooters originally went in JSContext is because JSThread had not existed at the time of their introduction and JSContext was used to store various per-thread bits. And here we have a clear per-native-thread data structure that, in view of single-threaded runtime, should live in JSRuntime.

Now from a practical point of view for bug 723286 I needed in couple of places to change the code that uses an autorooter in a trace callback. With that patch cx is not available there. It is possible to fix that without the change here but that is sort-of awkward and points to the need to have non-cx rooters so I decided to finally move them from JSContext and eliminate extra cx iterator.

> ::: js/src/vm/Stack.h
> @@ +1566,5 @@
> >  
> >  class ContextStack
> >  {
> >      StackSegment *seg_;
> > +    StackSpace *const space_;
> 
> What's this about?

This was to ensure using a compiler that cx->stack.space() is just an alias to rt->stackSpace and it is safe to drop the cx argument from JSCompartment::hasScriptsOnStack(). If this seems unrelated, I can remove this change.

Comment 6

[Brian Hackett [Laid off!]]

Unless we start passing around JSRuntime instead of JSContext, hot things like rooter lists should I think stay in JSContext.  It would also be good to not totally take for granted that JSRuntime will stay single threaded, e.g. bug 718121.

Comment 7

[Igor Bukanov]

(In reply to Brian Hackett (:bhackett) from comment #6)
> Unless we start passing around JSRuntime instead of JSContext, hot things
> like rooter lists should I think stay in JSContext.

Ok, I will keep then lists for now in JSContext.

>  It would also be good
> to not totally take for granted that JSRuntime will stay single threaded,
> e.g. bug 718121.

I would prefer to keep JSRuntime single-threaded as it is now. That bug does not require multi-threaded runtime, we can have 2 separated runtimes and a notion of a cross-runtime thread-safe wrappers that try not to synchronize things unless absolutely necessary. For example, it would be nice if a full GC in the content would not require to pause chrome.

And even with single-threaded runtime we need not to run things in parallel. Incremental marking and sweeping (including incremental full GC) should remove the GC pause pressure. To workaround long-running content scripts we can start using operation callback to allow to process chrome events when the content script is busy.

Comment 8

[Igor Bukanov]

(In reply to Igor Bukanov from comment #7)
> (In reply to Brian Hackett (:bhackett) from comment #6)
> > Unless we start passing around JSRuntime instead of JSContext, hot things
> > like rooter lists should I think stay in JSContext.
> 
> Ok, I will keep then lists for now in JSContext.

I meant the lists for new rooter implementation.

Comment 9

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f615c44322a

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/6f615c44322a