Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
lib: micro-optimize EventEmitter#removeListener()#185
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sonewman commented Dec 20, 2014
👍 |
algesten commented Dec 20, 2014
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 <[email protected]>
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 <[email protected]>
chrisdickinson commented Dec 20, 2014
LGTM. I looked into splitting |
1ee720b to d3f8db1Comparebnoordhuis commented Dec 20, 2014
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