lib: replace self/bind usage with arrow func #3816

tflanagan · 2015-11-13T20:12:06Z

This commit replaces trivial occurances of var self = this and .bind()
where appropriate.

FWIW, passes local tests.

May be adding to this over the next day - this is the result of a couple simple greps.

This is in parallel to #3622.

cjihrig · 2015-11-13T20:18:02Z

LGTM if the CI is happy

mscdex · 2015-11-13T21:09:34Z

I'm curious as to any performance impact this may have. I know that bind() is slow, but how does it compare to arrow functions or even using var self = this; and an anonymous function?

Does anyone have actual numbers to show?

Fishrock123 · 2015-11-13T21:15:51Z

@mscdex Afaik Arrow Functions have identical overhead to regular functions.

Fishrock123 · 2015-11-13T21:16:55Z

And regarding bind, to my knowledge it is specifically making the bound function that is slow, not running it.

mscdex · 2015-11-13T21:21:45Z

@Fishrock123 A quick google search turns up this jsperf. So at first glance it seems like it's not just the creation of bound functions that is slow? At any rate, if that jsperf is trustworthy, then arrow functions should be as fast as self = this and both are much faster than functions bound with bind().

lucthev · 2015-11-13T21:31:10Z

lib/cluster.js

-    self.server = null;
+  this.server.once('listening', () => {
+    this.handle = this.server._handle;
+    this.handle.onconnection = this.distribute.call(this);


Should this still be a bind (or another arrow function)?

Yes, this should not be a .call(), otherwise the function will be immediately invoked.

Whoops, forgot to wrap that in a () => {}. One sec...

Actually...

What's better?

this.handle.onconnection = () => this.distribute.call(this);

or

this.handle.onconnection = this.distribute.bind(this);

One is one and done and the other is an anon function that invokes .call each time

Since you have access to this, can't you just do the following?

this.handle.onconnection = () => this.distribute();

Although it looks like .distribute expects some arguments, so .bind might be more appropriate here (I'm not familiar with cluster internals, so that's really just speculative).

Long day. @lucthev, you're 100% right. I'll revisit after some shuteye.

tflanagan · 2015-11-16T15:30:36Z

Okay, looks like I've got all of the non-stacktrace changing (named functions like onDone), trivial self/bind usages that were not addressed in parallel PR.

FWIW, passing all local tests

tflanagan · 2015-11-16T15:36:04Z

lib/dgram.js

-          self.send.apply(self, self._sendQueue[i]);
-        self._sendQueue = undefined;
+        for (var i = 0; i < this._sendQueue.length; i++)
+          this.send.apply(this, this._sendQueue[i]);


Not sure if this is worthy of a comment or not.

Very interestingly, rewriting this to this.send(this._sendQueue[i]); blew up the dgram tests.

@tflanagan Note that it is apply and not call, that is, it applies like so:

this.send.call(this, this._sendQueue[i][0], this._sendQueue[i][1], etc...);

@Fishrock123 right, dumb moment, don't mind me. ( thank you ;) )

I think we can do spread call now: this.send(...this._sendQueue[i])

@targos I remember reading that the spread operator isn't too friendly to v8 optimizations and to performance in general.

tflanagan · 2015-11-16T21:14:52Z

@jasnell I've reverted that constructor and also rebase'd with the newest lib/_debugger.js changes.

tflanagan · 2016-01-22T19:13:07Z

I've just updated this PR to match master.

/poke

silverwind · 2016-01-28T18:00:02Z

lib/_debugger.js


  // Don't display any default messages
  var listeners = this.repl.rli.listeners('SIGINT').slice(0);
  this.repl.rli.removeAllListeners('SIGINT');

-  function exitDebugRepl() {
+  var exitDebugRepl = () => {


silverwind · 2016-01-28T19:36:34Z

Please don't force-push after a review the next time, it makes it very hard for me to track the new changes.

tflanagan · 2016-01-28T19:38:54Z

I'm sorry - I'm running into errors with local tests, will commit fixes (wont force), in a moment

This commit replaces trivial occurances of var self = this and .bind() where appropriate.

tflanagan · 2016-01-28T23:29:24Z

I'm done toying now :). Passes local tests fwiw (minus some flaky ones that happen on master too)

jasnell · 2016-01-29T06:53:10Z

LGTM

silverwind · 2016-01-29T11:27:28Z

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

tflanagan · 2016-01-29T15:03:05Z

Any reason those CI's aren't public? Would love to see if there some errors I can fix

jasnell · 2016-01-29T15:06:19Z

Yeah, we're currently testing the security fixes for next weeks release using the CI. Because the details of the fix are embargoed until the release we need to restrict access to the CI. Once we have the fixes fully tested the full CI will be made public again. Standard procedure when we have security updates pending! I think @rvagg just forgot to mention that in the security release notification posted the other day.

jasnell · 2016-01-29T15:08:05Z

Just looking at the CI now and it's almost all red. There are a ton of failures on this one unfortunately :-( .. I'm a bit tied up currently but here shortly I'll post some of the failures here for review

tflanagan · 2016-01-29T15:11:51Z

Oh boy, sounds like some fun heading my way. ;) Thanks!

-- I'll build it on my own boxes until then

silverwind · 2016-01-29T16:43:40Z

Not sure how far @jasnell got, but here are some OS X failures:

not ok 514 test-listen-fd-cluster.js
# TIMEOUT
# Cluster listen fd test runner
# about to listen in parent
# server listening on 12346
# master spawned
# Cluster listen fd test master
# in master, spawning worker
# Cluster listen fd test worker
# worker, about to create server and listen on fd=3
# worker listening on fd=3

not ok 862 test-tls-ticket-cluster.js
# TIMEOUT
# [master] got "listening"
# [master] connecting
# [master] got "listening"
# [master] got "listening"
# [master] got "listening"

not ok 87 test-cluster-disconnect.js
# TIMEOUT

not ok 100 test-cluster-http-pipe.js
# TIMEOUT

not ok 103 test-cluster-message.js
# TIMEOUT

not ok 109 test-cluster-send-handle-twice.js
# TIMEOUT

tflanagan · 2016-01-29T16:50:32Z

Thanks, @silverwind, I see those on my debian box too, I'm working on it.

jasnell · 2016-02-01T16:02:45Z

@silverwind .. thank you, @tflanagan sorry I didn't get the details like I said I would, ended up completely sidetracked for the rest of that day :-/

tflanagan · 2016-02-17T23:49:25Z

Just an update, I've been really busy these last couple months, I plan in fixing this soon. Its just a little low on priorities.

The main deterrent is the lack of view rights on ci's. I don't see all the errors locally...

jasnell · 2016-02-19T20:39:25Z

@tflanagan ... no worries, completely understandable. The CI visibility is getting to be a major pain. Hopefully you'll be onboarded soon so you can access it.

silverwind · 2016-02-19T21:35:47Z

All failures on the Ubuntu 14.04 x64 box:

not ok 87 test-cluster-disconnect.js
# TIMEOUT
  ---
  duration_ms: 60.96
  ...
not ok 100 test-cluster-http-pipe.js
# TIMEOUT
  ---
  duration_ms: 60.78
  ...
not ok 103 test-cluster-message.js
# TIMEOUT
  ---
  duration_ms: 60.85
  ...
not ok 109 test-cluster-send-handle-twice.js
# TIMEOUT
  ---
  duration_ms: 60.101
  ...
not ok 131 test-cluster-worker-wait-server-close.js
# TIMEOUT
  ---
  duration_ms: 60.97
  ...
not ok 514 test-listen-fd-cluster.js
# TIMEOUT
# Cluster listen fd test runner
# about to listen in parent
# server listening on 12346
# master spawned
# Cluster listen fd test master
# in master, spawning worker
# Cluster listen fd test worker
# worker, about to create server and listen on fd=3
# worker listening on fd=3
# master exit on disconnect
  ---
  duration_ms: 60.81
  ...
not ok 862 test-tls-ticket-cluster.js
# TIMEOUT
# [master] got "listening"
# [master] connecting
# [master] got "listening"
# [master] got "listening"
# [master] got "listening"
# [worker] exit
# [worker] exit
# [worker] exit
# [worker] exit
  ---
  duration_ms: 60.83
  ...

jasnell · 2016-02-19T23:41:53Z

Given the changes here I'm going to mark this as a don't land on v4 for the time being. We may want to revisit this later /cc @thealphanerd

jasnell · 2016-04-02T03:53:04Z

What's the status on this one?

MylesBorins · 2016-05-09T21:20:41Z

I'm going to close this for now as there has been no response in a month. Please feel free to reopen if you would like to pick this back up

tflanagan mentioned this pull request Nov 13, 2015

lib: use arrow functions instead of bind #3622

Closed

jasnell added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 13, 2015

lucthev reviewed Nov 13, 2015
View reviewed changes

tflanagan force-pushed the trivial-arr-func branch 4 times, most recently from 65e152c to 3c8630e Compare November 16, 2015 15:22

tflanagan reviewed Nov 16, 2015
View reviewed changes

tflanagan force-pushed the trivial-arr-func branch 4 times, most recently from afa939a to 9df8a7f Compare November 16, 2015 21:14

svenheden mentioned this pull request Nov 18, 2015

Replace .bind() with arrow functions keystonejs/keystone-classic#1832

Merged

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

tflanagan force-pushed the trivial-arr-func branch from 9df8a7f to 6f20a17 Compare January 22, 2016 19:09

tflanagan mentioned this pull request Jan 25, 2016

fs: refactor self=this to arrow #4831

Closed

silverwind reviewed Jan 28, 2016
View reviewed changes

tflanagan added 2 commits January 28, 2016 15:22

lib: replace self/bind usage with arrow func

efa4814

This commit replaces trivial occurances of var self = this and .bind() where appropriate.

lib: fix position of new const funcs, jslint

139e960

tflanagan force-pushed the trivial-arr-func branch 2 times, most recently from d5608cf to c780fcd Compare January 28, 2016 23:26

lib: fix repl.js

a1b9103

tflanagan force-pushed the trivial-arr-func branch from c780fcd to a1b9103 Compare January 28, 2016 23:28

jasnell added the dont-land-on-v4.x label Feb 19, 2016

jasnell added the stalled Issues and PRs that are stalled. label Apr 2, 2016

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

MylesBorins closed this May 9, 2016

Clemens-git76 mentioned this pull request Apr 16, 2024

[Snyk] Upgrade d3 from 5.16.0 to 7.9.0 Clemens-git76/node#5

Open

lib: replace self/bind usage with arrow func #3816

lib: replace self/bind usage with arrow func #3816

Conversation

tflanagan commented Nov 13, 2015

cjihrig commented Nov 13, 2015

mscdex commented Nov 13, 2015

Fishrock123 commented Nov 13, 2015

Fishrock123 commented Nov 13, 2015

mscdex commented Nov 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tflanagan commented Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tflanagan commented Nov 16, 2015

tflanagan commented Jan 22, 2016

Choose a reason for hiding this comment

silverwind commented Jan 28, 2016

tflanagan commented Jan 28, 2016

tflanagan commented Jan 28, 2016

jasnell commented Jan 29, 2016

silverwind commented Jan 29, 2016

tflanagan commented Jan 29, 2016

jasnell commented Jan 29, 2016

jasnell commented Jan 29, 2016

tflanagan commented Jan 29, 2016

silverwind commented Jan 29, 2016

tflanagan commented Jan 29, 2016

jasnell commented Feb 1, 2016

tflanagan commented Feb 17, 2016

jasnell commented Feb 19, 2016

silverwind commented Feb 19, 2016

jasnell commented Feb 19, 2016

jasnell commented Apr 2, 2016

MylesBorins commented May 9, 2016