-
Notifications
You must be signed in to change notification settings - Fork 30k
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
lib: micro-optimize EventEmitter#removeListener() #185
Conversation
👍 |
That's super sweet! Perhaps a pedantic point, but shouldn't we call it something else but |
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>
LGTM. I looked into splitting |
1ee720b
to
d3f8db1
Compare
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. |
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