Closed
Bug 1355056
Opened 8 years ago
Closed 8 years ago
Replace (function(args) { /* do stuff using this */ }).bind(this) with arrow functions
Categories
(Firefox :: General, enhancement, P1)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-performance])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details |
It should be a perf and memory win, and easy to script.
Comment 1•8 years ago
|
||
Can you please get some timings? It's easy to believe this can be a memory win, but according to sixspeed we're still slower on some aspects of arrow functions, for example on this test case <https://github.com/kpdecker/six-speed/blob/master/tests/arrow-declare/arrow-declare.es6> compared to normal functions, so I'd like to back up the perf win assertion with some measurements if possible. :-) Thanks!
Flags: needinfo?(florian)
Assignee | ||
Comment 2•8 years ago
|
||
Benjamin, could you please share here the data you gave me over email, to answer Ehsan's question?
Flags: needinfo?(florian) → needinfo?(bbouvier)
Updated•8 years ago
|
Comment 3•8 years ago
|
||
Well, the JavaScript team is paying close attention to performance of arrow functions, in particular using AWFY's SixSpeed benchmark: https://arewefastyet.com/#machine=29&view=breakdown&suite=six-speed
Jandem blogged about making arrow functions faster two years ago ( https://jandemooij.nl/blog/2014/04/11/fast-arrow-functions-in-firefox-31/ ) and many other improvements have been made in the meanwhile. I've asked yesterday and I've been told that performance of arrow functions should almost be the same as non-arrow functions. For what it's worth, bound functions are also getting jitted nowadays, but we've regressed their performance recently (see also bug 1355096). If something else gets problematic in real-world front-end code when using arrow functions, please open a new bug in the JavaScript engine component; it is an explicit goal of the JS team to make ES6 faster so this would help.
Flags: needinfo?(bbouvier)
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Updated•8 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify? → qe-verify-
Priority: P2 → P1
Assignee | ||
Comment 4•8 years ago
|
||
I tried to benchmark this to sanity check the approach before I start working on the actual replacement, and the results are surprising.
Using a closure with a self variable, or using an arrow function has about the same speed. Using .bind is an order of magnitude faster.
Here is a profile of the attached benchmark: https://perfht.ml/2pG8231
Benjamin, am I doing something wrong that doesn't reflect a real life situation in this benchmark? If not, is there room for optimization?
Flags: needinfo?(bbouvier)
Comment 5•8 years ago
|
||
That's beyond my knowledge of the current implementation state; cc-ing evilpie/jandem who might know more.
Flags: needinfo?(bbouvier)
Comment 6•8 years ago
|
||
Arrow functions should definitely be faster (and allocate less) than (function() {..}).bind(this)
Comment 7•8 years ago
|
||
Note that the `f1` and `f2` properties are missing in the objects for the self and arrow methods.
Comment 8•8 years ago
|
||
These benchmarks paint a different picture: https://jsperf.com/bind-vs-arrow-2 and https://jsperf.com/bind-vs-arrow-3/.
arrow > self > bind
If you add `f1` and `f2` to all your objects, both the `self` and the `arrow` options become way faster, but for some reason the `arrow` option is still slower.
self > bind > arrow
Comment 9•8 years ago
|
||
These micro-benchmarks are not super useful though because JS engines will loop hoist or dead-code eliminate (some of) these functions.
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8859982 [details]
benchmark
(In reply to Marco Castelluccio [:marco] from comment #7)
> Note that the `f1` and `f2` properties are missing in the objects for the
> self and arrow methods.
Yes, my benchmark is broken, thanks for spotting this!
Given comment 6 and comment 8, I think we should go ahead and replace .bind(this) with arrow functions. Thanks!
Attachment #8859982 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8862055 -
Flags: review?(jaws)
Assignee | ||
Comment 12•8 years ago
|
||
Separate attachment for the fixes I made by hand after running the script.
Try seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d200fe4b9f60fca1616847c7def0eff5a6f3fddb
Except for an eslint failure I fixed before attaching, and for a Windows failure due to another patch that got backed out (https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea39575ae0c)
Attachment #8862056 -
Flags: review?(jaws)
Assignee | ||
Comment 13•8 years ago
|
||
Updated•8 years ago
|
Attachment #8862055 -
Flags: review?(jaws) → review+
Updated•8 years ago
|
Attachment #8862056 -
Flags: review?(jaws) → review+
Comment 14•8 years ago
|
||
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f97f76c0b34
replace (function(args) { /* do stuff using this */ }).bind(this) with arrow functions, r=jaws.
Comment 15•8 years ago
|
||
Since this doesn't have any protection from introducing new binds, I wonder if we could go further and use eslint prefer-arrow-callback, maybe with the allowNamedFunctions: true option initially (--fix could help fixing most of the code)
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #15)
> Since this doesn't have any protection from introducing new binds, I wonder
> if we could go further and use eslint prefer-arrow-callback, maybe with the
> allowNamedFunctions: true option initially (--fix could help fixing most of
> the code)
The reason I didn't try to add eslint protection here is that we still have plenty of .bind(this) calls in our code on generator functions used as tasks, and these don't support the arrow function syntax. I intend to clean that up as part of bug 1353542, as async functions do support the arrow function syntax.
That said, if prefer-arrow-callback doesn't choke on generators using bind, I would be all for enabling it. I don't think we need "allowNamedFunctions: true", my script removed function names when they were unused, and eslint won't report them if they are used.
Comment 17•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> That said, if prefer-arrow-callback doesn't choke on generators using bind,
> I would be all for enabling it.
It shouldn't, based on its documentation.
Comment 18•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Updated•7 years ago
|
No longer blocks: photon-performance-triage
Assignee | ||
Updated•7 years ago
|
Blocks: photon-performance-triage
You need to log in
before you can comment on or make changes to this bug.
Description
•