1335095 - Allow cooperating JSContexts to iterate over each others' activations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

There are several places in the JS engine where iteration over the frames in an arbitrary context might be necessary; these are mainly for use within the GC and by the debugger.  Such iteration is not possible with a preemptively multithreaded runtime --- we do not want to ever have a need to suspend another running thread so that we can poke at its stack --- but with a cooperatively multithreaded runtime this isn't too hard to do and is preferable for now in comparison with reworking the related APIs so that such iteration is no longer necessary.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8831735 [details] [diff] [review]
patch

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

Makes sense, but some questions below.

::: js/src/jit/JitFrames.cpp
@@ +1455,5 @@
>  void
>  TraceJitActivations(JSRuntime* rt, JSTracer* trc)
>  {
> +    JSContext* cx = TlsContext.get();
> +    for (size_t i = 0; i < rt->cooperatingContexts().length(); i++) {

Shouldn't we handle this higher up the stack and pass a targetCx to TraceJitActivations instead of a rt? Where we call TraceJitActivations there are probably other calls that need similar treatment, for instance GC roots?

::: js/src/jscompartment.cpp
@@ +1134,5 @@
>          // Interrupt any running interpreter frame. The scriptCounts are
>          // allocated on demand when a script resume its execution.
> +        JSContext* cx = TlsContext.get();
> +        JSContext* targetcx = zone()->group()->context;
> +        JSRuntime::AutoProhibitActiveContextChange apacc(cx->runtime());

Hm when is cx != targetcx here? Could use a comment.

::: js/src/jsgc.cpp
@@ +5744,5 @@
> +    JSContext* cx = TlsContext.get();
> +    for (size_t i = 0; i < rt->cooperatingContexts().length(); i++) {
> +        JSContext* targetcx = rt->cooperatingContexts()[i];
> +        for (ActivationIterator iter(cx, targetcx); !iter.done(); ++iter)
> +            iter->compartment()->zone()->active = true;

I removed the active flag 2 months ago, so we can drop these changes.

::: js/src/vm/Stack.cpp
@@ +1708,5 @@
> +        targetcx = cx;
> +    }
> +
> +    // Tolerate a null targetcx, in case we are iterating over the activations
> +    // for a zone group that is not in use by any context.

Is this a stale comment? Not sure how it relates to the code below.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Comment on attachment 8831735 [details] [diff] [review]
> patch
> 
> Review of attachment 8831735 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense, but some questions below.
> 
> ::: js/src/jit/JitFrames.cpp
> @@ +1455,5 @@
> >  void
> >  TraceJitActivations(JSRuntime* rt, JSTracer* trc)
> >  {
> > +    JSContext* cx = TlsContext.get();
> > +    for (size_t i = 0; i < rt->cooperatingContexts().length(); i++) {
> 
> Shouldn't we handle this higher up the stack and pass a targetCx to
> TraceJitActivations instead of a rt? Where we call TraceJitActivations there
> are probably other calls that need similar treatment, for instance GC roots?

Yes, good point.  GCRuntime::traceRuntimeCommon needs to look at all the cooperating contexts in other cases, I just haven't put up patches for them yet.

> ::: js/src/jscompartment.cpp
> @@ +1134,5 @@
> >          // Interrupt any running interpreter frame. The scriptCounts are
> >          // allocated on demand when a script resume its execution.
> > +        JSContext* cx = TlsContext.get();
> > +        JSContext* targetcx = zone()->group()->context;
> > +        JSRuntime::AutoProhibitActiveContextChange apacc(cx->runtime());
> 
> Hm when is cx != targetcx here? Could use a comment.

It's not clear yet to me how to guarantee that debuggers are in the same zone group as their debuggee.  Clearly with preemptive multithreading we need to ensure this (or that the debugger is in the system zone group), so that cx and targetcx are the same, but we don't have any assurance that is the case right now.

> ::: js/src/jsgc.cpp
> @@ +5744,5 @@
> > +    JSContext* cx = TlsContext.get();
> > +    for (size_t i = 0; i < rt->cooperatingContexts().length(); i++) {
> > +        JSContext* targetcx = rt->cooperatingContexts()[i];
> > +        for (ActivationIterator iter(cx, targetcx); !iter.done(); ++iter)
> > +            iter->compartment()->zone()->active = true;
> 
> I removed the active flag 2 months ago, so we can drop these changes.

OK, I've (finally) rebased these patches to tip now.

> ::: js/src/vm/Stack.cpp
> @@ +1708,5 @@
> > +        targetcx = cx;
> > +    }
> > +
> > +    // Tolerate a null targetcx, in case we are iterating over the activations
> > +    // for a zone group that is not in use by any context.
> 
> Is this a stale comment? Not sure how it relates to the code below.

Well, the code following this comment tests for a null targetcx, and leaves the activation list empty if that is the case.  This is in place so that callers don't have to sprinkle error-prone 'if (zone->group()->context)' tests around.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> Yes, good point.  GCRuntime::traceRuntimeCommon needs to look at all the
> cooperating contexts in other cases, I just haven't put up patches for them
> yet.

I think it would be nicer and simpler to pass these contexts down to TraceInterpreterActivations etc, so it's less arbitrary where we need to consider different contexts.

Ideally we'd use a different type for these contexts, so it's immediately obvious whether a function operates on the current thread's context or on an arbitrary one...

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> > Yes, good point.  GCRuntime::traceRuntimeCommon needs to look at all the
> > cooperating contexts in other cases, I just haven't put up patches for them
> > yet.
> 
> I think it would be nicer and simpler to pass these contexts down to
> TraceInterpreterActivations etc, so it's less arbitrary where we need to
> consider different contexts.
> 
> Ideally we'd use a different type for these contexts, so it's immediately
> obvious whether a function operates on the current thread's context or on an
> arbitrary one...

Sure, I'll fix these issues and post a new patch.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8833441 [details] [diff] [review]
patch

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

Thanks for making these changes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fedb64403
Allow cooperating JSContexts to iterate over each others' activations, r=jandem.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe2fedb64403