Closed
Bug 1273858
Opened 8 years ago
Closed 7 years ago
Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jandem, Assigned: tcampbell)
References
(Blocks 4 open bugs)
Details
(Keywords: perf)
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
jandem
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jandem
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jandem
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jandem
:
review+
|
Details |
7.92 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 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•8 years ago
|
||
Shu mentions on IRC that we should wait for his frontend rewrite before we do this.
Reporter | ||
Comment 4•8 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•8 years ago
|
Blocks: sm-js-perf
Priority: -- → P3
Comment 6•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tcampbell
Reporter | ||
Comment 8•7 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.
Comment 9•7 years ago
|
||
(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•7 years ago
|
Summary: Ion-compile JSOP_PUSHBLOCKSCOPE/JSOP_POPBLOCKSCOPE → Ion-compile JSOP_PUSHLEXICALENV/JSOP_POPLEXICALENV
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8836172 -
Flags: feedback?(shu)
Comment 11•7 years ago
|
||
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•7 years ago
|
Attachment #8836172 -
Flags: feedback?(shu)
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Flags: needinfo?(tcampbell)
Reporter | ||
Comment 28•7 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•7 years ago
|
||
Alternate patch for unwinding environment using FinishBailoutToBaseline instead.
Flags: needinfo?(tcampbell)
Attachment #8839931 -
Flags: review?(jdemooij)
Reporter | ||
Comment 33•7 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•7 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•7 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•7 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•7 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
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•