545759 - re-enable partial flat closure by reifying full scope chain on trace

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See bug 545573 comment 5.

/be

Comment 1

[Brendan Eich [:brendan]]

(In reply to bug 545573 comment 5)
> There are two separate bugs related to the last test.
> 
> 1. TR::record_LAMBDA_FC creates a closure parented to the global instead of the
> current scope chain. It needs to call TR::scopeChain, as TR::record_LAMBDA
> does.

I see a scopeChain() call in TR::record_JSOP_LAMBDA_FC -- what am I missing?

The problem seems to be that TR::scopeChain uses the tracker to find the most recently generated LIR for the current frame's scope chain, which is generated in TR::record_EnterFrame for calls, but not for block entry or exit.

> 2. TR::scopeChain() returns the wrong scope chain when we're in a let block.

Right, this is the bug I see -- I don't see a failure to use TR::scopeChain() in TR::record_JSOP_LAMBDA*, s'all.

> The easiest fix is to make TR::scopeChain() fallible and have it call
> js_GetScopeChain and walk the interpreter's stack chain (up to the entry frame)
> looking for Block objects. If it finds one, abort recording.

Active blocks in frames being inlined on trace don't matter unless they are also on the scope chain, right? That is, walking up the dynamic link does not tell you what's in static scope.

I'll take a shot at fixing this. Good to have the review and comments, thanks! I want to be sure I'm not missing anything important in comment 5, still.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to comment #1)
> I see a scopeChain() call in TR::record_JSOP_LAMBDA_FC -- what am I missing?

Nothing. I was looking at the code before bug 542002 was fixed, that's all.

> > The easiest fix is to make TR::scopeChain() fallible and have it call
> > js_GetScopeChain and walk the interpreter's stack chain [...]

This was a typo. I meant "scope chain" of course.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> (In reply to comment #1)
> > I see a scopeChain() call in TR::record_JSOP_LAMBDA_FC -- what am I missing?
> 
> Nothing. I was looking at the code before bug 542002 was fixed, that's all.

Ah, ok! How soon I forget my own patchwork...

> > > The easiest fix is to make TR::scopeChain() fallible and have it call
> > > js_GetScopeChain and walk the interpreter's stack chain [...]
> 
> This was a typo. I meant "scope chain" of course.

Gotcha.

/be

Comment 4

[Brendan Eich [:brendan]]

See bug 554572.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Bug 590006 fixed the correctness bug here, at least as far as the test case in the patch in bug 545573 is concerned.

I don't think there's anything else to be done here, so I'm marking it as a duplicate. Brendan?

Comment 6

[Brendan Eich [:brendan]]

We still have this comment:

jsparse.cpp:                    /* FIXME bug 545759: to test nflattened != 0 */

in this section of Parser::setFunctionKinds:

                if (nupvars == 0) {
                    FUN_METER(onlyfreevar);
                    FUN_SET_KIND(fun, JSFUN_NULL_CLOSURE);
                } else if (nflattened == nupvars) {
                    /* FIXME bug 545759: to test nflattened != 0 */
                    /*
                     * We made it all the way through the upvar loop, so it's
                     * safe to optimize to a flat closure.
                     */
                    FUN_METER(flat);
                    FUN_SET_KIND(fun, JSFUN_FLAT_CLOSURE);
                    switch (PN_OP(fn)) {
                      case JSOP_DEFFUN:
                        fn->pn_op = JSOP_DEFFUN_FC;
                        break;
                      case JSOP_DEFLOCALFUN:
                        fn->pn_op = JSOP_DEFLOCALFUN_FC;
                        break;
                      case JSOP_LAMBDA:
                        fn->pn_op = JSOP_LAMBDA_FC;
                        break;
                      default:
                        /* js_EmitTree's case TOK_FUNCTION: will select op. */
                        JS_ASSERT(PN_OP(fn) == JSOP_NOP);
                    }
                } else {
                    FUN_METER(badfunarg);
                }

I wanted this bug to be about that performance improvement and not the correctness bug. I think this bug morphed, or maybe I just was unclear (sorry if so). Reopening and (un|re)morphing.

/be

Comment 7

[Jason Orendorff [:jorendorff]]

From a comment in JSParser::setFunctionKinds:
> To re-enable partial flat closures, we'll need to avoid n^2 bugs such as
> bug 617430 in uncommenting the following:
> 
>     if (DeoptimizeUsesWithin(lexdep, funbox->node->pn_body->pn_pos))
>         FlagHeavyweights(lexdep, funbox, tcflags);
> 
> For now it's best to avoid this tedious, use-wise deoptimization and let fun
> remain an unoptimized closure.

I think that information belongs here in this bug rather than in the source code. So I'm moving it. (Patch is in bug 669369.)

Comment 8

[Luke Wagner [:luke]]

No more tracer or flat closures.