Replace (function(args) { /* do stuff using this */ }).bind(this) with arrow functions

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months ago
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)
(Assignee)

Comment 2

3 months ago
Benjamin, could you please share here the data you gave me over email, to answer Ehsan's question?
Flags: needinfo?(florian) → needinfo?(bbouvier)
See Also: → bug 1355096
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

2 months ago
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]

Updated

2 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify? → qe-verify-
Priority: P2 → P1
(Assignee)

Comment 4

2 months ago
Created attachment 8859982 [details]
benchmark

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

Comment 10

2 months 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

2 months ago
Created attachment 8862055 [details] [diff] [review]
script-generated patch
Attachment #8862055 - Flags: review?(jaws)
(Assignee)

Comment 12

2 months ago
Created attachment 8862056 [details] [diff] [review]
hand-cleanup

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

2 months ago
Created attachment 8862059 [details]
xpcshell script
Attachment #8862055 - Flags: review?(jaws) → review+
Attachment #8862056 - Flags: review?(jaws) → review+

Comment 14

2 months 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.
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

2 months 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.
(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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0f97f76c0b34
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

2 months ago
No longer blocks: 1348289
(Assignee)

Updated

2 months ago
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.