Closed Bug 1488855 Opened 3 years ago Closed 3 years ago

InterpreterStack is part of JSContext


(Core :: JavaScript Engine, enhancement, P5)

61 Branch



Tracking Status
firefox64 --- fixed


(Reporter: jonco, Assigned: dpino, Mentored)


(Keywords: good-first-bug)


(1 file, 2 obsolete files)

We have one main thread JSContext but also one per helper thread, so it seems like InterpreterStack should live in the JSRuntime.
Giving this a try. I tried to make the patch simple and kept JSContext->interpreterStack() as a wrapper for JSRuntime->interpretStack(), although it can be removed. I dunno if the patch is correct either (I don't see where interpreterStack_ is written).
Attachment #9014805 - Flags: feedback?(jcoppeard)
Updated patch (wrong patch format).
Attachment #9014805 - Attachment is obsolete: true
Attachment #9014805 - Flags: feedback?(jcoppeard)
Attachment #9014820 - Flags: review?(jcoppeard)
Comment on attachment 9014820 [details] [diff] [review]

Review of attachment 9014820 [details] [diff] [review]:

Thanks for the patch!

It's fine to leave the JSContext method as a wrapper since most places we need to access the stack we only have a JSContext anyway.

::: js/src/vm/Runtime.h
@@ +230,5 @@
>      friend class js::jit::JitActivation;
>      friend class js::jit::CompileRuntime;
> +    /* Space for interpreter frames. */
> +    js::ThreadData<js::InterpreterStack> interpreterStack_;

Can you make this MainThreadData instead of ThreadData?  This matches the other fields in JSRuntime that are only supposed to be accessed from the main thread.
Attachment #9014820 - Flags: review?(jcoppeard) → review+
Assigning to you.  Let me know if you need me to queue a try build.
Assignee: nobody → dpino
Updated patch.
Attachment #9014820 - Attachment is obsolete: true
Attachment #9015477 - Flags: review?(jcoppeard)
Comment on attachment 9015477 [details] [diff] [review]

Review of attachment 9015477 [details] [diff] [review]:

Look good.
Attachment #9015477 - Flags: review?(jcoppeard) → review+
Keywords: checkin-needed
Pushed by
Move InterpreterStack from JSContext to JSRuntime. r=jonco
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.