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

EventEmitter: use a Map to store event listeners #1785

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented May 24, 2015

This is a fix for #728.

I ran the events benchmarks to compare this with master and there seems to be a significant performance drop for some cases (though I'm not sure of how I should compare the runs).

Bench master Map
ee-add-remove 1216157 950515 (-22%)
ee-emit-multi-args 5909434 5640342 (-5%)
ee-emit 8485253 7912000 (-7%)
ee-listener-count 432010872 56945905 (-87%)
ee-listeners-many 3558539 3490269 (-2%)
ee-listeners 21596479 18429471 (-15%)

@petkaantonov
Copy link
Contributor

In the results you posted Map is always performing worse?

else
dest._events.error = [onerror, dest._events.error];
dest._events.set('error', [onerror, dest._events.get('error')]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe all of these changes to readable stream are going to result in the same problem as when I proposed optimizations for eventEmitter.once() (#914), which is that some modules on npm have their readable-stream pinned to specific versions or limited version ranges. So if someone uses an older readable-stream on an io.js with these changes, things will break.

Copy link
Member

Choose a reason for hiding this comment

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

@mscdex That would be a lot of someones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Last I checked npm (a few weeks ago) it was only about a dozen or so? I had actually thought about submitting PRs to those projects just so we could get the .once() optimizations.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, there are other modules that are using _events directly. For example:

That's in my current node_modules dir, except for the readable-stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChALkeR I was referring more generally about changes to readable-stream and how modules are depending on it version-wise, not specific code changes. But yes, there are probably a ton of modules that use _events directly.

Copy link
Member

Choose a reason for hiding this comment

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

Last I checked npm (a few weeks ago) it was only about a dozen or so?

Just in my current node_modules dir I have 19 of those. List:

bufferstreams       "readable-stream": "^1.0.33"
tar-stream      "readable-stream": "~1.0.33",
dicer           "readable-stream": "1.1.x",
duplexer2       "readable-stream": "~1.1.9"
through2        "readable-stream": ">=1.0.33-1 <1.1.0-0",
are-we-there-yet    "readable-stream": "^1.1.13"
decompress-zip      "readable-stream": "^1.1.8",
tar-pack        "readable-stream": "~1.0.2",
bl          "readable-stream": "~1.0.26"
readdirp        "readable-stream": "~1.0.26-2"
busboy          "readable-stream": "1.1.x"
raw-body        "readable-stream": "~1.0.33",
gulp-bower/through2 "readable-stream": ">=1.0.28 <1.1.0-0",
htmlparser2     "readable-stream": "1.1"
concat-stream       "readable-stream": "~1.1.9",
mongodb         "readable-stream": "1.0.31"
read-all-stream     "readable-stream": "~1.1.13"
finalhandler        "readable-stream": "~1.0.33",
duplexify       "readable-stream": "^1.1.13"

Copy link
Member

Choose a reason for hiding this comment

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

If this gets merged, will 1.0.* and 1.1.* branches of readable-stream receive patches?
If yes, it is true that there are not so many packages that specify a fixed version of readable-stream as a dependency.

@ChALkeR
Copy link
Member

ChALkeR commented May 24, 2015

@petkaantonov Map should be faster than an Object here. I guess that's the time measurements in that table. But it would be better if @targos shared his tests.

@petkaantonov
Copy link
Contributor

The benchmark gives results in op/s (higher is better).

if (events.removeListener)
this.emit('removeListener', type, listener);
}
events.delete(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about performance differences with this particular change. Is it faster to keep _eventsCount around and create a new Map()?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no significant performance difference if I revert ca75072. I tested with this._events.clear() and this._events = new Map()

@ChALkeR
Copy link
Member

ChALkeR commented May 24, 2015

Ah, that's the bundled benchmark from io.js/benchmark/events, I missed that. Something is not right here then.

@@ -2914,9 +2914,6 @@ void SetupProcessObject(Environment* env,
env->SetMethod(process, "_setupNextTick", SetupNextTick);
env->SetMethod(process, "_setupPromises", SetupPromises);
env->SetMethod(process, "_setupDomainUse", SetupDomainUse);

// pre-set _events object for faster emit checks
process->Set(env->events_string(), Object::New(env->isolate()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Isn't there a way to create a Map from C++?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. I couldn't find anything related to maps in the v8 API and I know that the Map constructor is written in JS but there may be a way to call it from C++

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the NativeWeakMap but I can't find the regular Map either.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be possible with v8 4.5: v8/v8@395fa8b

@targos
Copy link
Member Author

targos commented May 24, 2015

@petkaantonov is right, I assumed it was the time...
So yes, Map is always performing worse :(

@yosuke-furukawa
Copy link
Member

ES6 Map is slower than Object props in V8.
Ref: https://code.google.com/p/v8/issues/detail?id=4014

And EventEmitter is our core module, if event emitter is slower, whole system may be slow on performance.

@mscdex mscdex added the events Issues and PRs related to the events subsystem / EventEmitter. label May 24, 2015
@ChALkeR
Copy link
Member

ChALkeR commented May 24, 2015

@yosuke-furukawa This seems to depend on the usecase. I saw usecases where replacing objects with Maps resulted in a 2x speedup.

@yosuke-furukawa
Copy link
Member

@ChALkeR Of course, the benchmark result is depend on the usecase. But current result seems to be worse. @petkaantonov is correct. our benchmark test shows ops per second https://github.com/nodejs/io.js/blob/master/benchmark/common.js#L225 .

@ChALkeR
Copy link
Member

ChALkeR commented May 24, 2015

@yosuke-furukawa, Yes, I know about the bundled benchmarks.
In the original OP post there was a claim that these numbers show a performance speedup, that's why I thought that this is not a bundled benchmark but something of his own that measures the time taken.

@Fishrock123
Copy link
Contributor

Is prefixing events something that we can do without breaking anything? I think that would be a much better alternative. Map simply isn't fast enough yet.

@targos
Copy link
Member Author

targos commented May 24, 2015

@yosuke-furukawa @Fishrock123 I agree that the current perf of Map is blocking this.
We can keep the PR open and re-run the benchmarks when there is a v8 upgrade to see if it's improving

@chrisdickinson
Copy link
Contributor

It's going to be an uphill battle to directly change the format of _events, as @mscdex noted, at minimum making sure that both minor versions of readable-stream get a backwards-compatible patch release, and propagating that through the package ecosystem.

If the perf improves, I'd advise presenting a legacy _events interface via a getter and putting the map-backed interface behind a symbol or weakmap.

On May 24, 2015, at 11:12 AM, Jeremiah Senkpiel notifications@github.com wrote:

Is prefixing events something that we can do without breaking anything? I think that would be a much better alternative. Map simply isn't fast enough yet.


Reply to this email directly or view it on GitHub.

@ChALkeR
Copy link
Member

ChALkeR commented May 25, 2015

Btw, about the _events usage — it looks like a very bad thing to me that modules use that.
It probably means that some of the API methods that should be there are missing.
Maybe the EventEmmiter api should be extended to cover those use-cases?

Samples:

  • readable-stream/lib/_stream_readable.js:
    1. if (!dest._events || !dest._events.error)
    2. else if (isArray(dest._events.error))
    3. dest._events.error.unshift(onerror);
    4. dest._events.error = [onerror, dest._events.error];
  • dicer/lib/Dicer.js
    1. if (this._events.preamble)
    2. if ((start + i) < end && this._events.trailer)
    3. if (this._events[ev])
  • busboy/lib/types/multipart.js
    1. if (!boy._events.file) {
  • ultron/index.js:
    1. for (event in this.ee._events) { if (this.ee._events.hasOwnProperty(event)) {

It looks to me that there should be methods in EventEmitter to:

  1. Get a list of all events from an EventEmmiter that currently have listeners.
  2. (optional) Check if there are any listeners for a specific event. Like EventEmitter.listenerCount but using a prototype, like EventEmitter.prototype.listeners but returning just the count.
  3. (optional) To prepend an event. Does the documentation specify the order in which the events are executed, btw?

Also it looks like readable-stream (ah, and iojs/lib/_stream_readable.js) breaks _eventsCount. I was wrong, it's fine.

@mscdex, thoughts?

@mscdex
Copy link
Contributor

mscdex commented May 25, 2015

@ChALkeR Using EventEmitter.listenerCount() is easy and efficient enough and was basically all I was using direct usage for.

@ChALkeR
Copy link
Member

ChALkeR commented May 25, 2015

@mscdex That's why I marked that as (optional).
But ultron module iterates over events, and there seems to be no method for that in the public API.

@mscdex
Copy link
Contributor

mscdex commented May 26, 2015

@ChALkeR Well, for that you could iterate over .listeners()?

@chrisdickinson
Copy link
Contributor

  • readable-stream is directly vendored from core's streams. Core has a need for prepending events – userland does not (with the exception of readable-stream, which doesn't seem like a good excuse to add it.)
  • dicer seems to be checking for specific events, which would be better done with EE.listenerCount or even ee.listeners('event').
  • cloning an event emitter wholesale (like ultron is doing) is a pretty unexpected / surprising use of the EE API, I don't think we should support that.

@targos targos force-pushed the fix-728 branch 2 times, most recently from 0921fb9 to 66518a0 Compare May 27, 2015 08:39
@sam-github
Copy link
Contributor

But ultron module iterates over events, and there seems to be no method for that in the public API.

Not to defend ultron - whatever it is - but introspecting an EE to see all the events that have been listened on would be very useful for debugging. For example, to check if a misspelled event name is being used. I'd like to see node be more introspectable, so if getting this info causes zero additional overhead, I think it'd be nice to have.

@Qard
Copy link
Member

Qard commented May 27, 2015

👍 to allowing more introspection of event emitters.

@jasnell
Copy link
Member

jasnell commented Aug 27, 2015

@ChALkeR @targos ... No rush, just curious... where are things at on this one?

@@ -0,0 +1,3 @@
'use strict';

exports.EE_events = Symbol('events');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use camelCase.

@targos
Copy link
Member Author

targos commented Aug 27, 2015

@jasnell I rebased on master. here are the new benchmark numbers:

Bench master Map
ee-add-remove 1100610 693568 (-37%)
ee-emit-multi-args 5566010 5303709 (-5%)
ee-emit 8074416 7592220 (-6%)
ee-listener-count 409444871 44700710 (-90%)
ee-listeners-many 3426115 3345520 (-2%)
ee-listeners 21558724 15990929 (-26%)

@targos
Copy link
Member Author

targos commented Aug 27, 2015

I guess I can close this PR for now. I don't think it will improve in a near future.

@thefourtheye
Copy link
Contributor

@targos This also fixes another bug in the implementation. As of now, if we use inherited members as event names, it will confuse the events module a little. So I kinda hoped that this would land :(

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

@targos ... Given the current status on this and your last comment, I'm inclined to close. Per @thefourtheye's comment tho, there may still be some stuff worth saving in this so perhaps another look is warranted? Particularly with the newer V8 in master?

@targos
Copy link
Member Author

targos commented Oct 22, 2015

OK, closing for now.

@thefourtheye I think the problem you are talking about can be fixed by using Object.create(null) instead of an empty literal to create the map.

@targos targos closed this Oct 22, 2015
@thefourtheye
Copy link
Contributor

@targos I tried that already in #2350 but looks like Object.create(null) is not preferred because of the performance hit.

@targos
Copy link
Member Author

targos commented Oct 22, 2015

the performance tests were run in 2012. I would try with current V8 version to be sure it is still a problem.

@thefourtheye
Copy link
Contributor

@targos I wrote this simple script to compare the performance of them

'use strict';

const times = 10e7;

console.time('Object.create');
for (var i = 0; i < times; i += 1) {
  let obj = Object.create(null);
}
console.timeEnd('Object.create');


console.time('Object Literal');
for (var i = 0; i < times; i += 1) {
  let obj = {};
}
console.timeEnd('Object Literal');

Object.create is 10 times faster, but the speed gain is in nano-seconds.

@targos targos deleted the fix-728 branch June 1, 2017 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.