Closed Bug 1488855 Opened Last year Closed 11 months ago

InterpreterStack is part of JSContext

Categories

(Core :: JavaScript Engine, enhancement, P5)

61 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jonco, Assigned: dpino, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(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]
Bug-1488855-Move-InterpreterStack-from-JSContext-to-.patch

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]
Bug-1488855-Move-InterpreterStack-from-JSContext-to-.patch

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

Look good.
Attachment #9015477 - Flags: review?(jcoppeard) → review+
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d49a5d674e00
Move InterpreterStack from JSContext to JSRuntime. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d49a5d674e00
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.