function f() { var x = 2; var y = 3; var o = { valueOf: function () { x = 99; return x; } }; return [x, o + x, x] } print(f()); The problem is that the tracker propagates the location of "x", not the value, along the stack. |o + x| can call |o.valueOf()|, which changes |x|. We don't expect these sorts of shenanigans. Easy fix is to invalidate references on the tracker below the "nuses" of the opcode. Harder fix is to know which slots can be tackled by upvars.

Affects JM2 as well. Time to fix, taking.

Parser changes - stole the last JSDefinition flag to propagate up whether a definition was in lexdeps. This is used to emit a hoisted opcode, JSOP_ESCLOCAL, to inform the JIT not to track the variable. This approach is kind of hacky, but lightweight and not very invasive. I'm not sure if it's 100% sound, but it's enough to try and fix this in the JIT.

Comment on attachment 452176 [details] [diff] [review] actual parser patch >diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp >--- a/js/src/jsemit.cpp >+++ b/js/src/jsemit.cpp >@@ -3681,6 +3681,22 @@ MaybeEmitVarDecl(JSContext *cx, JSCodeGe > CG_SWITCH_TO_MAIN(cg); > } > >+ if (JOF_OPTYPE(pn->pn_op) == JOF_LOCAL && !(cg->flags & TCF_FUN_USES_EVAL) && >+ (pn->pn_used || pn->pn_defn)) { New line after each && for consistency and readability (any overlong chain gets this layout). >+ JSDefinition *dn; >+ if (pn->pn_used) >+ dn = pn->pn_lexdef; >+ else >+ dn = (JSDefinition *)pn; These five lines should be just .. JSDefinition *dn = pn->pn_used ? pn->pn_lexdef : (JSDefinition *)pn; >+ if (dn->pn_dflags & PND_CLOSED) { >+ CG_SWITCH_TO_PROLOG(cg); >+ if (!UpdateLineNumberNotes(cx, cg, pn->pn_pos.begin.lineno)) >+ return JS_FALSE; >+ EMIT_UINT16_IMM_OP(JSOP_ESCLOCAL, pn->pn_cookie); Don't think this NOP requires line number note overhead (one to change to this line, one to change back for next main-code op emitted). Also, do we need more than one such prolog op per local in a function? If not, then do it only if pn->pn_defn, simplifying dn computation (or even elimiating dn by using pn->pn_lexdef). >diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl >--- a/js/src/jsopcode.tbl >+++ b/js/src/jsopcode.tbl >@@ -621,3 +621,5 @@ OPDEF(JSOP_GLOBALDEC, 246,"globaldec > OPDEF(JSOP_CALLGLOBAL, 247,"callglobal", NULL, 3, 0, 2, 19, JOF_GLOBAL|JOF_NAME|JOF_CALLOP) > OPDEF(JSOP_FORGLOBAL, 248,"forglobal", NULL, 3, 1, 1, 19, JOF_GLOBAL|JOF_NAME|JOF_FOR|JOF_TMPSLOT) > >+OPDEF(JSOP_ESCLOCAL, 249,"esclocal", NULL, 3, 0, 0, 19, JOF_LOCAL|JOF_NAME) ESC for ESCaping? This is a prolog op declaring that the indexed local is used by inner functions, right? I.e., that this is a definition of a local used as an upvar. So JSOP_DEFUPVAR? Or JSOP_CLOSED for consistency with the other name/trope you use. >diff --git a/js/src/jsops.cpp b/js/src/jsops.cpp >--- a/js/src/jsops.cpp >+++ b/js/src/jsops.cpp >@@ -130,6 +130,9 @@ END_EMPTY_CASES > BEGIN_CASE(JSOP_LINENO) > END_CASE(JSOP_LINENO) > >+BEGIN_CASE(JSOP_ESCLOCAL) >+END_CASE(JSOP_ESCLOCAL) Relocate to use ADD_EMPTY_CASE with other such ops, instead of BEGIN/END. Same goes for JSOP_LINENO, it appears. >diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp >--- a/js/src/jsparse.cpp >+++ b/js/src/jsparse.cpp >@@ -2570,6 +2570,9 @@ LeaveFunction(JSParseNode *fn, JSTreeCon > dn->pn_defn = false; > dn->pn_used = true; > dn->pn_lexdef = outer_dn; >+ >+ /* Mark the outer dn as escaping. */ >+ outer_dn->pn_dflags |= PND_CLOSED; Think you want this after this brace: > } and unindented one level, right here^. >@@ -2577,6 +2580,7 @@ LeaveFunction(JSParseNode *fn, JSTreeCon > if (!outer_ale) > return false; > ALE_SET_DEFN(outer_ale, ALE_DEFN(ale)); >+ ALE_DEFN(ale)->pn_dflags |= PND_CLOSED; > } Which suggests just moving the outer_dn->pn_dflags |= PND_CLOSED line here, after setting outer_dn = ALE_DEFN(ale) instead of the + line shown just above (+2580,7). Need to think more about this patch and what you're trying to do with it, but I thought you could use these ASAP. Good to try for static analysis relief, it looks promising. /be

Thanks! I like DEFUPVAR - it conveys the intent I neglected to communicate. The plan is that the JIT will not allocate registers for slots that pass through DEFUPVAR - for example, (x + x) will perform two loads, instead of one, if |x| can escape. Right now, on explicit calls, we throw away *all* state so closures will work. So fixing this could either be a perf win or not - calls could get faster, but anything with eval() or inner-referenced-vars will get slower. I think we'll have to suck it up either way. v8 has the same cost, and the same conceptual analysis: ... if ((var->is_this() || var->name()->length() > 0) && (var->is_accessed_from_inner_scope_ || scope_calls_eval_ || inner_scope_calls_eval_ || Causes a variable to be allocated in the heap, and not tracked as a local.

Looks like around a 2% SS regression, and no change on v8. New patch tomorrow.

Once we get conservative stack scanning on the fatval branch, we should get a bigger win with this patch. We can stop flushing the entire frame on every implicit (slow-path) call. At the very least it'll be a big codesize reduction.

(In reply to comment #5) > I think we'll have to suck it up either way. v8 has the same cost, and the same > conceptual analysis: > > ... > if ((var->is_this() || var->name()->length() > 0) && > (var->is_accessed_from_inner_scope_ || > scope_calls_eval_ || inner_scope_calls_eval_ || > > Causes a variable to be allocated in the heap, and not tracked as a local. Hmm, but maybe they do this heap boxing as part of lambda-lifting? That would win by eliminating activation objects. We should look into it too -- it requires a "reference" jsval type to point at the heap-boxed variable, which is dereferenced automatically when the variable is read and written. /be

I don't think they do this. It creates a "CONTEXT" variable, which indexes into some sort of activation record. Accessing one has a static scope chain length and emits assembly to walk the chain.