lib: use arrow functions instead of bind by JungMinu · Pull Request #3622 · nodejs/node · GitHub

use arrow functions instead of bind(this) in order to improve
performance through optimizations.

I don't think you should touch anything in deps/npm/node_modules/ and tools/eslint/ since they contain third-party dependencies.

Yes, please be sure not to touch anything in deps/ or tools/eslint

@JungMinu instead of making a new PR you can just amend you changes (via git add and git commit --amend) and then force push it to this branch.

@JungMinu actually; even more specific: please only do files in lib/ -- a change to the tests wouldn't be worth the change size I think.

Looks like the file permissions for the files in deps/ and tools/ are still being changed.

Also, have you done any benchmarking to show before and after these changes? Looking at the diff, I'm not sure that these changes actually affect any hot code paths.

Except for the last sentence, the PR text appears to be copied from https://www.nczonline.net/blog/2013/09/10/understanding-ecmascript-6-arrow-functions/. If you want to use someone else's text verbatim, please remember to cite it.

CI: https://ci.nodejs.org/job/node-test-pull-request/656/

I'd be keen to see that benchmark as well before landing this.

The win here, at least the way I see it, is in readability by getting rid of all the .bind(this) stuff. That's my opinion though. I wouldn't argue with someone else who thought that removing the explicit binding introduced too much magic. (I wouldn't agree, but I wouldn't argue either.)

As noted by @mscdex, none of this looks like it's in a hot path and so I very much doubt any of this will result in a measurable performance boost. Benchmark proving otherwise welcome, but it seems like the benefit here is a the readability/maintainability gain.

IMHO as far as readability goes, it's about the same, maybe worse (but that would be more of a general ES6 problem).

I guess where I was heading was that if this goes in with "faster" as the commit message it should be properly warranted. Style "fixes" is another category.

bind() is known to be slow, so if nothing else it makes institution faster. I'll look into the perf more, but also cc @trevnorris who originally suggested this kind of change

On Nov 1, 2015, at 6:39 PM, Johan Bergström notifications@github.com wrote:

I guess where I was heading was that if this goes in with "faster" as the commit message it should be properly warranted. Style "fixes" is another category.

—
Reply to this email directly or view it on GitHub.

Minus the file permission changes, LGTM. As @mscdex mentioned, it's not an improvement in readability, but that's not what we're going after. In these specific locations there's nothing hot so won't notice a performance gain, but might as well make the change where it makes sense.

@JungMinu I'm not sure you caught every case of .bind() in the files under the lib folder. When I did a search I came up with 111 matches across 32 files. In addition, could you also check for any cases in src/node.js -- that's the only relevant file that is weirdly placed, I think.

Also how did you edit the files? I'm not sure what would have caused those mode changes.

Oh, maybe those mode changes were from previously changing those files..

ok, that search from my windows machine wasn't correct, but using Atom Editor I find 28 results across 7 files for lib/ for .bind(this. (It's possible searching for just .bind( will come up with some other results that could also be replaced, if you're feeling brave.)

Looks like there's nothing in src/node.js.

Abandoning PRs and refiling them again is kind of annoying because it loses the discussion and the review history (of which there was quite a bit, it looks like.) Can I suggest you do a rebase, fix up the issues and force-push to your existing branch?

If your implicit question is 'how?', check the documentation for git rebase -i a.k.a. interactive rebase.

Presumably your editor did something to those files? When in doubt, use vim.

Hmm, A consideration we should make: If there is a obj.send = this.emit.bind(this, 'message') you'd probably call it like: obj.send(mymessage) which would apply as: this.emit.call(this, 'message', mymessage).

If it were possible () => this.emit('message', ...arguments) would work, however, arguments is not exposed in arrow functions.

Ideally we'd have (...args) => this.emit('message', ...args) but rest parameters (The first (...args)) doesn't yet exist..

We could write it as (a,b,c,d,e) => this.emit('message', a,b,c,d,f) but that is potentially not correct, and pretty messy.

@trevnorris Does that sound about right? I think we need to be careful for now.

Rest params come in v8 4.7, so we could just have this target master once that lands, even though it'l be 6 months until it ships then.

Rest params come in v8 4.7, so we could just have this target master once that lands, even though it'l be 6 months until it ships then.

There is no rush on this change, right?

There is no rush on this change, right?

@cjihrig Correct. (That being said, it would be nice to have subset in v5 if possible.)

@Fishrock123 Currently rest parameters are even less optimized than bind(). Stay away from it. Far away.

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 PR-URL: nodejs#4547

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 Refs: nodejs#4547 PR-URL: nodejs#4623 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

use `arrow functions` instead of `bind(this)` in order to improve performance through optimizations. PR-URL: #3622 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Notable Changes: * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622. - (Mistakenly missing from v5.4.0) - module: move unnecessary work for early return (Andres Suarez) #3579 * Various doc fixes * Various test improvements PR-URL: #4626

Just putting a note here that this commit has still not landed in v5.4.x

Some backstory on that:

I screwed up my release tooling for v5.4.0 which missed this commit, and only this commit, since it was the first one in the list.
@thealphanerd's Smoke-testing for v5.4.1 seemed to indicate this commit may have an error in it, but my testing afterwords seemed to indicate it was just tape's tests being flaky.

use `arrow functions` instead of `bind(this)` in order to improve performance through optimizations. PR-URL: #3622 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

Regarding benchmarks, I'm not sure if the requests were for bind to arrow functions performance in general, or their performance when applied to node's codebase via this PR. For the former, for those interested, there is https://github.com/bevry/esnext-benchmarks which compares performance with several esnext features with the es2015 and other counterparts. Perhaps it will be useful for some readers. If it was the latter, can't help there.

use `arrow functions` instead of `bind(this)` in order to improve performance through optimizations. PR-URL: #3622 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

use `arrow functions` instead of `bind(this)` in order to improve performance through optimizations. PR-URL: nodejs#3622 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 Refs: nodejs#4547 PR-URL: nodejs#4623 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

JungMinu closed this Nov 1, 2015

JungMinu reopened this Nov 1, 2015

mscdex added tls Issues and PRs related to the tls subsystem. debugger labels Nov 1, 2015

Fishrock123 added lib / src Issues and PRs related to general changes in the lib or src directory. and removed debugger tls Issues and PRs related to the tls subsystem. labels Nov 2, 2015

MylesBorins mentioned this pull request Jan 11, 2016

V5.4.1 propose #4626

Merged

evanlucas mentioned this pull request Jan 18, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 17, 2016

MylesBorins mentioned this pull request Feb 18, 2016

V4.4.0 proposal #5301

Merged

zackify mentioned this pull request Apr 14, 2017

Update for react 15.5 text-mask/text-mask#497

Merged

lib: use arrow functions instead of bind by JungMinu · Pull Request #3622 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: use arrow functions instead of bind - #3622

lib: use arrow functions instead of bind#3622JungMinu wants to merge 1 commit intonodejs:masternodejs/node:masterfrom JungMinu:jungminu-libCopy head branch name to clipboard

Conversation

JungMinu commented Nov 1, 2015

Uh oh!

kenany commented Nov 1, 2015

Uh oh!

Fishrock123 commented Nov 1, 2015

Uh oh!

Fishrock123 commented Nov 1, 2015

Uh oh!

Fishrock123 commented Nov 1, 2015

Uh oh!

mscdex commented Nov 1, 2015

Uh oh!

Trott commented Nov 1, 2015

Uh oh!

Trott commented Nov 1, 2015

Uh oh!

jbergstroem commented Nov 1, 2015

Uh oh!

Trott commented Nov 1, 2015

Uh oh!

mscdex commented Nov 1, 2015

Uh oh!

jbergstroem commented Nov 1, 2015

Uh oh!

Fishrock123 commented Nov 1, 2015

Uh oh!

trevnorris commented Nov 2, 2015

Uh oh!

Fishrock123 commented Nov 2, 2015

Uh oh!

Fishrock123 commented Nov 2, 2015

Uh oh!

Fishrock123 commented Nov 2, 2015

Uh oh!

bnoordhuis commented Nov 3, 2015

Uh oh!

bnoordhuis commented Nov 3, 2015

Uh oh!

bnoordhuis commented Nov 3, 2015

Uh oh!

Fishrock123 commented Nov 3, 2015

Uh oh!

cjihrig commented Nov 3, 2015

Uh oh!

Fishrock123 commented Nov 3, 2015

Uh oh!

trevnorris commented Nov 3, 2015

Uh oh!

MylesBorins commented Jan 13, 2016

Uh oh!

Fishrock123 commented Jan 13, 2016

Uh oh!

balupton commented Jan 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

lib: use arrow functions instead of bind#3622
JungMinu wants to merge 1 commit into
nodejs:masterfrom
JungMinu:jungminu-lib