Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: use arrow functions instead of bind #3622

Closed
wants to merge 1 commit into from
Closed

lib: use arrow functions instead of bind #3622

wants to merge 1 commit into from

Conversation

JungMinu
Copy link
Member

@JungMinu JungMinu commented Nov 1, 2015

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

@kenany
Copy link
Contributor

kenany commented Nov 1, 2015

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

@JungMinu JungMinu closed this Nov 1, 2015
@Fishrock123
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

@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 JungMinu reopened this Nov 1, 2015
@Fishrock123
Copy link
Contributor

@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.

@mscdex
Copy link
Contributor

mscdex commented Nov 1, 2015

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.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. debugger labels Nov 1, 2015
@Trott
Copy link
Member

Trott commented Nov 1, 2015

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.

@Trott
Copy link
Member

Trott commented Nov 1, 2015

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

@jbergstroem
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Nov 1, 2015

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.

@mscdex
Copy link
Contributor

mscdex commented Nov 1, 2015

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

@jbergstroem
Copy link
Member

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.

@Fishrock123
Copy link
Contributor

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 [email protected] 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.

@trevnorris
Copy link
Contributor

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.

@Fishrock123
Copy link
Contributor

@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.

@Fishrock123
Copy link
Contributor

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

@Fishrock123 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
@Fishrock123
Copy link
Contributor

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.

@bnoordhuis
Copy link
Member

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?

@bnoordhuis
Copy link
Member

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

@bnoordhuis
Copy link
Member

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

@Fishrock123
Copy link
Contributor

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.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 3, 2015

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?

@Fishrock123
Copy link
Contributor

There is no rush on this change, right?

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

@trevnorris
Copy link
Contributor

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

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
af591d4 
* 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 (José 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
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 11, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
5f0abd6 
* 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 (José 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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
@JungMinu
lib: use arrow functions instead of bind
94e2875 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: #3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 11, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
2016-01-11, Version 5.4.1 (Stable)
002b9f8 
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
@MylesBorins
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor

Some backstory on that:

  1. 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.
  2. @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.

rvagg pushed a commit that referenced this pull request Jan 14, 2016
@JungMinu @rvagg
lib: use arrow functions instead of bind
4e04063 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: #3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
Merged
evanlucas added a commit that referenced this pull request Jan 20, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
dd88256 
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
evanlucas added a commit that referenced this pull request Jan 21, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
318ded2 
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
@balupton
Copy link

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.

MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@JungMinu
lib: use arrow functions instead of bind
3c46ddd 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: #3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@JungMinu
lib: use arrow functions instead of bind
59255d7 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: #3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@JungMinu
lib: use arrow functions instead of bind
cf3a403 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: #3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@JungMinu
lib: use arrow functions instead of bind
1b48738 
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: nodejs#3622
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
3991aa8 
* 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 (José 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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas
2016-01-20, Version 5.5.0 (Stable)
de2ebef 
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
@zackify zackify mentioned this pull request Apr 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Fishrock123 Fishrock123

Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.