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)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.4 - May 1
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(3 files, 1 obsolete file)

It should be a perf and memory win, and easy to script.
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)
Benjamin, could you please share here the data you gave me over email, to answer Ehsan's question?
Flags: needinfo?(florian) → needinfo?(bbouvier)
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)
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify? → qe-verify-
Priority: P2 → P1
Attached file benchmark (obsolete) (deleted) —
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)
That's beyond my knowledge of the current implementation state; cc-ing evilpie/jandem who might know more.
Flags: needinfo?(bbouvier)
Arrow functions should definitely be faster (and allocate less) than (function() {..}).bind(this)
Note that the `f1` and `f2` properties are missing in the objects for the self and arrow methods.
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
These micro-benchmarks are not super useful though because JS engines will loop hoist or dead-code eliminate (some of) these functions.
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
Attached patch script-generated patch (deleted) — Splinter Review
Attachment #8862055 - Flags: review?(jaws)
Attached patch hand-cleanup (deleted) — Splinter Review
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)
Attached file xpcshell script (deleted) —
Attachment #8862055 - Flags: review?(jaws) → review+
Attachment #8862056 - Flags: review?(jaws) → review+
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.
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)
(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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
No longer blocks: photon-performance-triage
Depends on: 1378693
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: