remove cx argument from GC and compartment related functions

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

unspecified
mozilla13
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
88.88 KB, patch
billm
: review+
Details | Diff | Splinter Review
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.
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #600377 - Flags: review?(wmccloskey)
Posted patch v2Splinter Review
In the previous patch I have missed few more cases where JSContext *cx argument can be eliminated.
Attachment #600377 - Attachment is obsolete: true
Attachment #600403 - Flags: review?(wmccloskey)
Attachment #600377 - Flags: review?(wmccloskey)
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 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?
Attachment #600403 - Flags: review?(wmccloskey) → review+
(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.
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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/6f615c44322a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.