David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 15 years ago

FrameInfo::callee is not trace invariant. This is mucking up recursion performance in v8/early-boyer.js because it internalizes frames and guards on their identity.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 15 years ago

WIP, gets rid of GetUpvarOnTrace depending on FrameInfo::callee, which has the nice side effect of greatly simplifying the code (assuming it's correct).

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 2 • 15 years ago

Fixes bug 510987, removes FrameInfo::callee completely. Waiting for mochitests+tryserver before flagging for review.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 15 years ago

Comment on attachment 394935 [details] [diff] [review]
v2

requesting review from dmandelin for upvar changes

	David Mandelin [:dmandelin]
	Comment 4 • 15 years ago

Comment on attachment 394935 [details] [diff] [review]
v2

Very nice. Definitely simpler. I only have a few spots where I would like to see a bit more explanation in comments (although there's less pressure on that now that it's simpler).

Comment here something like:

// Return the number of values on the native stack, excluding
// the innermost frame.

May want to change the name to reflect that idea as well.

>+static int32
>+GetTopOfNativeStack(InterpState* state, uint32 callDepth)

>@@ -2393,12 +2403,21 @@
>     FrameInfo** fip = state->rp + callDepth;
> 
>     /*
>-     * First search the FrameInfo call stack for an entry containing
>-     * our upvar, namely one with level == upvarLevel.
>-     */
>-    while (--fip >= state->callstackBase) {
>+     * First search the FrameInfo call stack for an entry containing our upvar, namely one with
>+     * level == upvarLevel. Note that the first entry on the callstack is the trace entry, where
>+     * the callee may not exist. To avoid cluttering FrameInfo with this information, just skip
>+     * the entry frame.
>+     */

I don't understand what "Note that" and what follows it, so that probably needs to be clearer.

>+    int32 stackOffset = GetTopOfNativeStack(state, callDepth);
>+    while (--fip > state->callstackBase) {
>         FrameInfo* fi = *fip;
>-        JSFunction* fun = GET_FUNCTION_PRIVATE(cx, fi->callee);
>+        /*
>+         * The loop starts aligned to the top of the stack, so move down to the first meaningful
>+         * callee. Then read the callee directly from the frame.
>+         */

The above comment is very clear.

>diff -r 666ffe2c6938 js/src/jstracer.h
>--- a/js/src/jstracer.h	Mon Aug 17 17:36:36 2009 -0500
>+++ b/js/src/jstracer.h	Mon Aug 17 15:53:37 2009 -0700
>@@ -437,7 +437,6 @@
> };
> 
> struct FrameInfo {
>-    JSObject*       callee;     // callee function object
>     JSObject*       block;      // caller block chain head
>     jsbytecode*     pc;         // caller fp->regs->pc
>     jsbytecode*     imacpc;     // caller fp->imacpc
>@@ -446,19 +445,18 @@
>     /*
>      * Bit  15 (0x8000) is a flag that is set if constructing (called through new).
>      * Bits 0-14 are the actual argument count. This may be less than fun->nargs.
>+     * NB: This is argc for the callee, not the caller.
>      */
>     uint16          argc;
> 
>     /*
>-     * Stack pointer adjustment needed for navigation of native stack in
>-     * js_GetUpvarOnTrace. spoffset is the number of slots in the native
>-     * stack frame for the caller *before* the slots covered by spdist.
>-     * This may be negative if the caller is the top level script.
>-     * The key fact is that if we let 'cpos' be the start of the caller's
>-     * native stack frame, then (cpos + spoffset) points to the first
>-     * non-argument slot in the callee's native stack frame.
>+     * Number of stack slots in the caller, not counting slots pushed when invoking
>+     * the callee. That is, slots after JSOP_CALL completes but without the return value.
>      */

The above is good. But we should also say something like:

// This is also equal to the number of slots between the caller's
// 'fp->callee' and the callee's 'fp->callee'.

But that's kind of confusing wording. :-) The important point to get across is how the number is used to navigate stack frame.

	Brendan Eich [:brendan]
	Comment 5 • 15 years ago

Could I look at the next patch once dmandelin's comments are addressed? Thanks,

/be

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 15 years ago

(In reply to comment #4)
Good suggestions. I hope this version is more readable.

	Brendan Eich [:brendan]
	Comment 7 • 15 years ago

Comment on attachment 394993 [details] [diff] [review]
v2, with better comments

..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Wondering if comments following a smaller wrap margin is better for readability; up to you.

> /*
>+ * Returns the number of values on the native stack, excluding the innermost frame. This walks all
>+ * FrameInfos on the native frame stack and sums the slot usage of each frame.
>+ */
>+static int32
>+StackDepthFromCallStack(InterpState* state, uint32 callDepth)
>+{
>+    int32 nativeStackFramePos = 0;

Blank line here for readability?

>+    // Duplicate native stack layout computation: see VisitFrameSlots header comment.
>+    for (FrameInfo** fip2 = state->callstackBase; fip2 < state->rp + callDepth; fip2++)
>+        nativeStackFramePos += (*fip2)->stackslots;

s/fip2/fip/g

>+     * First search the FrameInfo call stack for an entry containing our upvar, namely one with
>+     * level == upvarLevel. The first FrameInfo is a transition from the entry frame to some
>+     * callee. However, it is not known (from looking at the FrameInfo) whether the entry frame
>+     * had a callee. Rather than special case this or insert more logic into the loop, instead just
>+     * stop before that FrameInfo (i.e. |> base| instead of |>= base|), and let the code after the
>+     * loop handle it.

Ditto wrap-margin nit, but even if you use a wider margin, the above looks ragged right -- auto-wrap in vim or emacs?

>+     */
>+    int32 stackOffset = StackDepthFromCallStack(state, callDepth);
>+    while (--fip > state->callstackBase) {
>         FrameInfo* fi = *fip;

Blank line here per house style (major comment following non-{).

>@@ -5686,12 +5709,15 @@
> 
>     JS_ARENA_RELEASE(&cx->stackPool, state.stackMark);
>     while (callstack < rp) {
>+        FrameInfo* fi = *callstack;

Blank line.

>+        /* Peek at the callee stored in the yet-unconstructed down frame. */
>+        JSObject* callee = *(JSObject**)&stack[fi->stackslots];

Ditto.

>     for (unsigned n = 0; n < calldepth; ++n) {
>-        calldepth_slots += SynthesizeFrame(cx, *callstack[n]);

Ditto -- a bit too dense here (not that jstracer.cpp doesn't get dense this way elsewhere :-).

>+        /* Peek at the callee in the yet-unconstructed down frame. */
>+        calleeOffset += callstack[n]->stackslots;
>+        JSObject* callee = *(JSObject**)&stack[calleeOffset];
>+
>+        /* Reconstruct the frame. */

Nice blank line before comment ;-).

> struct FrameInfo {
>-    JSObject*       callee;     // callee function object

Yay!

>     JSObject*       block;      // caller block chain head
>     jsbytecode*     pc;         // caller fp->regs->pc
>     jsbytecode*     imacpc;     // caller fp->imacpc
>@@ -446,19 +445,20 @@
>     /*
>      * Bit  15 (0x8000) is a flag that is set if constructing (called through new).
>      * Bits 0-14 are the actual argument count. This may be less than fun->nargs.
>+     * NB: This is argc for the callee, not the caller.
>      */
>     uint16          argc;
> 
>     /*
>-     * Stack pointer adjustment needed for navigation of native stack in
>-     * js_GetUpvarOnTrace. spoffset is the number of slots in the native
>-     * stack frame for the caller *before* the slots covered by spdist.
>-     * This may be negative if the caller is the top level script.
>-     * The key fact is that if we let 'cpos' be the start of the caller's
>-     * native stack frame, then (cpos + spoffset) points to the first
>-     * non-argument slot in the callee's native stack frame.
>+     * Number of stack slots in the caller, not counting slots pushed when invoking
>+     * the callee. That is, slots after JSOP_CALL completes but without the return value.
>+     * This is also equal to the number of slots between fp->down->argv[-2] (calleR fp->callee) and
>+     * fp->argv[-2] (calleE fp->callee).

Ragged right comment. Also overlong lines suggest rewrapping to 80 if you can stand it.

>      */
>-    int32          spoffset;
>+    uint32          stackslots;

Is there a better name? height or frameHeight?

>+
>+    /* argc of the caller */
>+    uint32          callerArgc;

Could be uint16 but there's nothing to pack it with. But couldn't stackslots be uint16 too? We limit the JIT's native stack depth to << 2^16.

> 
>     // Safer accessors for argc.
>     enum { CONSTRUCTING_MASK = 0x8000 };

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 15 years ago

(In reply to comment #7)
> Is there a better name? height or frameHeight?

Hrm, how about "callerSlots" or "callerHeight", to match callerArgc?


> Could be uint16 but there's nothing to pack it with. But couldn't stackslots be
> uint16 too? We limit the JIT's native stack depth to << 2^16.

Seems okay to use uint16 here because the sum is kept in a uint32 anyway.

	Brendan Eich [:brendan]
	Comment 10 • 15 years ago

Comment on attachment 395738 [details] [diff] [review]
addresses comments

>+     * callee. Rather than special case this or insert more logic into the

Nitus maximus: hyphenate special-case.

>+        /*
>+         * The loop starts aligned to the top of the stack, so move down to the first meaningful
>+         * callee. Then read the callee directly from the frame.
>+         */
>+        stackOffset -= fi->callerHeight;
>+        JSObject* callee = *(JSObject**)(&state->stackBase[stackOffset]);
>+        JSFunction* fun = GET_FUNCTION_PRIVATE(cx, callee);
>         uintN calleeLevel = fun->u.i.script->staticLevel;
>         if (calleeLevel == upvarLevel) {
>             /*
>+             * Now find the upvar's value in the native stack. stackOffset is the offset of the
>+             * start of the activation record corresponding to *fip in the native stack.
>+             */

These comments want rewrapping to match the bigger prior one.

>+    #if defined DEBUG

Forgot to request prevailing #ifdef DEBUG, unindented, style. This looks like nanojit style, but it is overlong in using #if defined, and IMHO hard to spot due to indentation. Old C requird # in col. 1 and we keep that memory alive to help readers spot cpp directives more easily.

>+        /* Peek at the callee in the yet-unconstructed down frame. */

Suggest "Peek at the callee native slot in the not-yet-synthesized down frame."

>-    enum { CONSTRUCTING_MASK = 0x8000 };

Pre-existing nit: CONSTRUCTING_FLAG would be a slightly better name.

Is there an assertion static or runtime, or an error check, that needs to be adjusted? Aha, grep spotted it for me:

$ grep -n 'argc >= ' jstracer.cpp
10693:    if (argc >= 0x8000)
...

We don't need any check now, but you could assert that argc < CONSTRUCTING_FLAG.

Fix these and r=me.

/be

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 15 years ago

Pushed with fixes from comment #10

http://hg.mozilla.org/tracemonkey/rev/31e363f76eae

	Robert Sayre
	Comment 12 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/31e363f76eae

	Robert Sayre
	Comment 13 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/635d777cd927