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

lib: micro-optimize EventEmitter#removeListener() #185

Merged
merged 2 commits into from
Dec 20, 2014

Conversation

bnoordhuis
Copy link
Member

Replace the call to Array#splice() with a faster open-coded version
that creates less garbage.

Add a new benchmark to prove it. With the change applied, it scores
a whopping 40% higher.

R=@chrisdickinson

@sonewman
Copy link
Contributor

👍

@algesten
Copy link

That's super sweet!

Perhaps a pedantic point, but shouldn't we call it something else but splice though? I mean since it does one very specific thing. shiftOne?

Don't use Number#toPrecision(), it switches to scientific notation for
numbers with more digits than the precision; use Number#toFixed().

PR-URL: nodejs#185
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Replace the call to Array#splice() with a faster open-coded version
that creates less garbage.

Add a new benchmark to prove it.  With the change applied, it scores
a whopping 40% higher.

PR-URL: nodejs#185
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@chrisdickinson
Copy link
Contributor

LGTM. I looked into splitting splice into spliceFront and spliceBack (copying backwards or forwards from the splice position, respectively) but since the usual number of listeners is <= 10, it didn't make too much of a difference. I also tried specializing position == 0 and position == list.length, but again, no difference!

@bnoordhuis bnoordhuis force-pushed the micro-optimize-ee-remove-listener branch 2 times, most recently from 1ee720b to d3f8db1 Compare December 20, 2014 12:38
@bnoordhuis bnoordhuis closed this Dec 20, 2014
@bnoordhuis bnoordhuis deleted the micro-optimize-ee-remove-listener branch December 20, 2014 12:38
@bnoordhuis bnoordhuis merged commit d3f8db1 into nodejs:v0.12 Dec 20, 2014
@bnoordhuis
Copy link
Member Author

Cheers, fixed up the indentation and (partially) incorporated @algesten's feedback by renaming the function to spliceOne(). Landed in d3f8db1.

@chrisdickinson Using Array#shift() to remove the first element is a bit faster, but only for larger lists. The lower bound is probably at least 16 elements but there doesn't seem to be an appreciable difference until the array length is in the low hundreds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants