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.

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

replace bind() #253

Closed
wants to merge 1 commit into from
Closed

replace bind() #253

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Dec 28, 2016

This is anticipation of Writable changes landing in nodejs/node#6533

/cc @calvinmetcalf @mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks!
LGTM

@calvinmetcalf
Copy link
Contributor

ok this works fine because we put it through babel to deal with the arrow functions, but I wonder why node core couldn't use the arrow function?

@mcollina
Copy link
Member

mcollina commented Jan 4, 2017

bind() changed from being a v8 killer to be faster than arrow functions from node v7+.

@mscdex
Copy link
Contributor Author

mscdex commented Jan 4, 2017

@mcollina s/arrow functions/inline closures/

@mcollina
Copy link
Member

mcollina commented Jan 4, 2017

@mscdex yes sorry. bind() is now faster than using function to wrap this.

@calvinmetcalf
Copy link
Contributor

so readable stream is intended for use in all versions of node/multiple browsers so I'm not sure the wisdom of including optimizations for older platforms, chrome for instance will be negatively effected by this

@mscdex
Copy link
Contributor Author

mscdex commented Jan 4, 2017

@calvinmetcalf Well, it's not just older node versions but also possibly other javascript engines that might not optimize the same things. I personally do not see a straight-forward solution because readable-stream is/could be used in so many different environments. If we leave the bind() it might work better in some environments but worse in others, same goes for reverting its usage. The same also goes for #255.

I just created these PRs because @mcollina had mentioned in the upstream node PR that he considered performance changes to be breaking.

@mcollina
Copy link
Member

mcollina commented Jan 4, 2017

@calvinmetcalf using bind() in node v0.10/v0.12 and the like can cause a massive slowdown. See Level/levelup#139 as an example. If we want to play it safe, we should really benchmark this change on node v0.10-v6, and see if it impacts things. Or, we can do the change and avoid potential breakage (yes, I consider a major slowdown a breakage).

@mcollina
Copy link
Member

We are going to merge this on the next update, as decided in #252

@mcollina mcollina mentioned this pull request Mar 11, 2017
@mcollina
Copy link
Member

Part of #262

@mcollina mcollina closed this Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants