Last Comment Bug 644074 - consolidate VM stack code, maybe into js/src/vm/Stack.h ?
: consolidate VM stack code, maybe into js/src/vm/Stack.h ?
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
: 649761 (view as bug list)
Depends on: 674843
Blocks: 653057
  Show dependency treegraph
 
Reported: 2011-03-23 00:53 PDT by Luke Wagner [:luke]
Modified: 2011-11-30 12:36 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
do it (734.92 KB, patch)
2011-04-20 14:56 PDT, Luke Wagner [:luke]
dvander: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-03-23 00:53:40 PDT
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?
Comment 1 David Anderson [:dvander] 2011-03-23 01:38:46 PDT
Oh God please yes, at least re: stack separation.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-23 03:07:38 PDT
Do you have a newsletter?
Comment 3 Luke Wagner [:luke] 2011-04-20 14:56:19 PDT
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).
Comment 4 Luke Wagner [:luke] 2011-04-20 15:15:12 PDT
*** Bug 649761 has been marked as a duplicate of this bug. ***
Comment 5 David Anderson [:dvander] 2011-04-25 18:40:06 PDT
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?
Comment 6 Luke Wagner [:luke] 2011-04-25 21:47:07 PDT
(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 7 David Anderson [:dvander] 2011-04-26 11:53:41 PDT
Comment on attachment 527393 [details] [diff] [review]
do it

Review of attachment 527393 [details] [diff] [review]:
Comment 8 Luke Wagner [:luke] 2011-04-26 13:30:02 PDT
http://hg.mozilla.org/tracemonkey/rev/e9da34dfa8c5
Forgot r=dvander :-/
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-05-02 16:00:11 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/e9da34dfa8c5
Comment 10 Cameron Kaiser [:spectre] 2011-07-12 09:22:13 PDT
I'm looking at this due to "recursion errors" I'm getting with SunSpider on the PowerPC tracejit. How was MAX_INLINE_CALLS determined?
Comment 11 Luke Wagner [:luke] 2011-07-12 09:45:32 PDT
MAX_INLINE_CALLS has been removed by a later patch; instead, we just let code run until it hits the end of the stack.
Comment 12 Cameron Kaiser [:spectre] 2011-07-12 10:31:55 PDT
Then that's what I'll do for our internal build of 6. Thanks! What was that patch, for my reference?
Comment 13 Luke Wagner [:luke] 2011-07-12 10:39:27 PDT
Bug 538293

Note You need to log in before you can comment on or make changes to this bug.