Closed Bug 1335095 Opened 3 years ago Closed 3 years ago

Allow cooperating JSContexts to iterate over each others' activations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8831735 - Flags: review?(jdemooij)
Blocks: 1335846
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.
(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.
(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...
(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.
Attachment #8831735 - Flags: review?(jdemooij)
Attached patch patchSplinter Review
Attachment #8831735 - Attachment is obsolete: true
Attachment #8833441 - Flags: review?(jdemooij)
Blocks: 1337070
Blocks: 1337106
Comment on attachment 8833441 [details] [diff] [review]
patch

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

Thanks for making these changes.
Attachment #8833441 - Flags: review?(jdemooij) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fedb64403
Allow cooperating JSContexts to iterate over each others' activations, r=jandem.
https://hg.mozilla.org/mozilla-central/rev/fe2fedb64403
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1339944
Depends on: 1347539
You need to log in before you can comment on or make changes to this bug.