Attached test case fails with -m in debug and release mode: Error: Assertion failed: got "67108864,134217728,268435456,536870912,1073741824,undefined,2147483648,4294967296,8589934592,17179869184,34359738368,undefined,68719476736,137438953472,274877906944,549755813888,1099511627776,undefined,2199023255552,4398046511104,8796093022208,17592186044416,35184372088832,undefined,70368744177664,140737488355328,281474976710656,562949953421312,1125899906842624,undefined,", expected "2,4,8,16,32,undefined,64,128,256,512,1024,undefined,2048,4096,8192,16384,32768,undefined,65536,131072,262144,524288,1048576,undefined,2097152,4194304,8388608,16777216,33554432,undefined,"

Tentatively this should block until we know what's going wrong. At a first glance it looks like the stack frame has the wrong scope chain.

This became exposed because we don't IC JSOP_CALLNAME. Instead it goes to the property cache, which brands the object. SETGLOBAL doesn't invalidate the property cache when it changes the function shape. Fixing this requires adding a shape guard to SETGLOBAL - essentially, devolving it to SETGNAME, which is the same fix as for bug 617171. Might as well kill two birds with one stone here.

Most of this patch is removing code. The tricky part was delaying decisions about globals until BindNameToSlot. It wasn't really needed for correctness, but simplifies the code and minimizes the number of places we'd have to deoptimize (for example, no transitions from GETGLOBAL to SETGNAME, which requires changing the cookie).

Performance... predictably, bitwise-and gets slower with -m, but with -m -j -p all changes are in the noise. earley-boyer has lots of global sets, I measured a small slowdown on v8-v4 but v8-v6 was slightly faster.

Comment on attachment 496873 [details] [diff] [review] fix >+static bool >+BindGlobal(JSContext *cx, JSCodeGenerator *cg, JSParseNode *pn, JSAtom *atom, UpvarCookie *cookie) >+{ >+ cookie->makeFree(); >+ >+ if (cg->mightAliasLocals()) >+ return true; >+ >+ GlobalScope *globalScope = cg->compiler()->globalScope; >+ JSCodeGenerator *globalCg = globalScope->cg; >+ >+ JSDefinition *dn; >+ if (pn->pn_used) { >+ dn = pn->pn_lexdef; >+ } else { >+ if (!pn->pn_defn) >+ return true; >+ dn = (JSDefinition *)pn; >+ } >+ >+ // Only optimize for defined globals. >+ if (!(dn->pn_dflags & PND_GVAR)) >+ return true; This part of BindGlobal is not needed for the call site in BindNameToSlot, which could pass dn as well as pn into a common subroutine factored out the tail of BindGlobal. MaybeBindGlobal and BindGlobal for the tail? Or TryToBindGlobal / BindGlobal? BindNameToSlot doesn't say "Maybe" or "Try", so perhaps go the other way: BindGlobal / BindKnownGlobal. >+ >+ // If the definition is bound, and we're in the same cg, we can re-use its >+ // cookie. >+ if (!dn->pn_cookie.isFree() && globalCg == cg) { >+ *cookie = dn->pn_cookie; >+ return true; >+ } >+ >+ uint32 index; >+ if (dn->pn_cookie.isFree()) { >+ // The definition wasn't bound, so find its atom's index in the >+ // mapping of defined globals. >+ JSAtomListElement *ale = globalScope->names.lookup(atom); >+ index = ALE_INDEX(ale); >+ } else { Don't test if (!dn->pn_ookie.isFree() twice, instead, move the if (globalCg == cg) with early returning then clause down here. Nit: globalcg wins over camelCaps. > op = PN_OP(pn3); > switch (op) { > case JSOP_GETARG: /* FALL THROUGH */ > case JSOP_SETARG: op = JSOP_FORARG; break; > case JSOP_GETLOCAL: /* FALL THROUGH */ > case JSOP_SETLOCAL: op = JSOP_FORLOCAL; break; >- case JSOP_GETGLOBAL: /* FALL THROUGH */ >- case JSOP_SETGLOBAL: op = JSOP_FORGLOBAL; break; >+ case JSOP_GETGLOBAL: >+ cookie.makeFree(); >+ op = JSOP_FORGNAME; >+ break; > default: JS_ASSERT(0); > } Starting to get ugly. Suggest indenting case-and-default-labeled statements to start in same column, and make op = JSOP_FORGNAME; cookie.makeFree(); break line up too (with op = JSOP_FORGNAME first). >@@ -1695,19 +1695,27 @@ fun_resolve(JSContext *cx, JSObject *obj > /* > * Assert that fun is not a compiler-created function object, which > * must never leak to script or embedding code and then be mutated. > * Also assert that obj is not bound, per the ES5 15.3.4.5 ref above. > */ > JS_ASSERT(!IsInternalFunctionObject(obj)); > JS_ASSERT(!obj->isBoundFunction()); > >- /* No need to reflect fun.prototype in 'fun.prototype = ... '. */ >- if (flags & JSRESOLVE_ASSIGNING) >+ /* >+ * No need to reflect fun.prototype in 'fun.prototype = ... '. >+ * NB: The interpreter/JIT get this property at whatever the first op >+ * of the function is, so inferred resolve flags could be bogus. >+ */ >+ if ((flags & JSRESOLVE_ASSIGNING) && >+ (!cx->fp() || >+ !cx->fp()->isConstructing() || >+ cx->fp()->maybeCallee() != obj)) { Painful, how about applying DeMorgan's Theorem? .. if ((flags & JSRESOLVE_ASSIGNING) && .. !(cx->fp() && cx->fp()->isConstructing() && cx->fp()->maybeCallee() == obj)) { Does this patch need rebasing? Out of time now. /be

Bug 621121 is a fix for INCGLOBAL/etc which are removed by the proposed fix, so I'm marking this as blocking that bug.

The first bad revision is: changeset: 96a84cd98d84 user: David Mandelin date: Thu Nov 11 16:51:30 2010 -0800 summary: Bug 584603: don't optimize names to JSOP_GETGLOBAL if the function contains JSOP_DEFFUN, r=dvander

blame is a red herring, this has been a bug since JM landed. Brendan, thanks for the comments - I'll rebase and re-r? soon.

addresses initial review comments

small refactoring from irc comments

Comment on attachment 500974 [details] [diff] [review] v1.2 actual patch >+// Binds a global, given a |dn| that is known to have the PND_GVAR bit, and a pn >+// that is |dn| or whose definition is |dn|. |cookie| is an outparam that will >+// be free (meaning no binding), or a slot number. >+static bool >+BindKnownGlobal(JSContext *cx, JSCodeGenerator *cg, JSParseNode *dn, JSParseNode *pn, JSAtom *atom) No more cookie outparam, so comment needs adjusting, minimally |pn->pn_cookie|. This raises an issue I should have pointed out already: BindNameToSlot always binds fully if it binds at all: pn->pn_cookie set, PND_BOUND set, pn_op specialized appropriately. The Bind*Global helpers set only pn_cookie if they can bind. Worth noting if not fixing. You could have the caller pass in the op too and consolidate the full PND_BOUND-flagged cookie and op specialization step instead of open-coding in the callers. >+ if (dn->isGlobal()) { >+ /* >+ * The locally stored cookie here should really come from |pn|, not >+ * |dn|. For example, we could have a SETGNAME op's lexdef be a >+ * GETGLOBAL op, and their cookies have very different meanings. As >+ * a workaround, just make the cookie free. >+ */ >+ cookie.makeFree(); Is this still needed? If so, move it down to >>>here<<<. >+ >+ if (op == JSOP_NAME) { >+ /* >+ * If the definition is a defined global, not potentially aliased >+ * by a local variable, and not mutating the variable, try and >+ * optimize to a fast, unguarded global access. >+ */ >+ if (!BindKnownGlobal(cx, cg, dn, pn, atom)) >+ return JS_FALSE; >+ if (!pn->pn_cookie.isFree()) { >+ pn->pn_op = JSOP_GETGLOBAL; >+ pn->pn_dflags |= PND_BOUND; >+ return JS_TRUE; >+ } >+ } >>>here<<< r=me at this point, with the above addressed. I don't see a bug and I've held this up too long. Thanks, /be

http://hg.mozilla.org/tracemonkey/rev/f497fca35415 w/ comments addressed

http://hg.mozilla.org/mozilla-central/rev/f497fca35415 http://hg.mozilla.org/mozilla-central/rev/f5b3294a3180

Filter on qa-project-auto-change: Bug in removed tracer code, setting in-testsuite- flag.