1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Jan de Mooij [:jandem]]

We abort Ion compilation of a function in the Knockout framework because of this. It's annex b code: they have a function statement in an if-statement.

Probably pretty common in the wild, so it's worth fixing.

Comment 1

[Morgan Phillips [:mrrrgn]]

I'm interested in taking this. I will be on PTO for ~1.5 weeks, if this becomes a priority before then please feel free to grab the bug. Otherwise I'll take it once I return.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Morgan Phillips [:mrrrgn] from comment #1)
> I'm interested in taking this.

Great! Some hints:

For PUSHBLOCKSCOPE, we have to add a new MIR instruction that calls ClonedBlockObject::create. The MIR instruction will take the current (dynamic) scope chain as operand, and it will also need a field for the static block object. See BaselineCompiler::emit_JSOP_PUSHBLOCKSCOPE for how to get the latter in IonBuilder.

For POPBLOCKSCOPE we can just do |current->setScopeChain(MEnclosingScope::New(...));| I think.

Comment 3

[Jan de Mooij [:jandem]]

Shu mentions on IRC that we should wait for his frontend rewrite before we do this.

Comment 4

[Jan de Mooij [:jandem]]

This comes up with ES6 stuff, but also with ES5 JS (see comment 0). I'll try to take a stab at it this week.

Comment 6

[Hannes Verschore [:h4writer]]

In ES6 performance stuff this is one of the next things that should happen (after baseline compiling ES6 classes). Could be pushed to P2, but further would be a stretch.

Comment 7

[Hannes Verschore [:h4writer]]

Realistically this will be for next release.

Comment 8

[Jan de Mooij [:jandem]]

Note that we have the following ops to consider now:

* JSOP_PUSHLEXICALENV
* JSOP_POPLEXICALENV
* JSOP_FRESHENLEXICALENV
* JSOP_RECREATELEXICALENV

We should check when these are emitted and which ones we should implement first.

Thanks for taking this on \o/. It's not an easy one, though.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> * JSOP_PUSHLEXICALENV

On entry to any block or for-loop with block-scoped variable, where a block-scoped variable may be captured by a nested function or eval.

> * JSOP_POPLEXICALENV

Same, but for exit.

> * JSOP_FRESHENLEXICALENV

At for(;;) loop edge when the loop head declares a |let| variable that may be captured by a nested function or eval.

> * JSOP_RECREATELEXICALENV

At for-in/of loop edge when the loop head declares a block-scoped variable that may be captured by a nested function or eval.

First two are obviously most important.  Not sure which of last two is best done first, would just pick one and move on.

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review commit: https://reviewboard.mozilla.org/r/111610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111610/

Comment 11

[Shu-yu Guo [:shu]]

Ted and I discussed the bailout HAS_INITIAL_ENV thing over video conference. I *think* the presence of any non-extensible LexicalEnv implies HAS_INITIAL_ENV, we'll see if there're any test failures.

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111610/diff/1-2/

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

Review commit: https://reviewboard.mozilla.org/r/113224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113224/

Comment 14

[Ted Campbell [:tcampbell]]

Attachment: Bug 1273858 - Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV

Review commit: https://reviewboard.mozilla.org/r/113226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113226/

Comment 15

[Ted Campbell [:tcampbell]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111610/diff/2-3/

Comment 16

[Ted Campbell [:tcampbell]]

The working patches are up. Can you give feedback particularly on the bailout changes? I'll try to put together some new jit-tests.

The environment unwinding logic doesn't really have a good home. It would be nice to put it in HandleExceptionIon, but there it is too difficult to update the environment chain pointer.

Comment 17

[Ted Campbell [:tcampbell]]

Attachment: Bug 1273858 - Testcases

Review commit: https://reviewboard.mozilla.org/r/113320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113320/

Comment 18

[Ted Campbell [:tcampbell]]

Running this in a browser sessions, I wasn't seeing much in the wild in a few minutes of browsing, but I did see a lot of activity from the browser chrome making use of Ion-compiling where it previously was unable to.

Comment 19

[Ted Campbell [:tcampbell]]

The work-around for exceptions needs some work. I'm getting a few assertions in some cases. I'll investigate further.

Comment 20

[Ted Campbell [:tcampbell]]

Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113224/diff/1-2/

Comment 21

[Ted Campbell [:tcampbell]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111610/diff/3-4/

Comment 22

[Ted Campbell [:tcampbell]]

Comment on attachment 8838283 [details]
Bug 1273858 - Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113226/diff/1-2/

Comment 23

[Ted Campbell [:tcampbell]]

Comment on attachment 8838379 [details]
Bug 1273858 - Testcases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113320/diff/1-2/

Comment 24

[Ted Campbell [:tcampbell]]

Okay, I think I finally sorted out the errant assertions in the Unwind code. It is refactored a bit to clean up too. Should be ready for review now. There is no rush, though.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

https://reviewboard.mozilla.org/r/111610/#review115548

Looks great, just some nits.

::: js/src/jit/CodeGenerator.cpp:3442
(Diff revision 4)
> +    FunctionInfo<NewLexicalEnvironmentObjectFn>(LexicalEnvironmentObject::createTemplateObject,
> +                                                "LexicalEnvironmentObject::createTemplateObject");

Nit: the createTemplateObject name is a bit weird, it suggests it's something we call once for a given scope. Please rename this overload to just create() as we're not using LexicalEnvironment template objects anywhere AFAIK.

::: js/src/jit/MIR.h:11499
(Diff revision 4)
> +    MNewLexicalEnvironmentObject(MDefinition* enclosing, LexicalScope* scope)
> +      : MUnaryInstruction(enclosing),
> +        scope_(scope)
> +    {
> +        setResultType(MIRType::Object);
> +        setMovable();

Nit: I'd remove this line (this instruction has isEffectful() returning true because we don't define an alias set for it, so it won't make any difference).

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8838283 [details]
Bug 1273858 - Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV

https://reviewboard.mozilla.org/r/113226/#review115568

Nice that we can handle these with the same MIR.

::: js/src/jit/MIR.h:11530
(Diff revision 2)
> +    MCopyLexicalEnvironmentObject(MDefinition* env, bool copySlots)
> +      : MUnaryInstruction(env),
> +        copySlots_(copySlots)
> +    {
> +        setResultType(MIRType::Object);
> +        setMovable();

Nit: I'd remove this line here too.

::: js/src/jit/VMFunctions.cpp:930
(Diff revision 2)
>  }
>  
>  JSObject*
> +CopyLexicalEnvironmentObject(JSContext* cx, HandleObject env, bool copySlots)
> +{
> +    Rooted<LexicalEnvironmentObject*> lexicalEnv(cx, &env->as<LexicalEnvironmentObject>());

Nit: can eliminate a Rooted here:

Handle<LexicalEnvironmentObject\*> lexicalEnv = env.as<LexicalEnvironmentObject>();

Or change the env argument to a Handle<LexicalEnvironmentObject\*>.

I also wonder if we should call clone/recreate directly in CodeGenerator? CopyLexicalEnvironmentObject is pretty simple but these non-inlined calls do add some amount of overhead.

::: js/src/jit/VMFunctions.cpp:933
(Diff revision 2)
> +        return LexicalEnvironmentObject::clone(cx, lexicalEnv);
> +    else
> +        return LexicalEnvironmentObject::recreate(cx, lexicalEnv);

Nit: no else after return.

if (copySlots)
    return ...;
return ...;

Comment 27

[Jan de Mooij [:jandem]]

The bailout patch looks reasonable but I wonder if it would be easier to call UnwindEnvironment in FinishBailoutToBaseline if cx->isExceptionPending(). Would you mind trying that to see if it works? I didn't look closely so it's possible I'm missing something.

(In reply to Ted Campbell [:tcampbell] from comment #18)
> Running this in a browser sessions, I wasn't seeing much in the wild in a
> few minutes of browsing, but I did see a lot of activity from the browser
> chrome making use of Ion-compiling where it previously was unable to.

Ah yeah, chrome JS uses let/const everywhere, so this might affect Talos results.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8838379 [details]
Bug 1273858 - Testcases

https://reviewboard.mozilla.org/r/113320/#review115570

Comment 29

[Ted Campbell [:tcampbell]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111610/diff/4-5/

Comment 30

[Ted Campbell [:tcampbell]]

Comment on attachment 8838283 [details]
Bug 1273858 - Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113226/diff/2-3/

Comment 31

[Ted Campbell [:tcampbell]]

Comment on attachment 8838379 [details]
Bug 1273858 - Testcases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113320/diff/2-3/

Comment 32

[Ted Campbell [:tcampbell]]

Attachment: Support-LexicalEnvironmentObjects-during-bailout-alternative.patch

Alternate patch for unwinding environment using FinishBailoutToBaseline instead.

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8839931 [details] [diff] [review]
Support-LexicalEnvironmentObjects-during-bailout-alternative.patch

Review of attachment 8839931 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thanks!

::: js/src/jit/BaselineBailouts.cpp
@@ +734,5 @@
> +
> +            // If Ion has updated env slot from UndefinedValue, it will be the
> +            // complete initial environment, so we can set the HAS_INITIAL_ENV
> +            // flag if needed.
> +            if (fun && fun->needsFunctionEnvironmentObjects()) {

This is so much simpler.

::: js/src/jit/BaselineJIT.h
@@ +591,5 @@
>  
>      // The bytecode pc where we will resume.
>      jsbytecode* resumePC;
>  
> +    // The bytecode pc of try block and fault block

Nit: add a full stop at the end.

Comment 34

[Jan de Mooij [:jandem]]

Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

https://reviewboard.mozilla.org/r/113224/#review115934

(I reviewed the second version.)

Comment 35

[Ted Campbell [:tcampbell]]

Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113224/diff/2-3/

Comment 36

[Ted Campbell [:tcampbell]]

Comment on attachment 8836172 [details]
Bug 1273858 - Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111610/diff/5-6/

Comment 37

[Ted Campbell [:tcampbell]]

Comment on attachment 8838283 [details]
Bug 1273858 - Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113226/diff/3-4/

Comment 38

[Ted Campbell [:tcampbell]]

Comment on attachment 8838379 [details]
Bug 1273858 - Testcases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113320/diff/3-4/

Comment 39

[Jan de Mooij [:jandem]]

Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

https://reviewboard.mozilla.org/r/113224/#review115940

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fb07ea247ba
Support LexicalEnvironmentObjects during Ion bailout r=jandem
https://hg.mozilla.org/integration/autoland/rev/da80011188ed
Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV r=jandem
https://hg.mozilla.org/integration/autoland/rev/0a3a0f9eb773
Ion-compile JSOP_FRESHENLEXICALENV/JSOP_RECREATELEXICALENV r=jandem
https://hg.mozilla.org/integration/autoland/rev/a6570d4652e6
Testcases r=jandem

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fb07ea247ba
https://hg.mozilla.org/mozilla-central/rev/da80011188ed
https://hg.mozilla.org/mozilla-central/rev/0a3a0f9eb773
https://hg.mozilla.org/mozilla-central/rev/a6570d4652e6