Bug 663138 added an optimization to greatly improve JM's performance on reentrant closures. With luke's work on bug 659577 and related stuff JM's closure perf can be improved (by generating code to directly walk the scope chain and avoiding an IC), and IM even more so --- with GVN and LICM on scope-walking code the savings from bug 663138 would be effectively unnecessary. This optimization is also incurring significant cost. It adds significant complexity, incurs some runtime overhead (the reentrance analysis is dynamic) and is getting in the way of lazy bytecode optimization (bug 678037). The optimization and dependent code should just be removed.

Awesome; were you planning to do this soon? If not, I was planning to remove it for bug 753158. I had actually started by doing bug 755186 which I think is required to remove script->globalObject and TypeObject::global, but I'd be really happy for you to do the deed.

I wasn't planning on doing this immediately, but can do it myself if necessary. Mainly recording the dependency as this blocks parts of lazy bytecode compilation (MarkInnerAndOuterFunctions needs to go away). Removing it as part of bug 753158 might be nice as it would lower the perf regression, also that bug should take care not to introduce other conflicts with bug 678037.

Mmmmmm cx->compartment->global()

Mmm tasty: 31 files changed, 156 insertions(+), 1154 deletions(-) Need to land these changes with bug 753158.

Comment on attachment 635994 [details] [diff] [review] rm optimization Review of attachment 635994 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ -1104,5 @@ > > script->compileAndGo = compileAndGo; > script->noScriptRval = noScriptRval; > > - script->globalObject = globalObject; You should now be able to remove the |globalObject| parameter of JSScript::Create(). And then you'll be able to remove the |needScriptGlobal| parameter of frontend::CompileScript(). ::: js/src/jsscript.h @@ +409,5 @@ > JSPrincipals *principals;/* principals for this script */ > JSPrincipals *originPrincipals; /* see jsapi.h 'originPrincipals' comment */ > > + /* The next link in the eval cache */ > + js::HeapPtrScript evalHashLink; You said you would remove JSScript::globalObject, but you didn't say you'd replace it with evalHashLink! :P @@ -517,5 @@ > undefined properties in this > script */ > bool hasSingletons:1; /* script has singleton objects */ > - bool isOuterFunction:1; /* function is heavyweight, with inner functions */ > - bool isInnerFunction:1; /* function is directly nested in a heavyweight BTW, this reminds me a bit of bug 754641, so please test this on 32 bit and 64 bit and --disable-methodjit, because the removal of the 3 bit fields might be enough to require some padding to change on one of those configurations.

Comment on attachment 635994 [details] [diff] [review] rm optimization Review of attachment 635994 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsinfer.h @@ -1002,5 @@ > - * Information about the scope in which a script executes. This information > - * is not set until the script has executed at least once and SetScope > - * called, before that 'global' will be poisoned per GLOBAL_MISSING_SCOPE. > - */ > - static const size_t GLOBAL_MISSING_SCOPE = 0x1; I'm pretty sure billm had to work around this GLOBAL_MISSING_SCOPE junk in a few places, you might want to check with him if these can be fixed now. ::: js/src/jsinferinlines.h @@ +1331,5 @@ > +// return &singleton->global(); > +// if (interpretedFunction && interpretedFunction->script()->compileAndGo) > +// return &interpretedFunction->global(); > +// return NULL; > +//} rm

(In reply to Nicholas Nethercote [:njn] from comment #7) > You said you would remove JSScript::globalObject, but you didn't say you'd > replace it with evalHashLink! :P Oh yeah, I forgot to file bug 767750.

JSScript::getGlobalObjectOrNull has subtle semantics (not implied by the name) for when it is null. Rather than have yet another variation of "does this script have a global for some definition of 'have'", I'd like to inline it b/c it makes more sense in the context of its use: the debugger. I suspect there is a lot more code that can be simplified now with CPG.

(This patch fixes some devvtools/debugger bustage introduced by the previous patches.)

With that change, green on try.

Is this relatively low-priority?

It blocks bug 753158 and bug 767013, which are benchmarking priorities. When bug 753158 is complete, I'll land them together.

Comment on attachment 636498 [details] [diff] [review] rm JSScript::getGlobalObjectOrNull Please file follow-up bug to immortalize CPG in song.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf7aa93994c https://hg.mozilla.org/integration/mozilla-inbound/rev/cd782fd66995

Unfortunately bug 772303 (https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5) had to be backed out for winxp pgo-only jsreftest failures. This bug (amongst others) either caused unresolvable (for someone who doesn't work on the JS engine at least) conflicts with the backout of fad7d06d7dd5, or else bugzilla dependencies indicated that one of it's dependants had now been backed out. Since there was no one in #developers that could resolve the conflicts, unfortunately this bug has been backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0be4b70b814

https://hg.mozilla.org/integration/mozilla-inbound/rev/749d103d8636 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9650bc4da1a

https://hg.mozilla.org/mozilla-central/rev/749d103d8636 https://hg.mozilla.org/mozilla-central/rev/d9650bc4da1a

RESOLVED FIXED?

Yes,sorry.