Closed
Bug 1506524
Opened 6 years ago
Closed 6 years ago
Arrow functions 6 times as slow as on Chrome or equivalent method
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jya, Assigned: anba)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
590 bytes,
application/x-javascript
|
Details | |
1.21 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
Comment 1•6 years ago
|
||
Link to reproduce: https://jsperf.com/function-vs-arrow-function I can attest that I experienced similar slowdowns (~70%).
Weird one: Normal profile - 83% slower Addons disabled - as above New clean profile - as above Safe mode - arrow functions are 1% faster
Comment 3•6 years ago
|
||
Running this in the shell I see the exact same performance for both arrow and normal functions. Looking at the JIT spew, we compile away everything inside the loop. So this is probably some bad interaction with jsperf (again)
Comment 4•6 years ago
|
||
I can repro that on nightly, the results from jspref show arrows as slower than simple functions. This is really bizarre - as evilpie noted we should be handling these and we do seem to be. This is on nightly (65) Linux x86-64, with add-ons: uBlock, multi-account containers, gecko profiler, and temporary containers. Tom, do you have a sense for the kinds of "weird jsperf interactions" that led to differential results from shell execution?
Priority: -- → P3
Assignee | ||
Comment 5•6 years ago
|
||
MFunctionEnvironment::foldsTo [1] doesn't handle MLambdaArrow, which prevents moving the FunctionEnvironment out of the loop. The following change fixes the performance issue for me, but I'm not 100% sure why MLambdaArrow wasn't handled there (Did we simply forget to handle it or is it actually not allowed to handle MLambdaArrow for MFunctionEnvironment::foldsTo?) --- MDefinition* MFunctionEnvironment::foldsTo(TempAllocator& alloc) { if (input()->isLambda()) { return input()->toLambda()->environmentChain(); } if (input()->isLambdaArrow()) { return input()->toLambdaArrow()->environmentChain(); } return this; } --- [1] https://searchfox.org/mozilla-central/rev/3564dfde3b125878dc5a04fe92629fc5195942df/js/src/jit/MIR.cpp#5086-5092
Assignee | ||
Comment 6•6 years ago
|
||
The equivalent of bug 1069260, but this time for MLambdaArrow instead of MLambda. Do you know if there's anything special about arrow-functions which prevents handling them in MFunctionEnvironment::foldsTo?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9029581 -
Flags: review?(nicolas.b.pierron)
Comment 7•6 years ago
|
||
Comment on attachment 9029581 [details] [diff] [review] bug1506524.patch Review of attachment 9029581 [details] [diff] [review]: ----------------------------------------------------------------- I think I might just have forgotten the case when I implemented it. Thanks for catching and fixing this issue.
Attachment #9029581 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b2293cd425b5b2fdf50859abd4ccdcda91970a
Keywords: checkin-needed
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8be0d3702f65 Handle LambdaArrow in MFunctionEnvironment::foldsTo. r=nbp
Keywords: checkin-needed
![]() |
||
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8be0d3702f65
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•