The default bug view has changed. See this FAQ.

Remove the non-reentrant closure optimization

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: luke)

Tracking

Other Branch
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments)

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.
(Assignee)

Comment 1

5 years ago
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.
Blocks: 753158
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.
Whiteboard: [js:t]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762091
(Assignee)

Updated

5 years ago
Blocks: 767013
(Assignee)

Comment 4

5 years ago
Created attachment 635992 [details] [diff] [review]
rm dead code
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #635992 - Flags: review?(bhackett1024)
(Assignee)

Comment 5

5 years ago
Created attachment 635993 [details] [diff] [review]
set Bindings' parent eagerly

Mmmmmm cx->compartment->global()
Attachment #635993 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

5 years ago
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.
Attachment #635994 - Flags: review?(bhackett1024)
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.
Attachment #635992 - Flags: review?(bhackett1024) → review+
Attachment #635993 - Flags: review?(bhackett1024) → review+
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
Attachment #635994 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

5 years ago
Blocks: 767750
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #636498 - Flags: review?(jorendorff)
(Assignee)

Comment 11

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

Comment 12

5 years ago
With that change, green on try.
Summary: Remove the reentrant closure optimization → Remove the non-reentrant closure optimization

Comment 13

5 years ago
Is this relatively low-priority?
(Assignee)

Comment 14

5 years ago
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.
Attachment #636498 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf7aa93994c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd782fd66995
Target Milestone: --- → mozilla16
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
Target Milestone: mozilla16 → ---
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/749d103d8636
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9650bc4da1a
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/749d103d8636
https://hg.mozilla.org/mozilla-central/rev/d9650bc4da1a
Flags: in-testsuite-

Comment 20

5 years ago
RESOLVED FIXED?
Yes,sorry.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 773929
You need to log in before you can comment on or make changes to this bug.