As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 765956 - Remove the non-reentrant closure optimization
: Remove the non-reentrant closure optimization
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
: 762091 (view as bug list)
Depends on: 773929
Blocks: LazyBytecode 753158 767013 767750
  Show dependency treegraph
Reported: 2012-06-18 16:29 PDT by Brian Hackett (:bhackett)
Modified: 2012-07-14 01:11 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

rm dead code (3.06 KB, patch)
2012-06-22 18:22 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
set Bindings' parent eagerly (4.84 KB, patch)
2012-06-22 18:24 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
rm optimization (99.07 KB, patch)
2012-06-22 18:27 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
rm JSScript::getGlobalObjectOrNull (10.58 KB, patch)
2012-06-25 14:58 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2012-06-18 16:29:55 PDT
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.
Comment 1 User image Luke Wagner [:luke] 2012-06-18 18:35:17 PDT
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.
Comment 2 User image Brian Hackett (:bhackett) 2012-06-18 18:44:13 PDT
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.
Comment 3 User image Luke Wagner [:luke] 2012-06-20 14:28:24 PDT
*** Bug 762091 has been marked as a duplicate of this bug. ***
Comment 4 User image Luke Wagner [:luke] 2012-06-22 18:22:49 PDT
Created attachment 635992 [details] [diff] [review]
rm dead code
Comment 5 User image Luke Wagner [:luke] 2012-06-22 18:24:10 PDT
Created attachment 635993 [details] [diff] [review]
set Bindings' parent eagerly

Mmmmmm cx->compartment->global()
Comment 6 User image Luke Wagner [:luke] 2012-06-22 18:27:20 PDT
Created attachment 635994 [details] [diff] [review]
rm optimization

Mmm tasty:
 31 files changed, 156 insertions(+), 1154 deletions(-)

Need to land these changes with bug 753158.
Comment 7 User image Nicholas Nethercote [:njn] 2012-06-22 19:28:56 PDT
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 8 User image Brian Hackett (:bhackett) 2012-06-23 08:19:45 PDT
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;
> +//}

Comment 9 User image Luke Wagner [:luke] 2012-06-23 18:17:22 PDT
(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.
Comment 10 User image Luke Wagner [:luke] 2012-06-25 14:58:53 PDT
Created attachment 636498 [details] [diff] [review]
rm JSScript::getGlobalObjectOrNull

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.
Comment 11 User image Luke Wagner [:luke] 2012-06-25 14:59:45 PDT
(This patch fixes some devvtools/debugger bustage introduced by the previous patches.)
Comment 12 User image Luke Wagner [:luke] 2012-06-25 19:27:04 PDT
With that change, green on try.
Comment 13 User image Paul Wright 2012-07-05 21:28:56 PDT
Is this relatively low-priority?
Comment 14 User image Luke Wagner [:luke] 2012-07-05 21:36:02 PDT
It blocks bug 753158 and bug 767013, which are benchmarking priorities.  When bug 753158 is complete, I'll land them together.
Comment 15 User image Jason Orendorff [:jorendorff] 2012-07-11 16:10:17 PDT
Comment on attachment 636498 [details] [diff] [review]
rm JSScript::getGlobalObjectOrNull

Please file follow-up bug to immortalize CPG in song.
Comment 17 User image Ed Morley [:emorley] 2012-07-12 05:24:25 PDT
Unfortunately bug 772303 ( 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:
Comment 20 User image Paul Wright 2012-07-13 05:53:42 PDT
Comment 21 User image Ryan VanderMeulen [:RyanVM] 2012-07-13 06:18:31 PDT

Note You need to log in before you can comment on or make changes to this bug.