Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV

RESOLVED FIXED in Firefox 54

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: tcampbell)

Tracking

(Blocks: 5 bugs, {perf})

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
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.
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.
(Reporter)

Comment 2

3 years ago
(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.
(Reporter)

Comment 3

3 years ago
Shu mentions on IRC that we should wait for his frontend rewrite before we do this.
(Reporter)

Updated

3 years ago
Blocks: 1284590
(Reporter)

Updated

3 years ago
Blocks: 1304984
Blocks: 1307395
(Reporter)

Comment 4

3 years ago
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.
(Reporter)

Updated

3 years ago
Blocks: 1307062
Priority: -- → P3
(Reporter)

Updated

3 years ago
Duplicate of this bug: 942810
Keywords: perf
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.
Priority: P3 → P1
Realistically this will be for next release.
Priority: P1 → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → tcampbell
(Reporter)

Comment 8

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

Updated

2 years ago
Summary: Ion-compile JSOP_PUSHBLOCKSCOPE/JSOP_POPBLOCKSCOPE → Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV
(Assignee)

Updated

2 years ago
Attachment #8836172 - Flags: feedback?(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.

Updated

2 years ago
Attachment #8836172 - Flags: feedback?(shu)
See Also: → bug 1338910
Blocks: 1338910
See Also: bug 1338910
Comment hidden (mozreview-request)
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
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.
Flags: needinfo?(jdemooij)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8838282 - Flags: review?(jdemooij)
Attachment #8836172 - Flags: review?(jdemooij)
Attachment #8838283 - Flags: review?(jdemooij)
Attachment #8838379 - Flags: review?(jdemooij)
(Assignee)

Comment 18

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

Comment 19

2 years ago
The work-around for exceptions needs some work. I'm getting a few assertions in some cases. I'll investigate further.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
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.
Flags: needinfo?(jdemooij)
(Reporter)

Comment 25

2 years ago
mozreview-review
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).
Attachment #8836172 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 26

2 years ago
mozreview-review
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 ...;
Attachment #8838283 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 27

2 years ago
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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(tcampbell)
(Reporter)

Comment 28

2 years ago
mozreview-review
Comment on attachment 8838379 [details]
Bug 1273858 - Testcases

https://reviewboard.mozilla.org/r/113320/#review115570
Attachment #8838379 - Flags: review?(jdemooij) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

2 years ago
Alternate patch for unwinding environment using FinishBailoutToBaseline instead.
Flags: needinfo?(tcampbell)
Attachment #8839931 - Flags: review?(jdemooij)
(Reporter)

Comment 33

2 years ago
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.
Attachment #8839931 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 34

2 years ago
mozreview-review
Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

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

(I reviewed the second version.)
Attachment #8838282 - Flags: review?(jdemooij)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 39

2 years ago
mozreview-review
Comment on attachment 8838282 [details]
Bug 1273858 - Support LexicalEnvironmentObjects during Ion bailout

https://reviewboard.mozilla.org/r/113224/#review115940
Attachment #8838282 - Flags: review?(jdemooij) → review+

Comment 40

2 years ago
Pushed by jandemooij@gmail.com:
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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1353648
Blocks: 1366470
Depends on: 1343723
You need to log in before you can comment on or make changes to this bug.