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

events: optimize adding and removing of listeners more #785

Closed
wants to merge 2 commits into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 10, 2015

These optimizations result in >2x speedup in the ee-add-remove
benchmark:

  • Don't mutate array.length when removing the last listener for
    an event
  • Don't bother checking max listeners if listeners isn't an array
  • Don't call delete when removing the last event in _events, just
    re-assign a new object instead

This keeps in line with how things are done for the fast path
and *might* even provide a *slight* performance increase.
@evanlucas
Copy link
Contributor

events/ee-add-remove.js n=250000: ./iojs: 1273100 /usr/local/bin/iojs: 311090 .......... 309.25%
events/ee-emit-multi-args.js n=2000000: ./iojs: 5653500 /usr/local/bin/iojs: 1773000 ... 218.86%
events/ee-emit.js n=2000000: ./iojs: 7866000 /usr/local/bin/iojs: 2100400 .............. 274.49%
events/ee-listener-count.js n=50000000: ./iojs: 405780000 /usr/local/bin/iojs: 403700000 . 0.51%
events/ee-listeners-many.js n=5000000: ./iojs: 6041800 /usr/local/bin/iojs: 6077800 ..... -0.59%
events/ee-listeners.js n=5000000: ./iojs: 22134000 /usr/local/bin/iojs: 7548900 ........ 193.21%

It seems to offer a significant improvement to more than just ee-add-remove. The ee-listener-count and ee-listeners-many seem to vary a little bit though.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 10, 2015

Huh... I thought I checked the other benchmarks also...

@mscdex
Copy link
Contributor Author

mscdex commented Feb 10, 2015

@evanlucas You must be testing against a version of iojs that is pre-b677b844fc1. I tested relative to the current head of v1.x and ee-add-remove is the only one that really shows an improvement.

@evanlucas
Copy link
Contributor

Ah, I had thought for some reason I was up to date on v1.x. Perhaps not. :]

@Fishrock123
Copy link
Contributor

events/ee-add-remove.js n=250000: ./out/Release/iojs: 1127100 iojs: 465520 ........... 142.11%
events/ee-emit-multi-args.js n=2000000: ./out/Release/iojs: 4900900 iojs: 3973500 ..... 23.34%
events/ee-emit.js n=2000000: ./out/Release/iojs: 6609200 iojs: 5808800 ................ 13.78%
events/ee-listener-count.js n=50000000: ./out/Release/iojs: 247180000 iojs: 253620000 . -2.54%
events/ee-listeners-many.js n=5000000: ./out/Release/iojs: 4544800 iojs: 6103200 ..... -25.53%
events/ee-listeners.js n=5000000: ./out/Release/iojs: 20549000 iojs: 19110000 .......... 7.53%

ee-listerners-many continues to give wildly inconsistent results.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 10, 2015

@Fishrock123 Yeah listeners() and listenerCount() really shouldn't be affected by these changes. I never saw the high negative number you're seeing, even after many runs, but I'm just chalking it up to "normal fluctuations."

m = $getMaxListeners(this);
if (m && m > 0 && existing.length > m) {
existing.warned = true;
console.error('(node) warning: possible EventEmitter memory ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be io.js instead of node now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shrug I didn't change anything in that whole block, just wrapped it all within an else {}.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 11, 2015

Code updated with suggested changes.

else
return false;
} else
doError = (type === 'error' && !events.error);
Copy link
Member

Choose a reason for hiding this comment

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

This can be made a little flatter and more readable by swapping the consequent and the alternate. Suggestion:

if (events)
  doError = (type === 'error' && events.error != null);
else if (type === 'error')
  doError = true;
else
  return false;

You can condense it but I'll leave it up to you to decide what's more readable.

doError = (type === 'error');
if (events)
  doError = (doError && events.error == null);
else if (!doError)
  return false;

These optimizations result in >2x speedup in the ee-add-remove
benchmark:

* Don't mutate array.length when removing the last listener for
an event
* Don't bother checking max listeners if listeners isn't an array
* Don't call delete when removing the last event in _events, just
re-assign a new object instead
@mscdex
Copy link
Contributor Author

mscdex commented Feb 11, 2015

Updated again with suggested changes.

@bnoordhuis
Copy link
Member

bnoordhuis pushed a commit that referenced this pull request Feb 11, 2015
This keeps in line with how things are done for the fast path
and *might* even provide a *slight* performance increase.

PR-URL: #785
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
bnoordhuis pushed a commit that referenced this pull request Feb 11, 2015
These optimizations result in >2x speedup in the ee-add-remove
benchmark:

* Don't mutate array.length when removing the last listener for
an event
* Don't bother checking max listeners if listeners isn't an array
* Don't call delete when removing the last event in _events, just
re-assign a new object instead

PR-URL: #785
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@bnoordhuis
Copy link
Member

Thanks Brian, landed in 630f636...7061669.

@ChALkeR
Copy link
Member

ChALkeR commented May 25, 2015

This code in lib/_stream_readable.js (and the readable-stream module) does not take _eventsCount into account (does not increase it):

  // This is a brutally ugly hack to make sure that our error handler
  // is attached before any userland ones.  NEVER DO THIS.
  if (!dest._events || !dest._events.error)
    dest.on('error', onerror);
  else if (Array.isArray(dest._events.error))
    dest._events.error.unshift(onerror);
  else
    dest._events.error = [onerror, dest._events.error];

Edit: Sorry, I was wrong, it's fine =). dest.on increases the count, other two cases shouldn't. ☺

@mscdex mscdex deleted the perf-events-part2 branch May 25, 2015 14:00
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.

5 participants