consolidate VM stack code, maybe into js/src/vm/Stack.h ?

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
VM stack logic is scattered over at least 3 .cpps and more headers.  I'd like to consolidate these into a single Stack{.cpp, .h, inlines.h} triple.

On a more general note, I was thinking of putting these new files in a new dir 'vm' in js/src whose eventual goal would be to contain other VM-global data structures and their central operations like strings, objects and shapes.  The idea is that the data structures and operations in 'vm' would either be called by all the different execution modes (mjit/tjit/interp) or simulated by jit code.  Thoughts on this?
Oh God please yes, at least re: stack separation.
Do you have a newsletter?
(Assignee)

Comment 3

6 years ago
Created attachment 527393 [details] [diff] [review]
do it

This patch does the hoisting.  There was a good amount of stack goop in JSContext and the interaction between JSContext and StackSpace was really messy; basically, before there was no logical abstraction and I just put the members where they fit.  In this patch, I pulled all the stack goop out of JSContext and into a new class ContextStack which is owned by JSContext.  Then, I relocated functionality so that now StackSpace is a coherent abstraction (concerned only with a single stack of segments and the stack limit) separate from ContextStack.  I'm pretty happy with the result.  Also, I made a single, unified, and hopefully more coherent "stack layout" comment.

The patch is rather large, but its mostly just shuffling around code and renaming and you can skim most of it as fast as you can scroll.  The new Stack.{h,cpp} is just a permutation of the old code, so probably doesn't need too careful attention (stack bugs tend to blow up loud and early, and this is all green on try).

The one thing that was changed that deserved some looking-at is the stack-limit handling code (now StackSpace::getStackLimit, bumpLimitWithinQuota, bumpLimit).  Also, there was a tiny bit of refactoring concerning popInlineFrame and setting regs (so that fp_ could be private, which I've wanted for a long time).
Attachment #527393 - Flags: review?(dvander)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 649761
Comment on attachment 527393 [details] [diff] [review]
do it

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

Nice refactoring, I especially like how many checks became less operational (i.e. "empty" versus "!segment"). I wouldn't say no to a separate file for the js::StackFrame structure, since usually when I want to find that it's nestled deep in code I'd rather not see. But up to you.

I'll look at the last bit (the limit checks) later tonight.

::: js/src/jscompartment.h
@@ +142,2 @@
     double *stack() { return stack_global_buf; }
+    double *global() { return stack_global_buf + StackSpace::MAX_NATIVE_STACK_SLOTS; }

Seems like these limits are more a property of TraceNativeStorage than StackSpace.

::: js/src/jsinterp.cpp
@@ -592,5 @@
         return JS_FALSE;
     args[1].setObject(*argsobj);
-    JSBool ok = (flags & JSINVOKE_CONSTRUCT)
-                ? InvokeConstructor(cx, args)
-                : Invoke(cx, args, flags);

Does Invoke() not need the ConstructOption here?

::: js/src/methodjit/MethodJIT.h
@@ +183,5 @@
+
+    static const size_t offsetOfFp = 5 * sizeof(void *) + FrameRegs::offsetOfFp;
+    static void staticAssert() {
+        JS_STATIC_ASSERT(offsetOfFp == offsetof(VMFrame, regs) + FrameRegs::offsetOfFp);
+    }

Could offsetOfFp just be a function doing this computation instead?
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> Seems like these limits are more a property of TraceNativeStorage than
> StackSpace.

Ah, good point; it felt a little weird to plop it there.

> ::: js/src/jsinterp.cpp
> @@ -592,5 @@
>          return JS_FALSE;
>      args[1].setObject(*argsobj);
> -    JSBool ok = (flags & JSINVOKE_CONSTRUCT)
> -                ? InvokeConstructor(cx, args)
> -                : Invoke(cx, args, flags);
> 
> Does Invoke() not need the ConstructOption here?

Here flags is always 0 (see the single call-site) and the default arg is INVOKE_NORMAL (since that is what everyone but InvokeConstructor need to call).

> Could offsetOfFp just be a function doing this computation instead?

I would've, but offsetOfFp is used in JS_STATIC_ASSERT (near the asm constants).

Thanks again
Comment on attachment 527393 [details] [diff] [review]
do it

Review of attachment 527393 [details] [diff] [review]:
Attachment #527393 - Flags: review?(dvander) → review+
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/tracemonkey/rev/e9da34dfa8c5
Forgot r=dvander :-/
(Assignee)

Updated

6 years ago
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

6 years ago
Blocks: 653057
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/e9da34dfa8c5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
I'm looking at this due to "recursion errors" I'm getting with SunSpider on the PowerPC tracejit. How was MAX_INLINE_CALLS determined?
(Assignee)

Comment 11

6 years ago
MAX_INLINE_CALLS has been removed by a later patch; instead, we just let code run until it hits the end of the stack.
Then that's what I'll do for our internal build of 6. Thanks! What was that patch, for my reference?
(Assignee)

Comment 13

6 years ago
Bug 538293
Depends on: 674843
You need to log in before you can comment on or make changes to this bug.