events: improve eventEmitter.once() performance

[mscdex]

This commit improves once() performance by storing the event handler directly instead of creating a wrapper function every time.

These changes bring ~150% increase in performance when simply adding once() event handlers, ~220% increase in the included ee-emit-once benchmark, and a ~50% increase in the included ee-add-remove-once benchmark.

This PR is #914 rebased on master.

The backwards compatibility issue (with directly modifying the private _events property) with readable-stream still stands however. We would need to get package owners who have a pegged dependency version for readable-stream to either bump the version (to the readable-stream version containing the changes from this PR) or use a less strict specifier.

[jasnell]

LGTM

[mscdex]

Ok, assuming people would be ok with the change itself, how about updating readable-stream first, give users some time to upgrade (and notify users who have pinned to specific readable-stream versions), and then land this in master? I think that should minimize any kind of compatibility issues?

/cc @nodejs/collaborators

[calvinmetcalf]

So the way readable streams works is we pull from the published version of node after it publishes, so we can do one of couple things

• land changes to streams in the next release and then changes to events in the next
• publish a version of readable-stream where we hand modify that section as a one off
• add a regex to make the change in case the event changes take more then one release.

[mcollina]

Just a quick question: does it introduce some slowdown on normal on() and emit() calls? I see a lot of ifs being added in an hot-path (https://github.com/nodejs/node/pull/5127/files#diff-71dcd327d0ca067b490b22d677f81966R371), which then can complicate V8's job in producing efficient bytecode.

Does it impact streams (net, http, fs) as they are? Are those faster (or slower) because of this?

[chrisdickinson]

I'm still kind of -0 on this - it stands to break anyone that has pegged a version of readable-stream even if we release a patch version of that package. Maybe putting it through CITGM first would be good?

On Feb 8, 2016, at 9:56 AM, Matteo Collina [email protected] wrote:

Just a quick question: does it introduce some slowdown on normal on() and emit() calls? I see a lot of ifs being added in an hot-path (https://github.com/nodejs/node/pull/5127/files#diff-71dcd327d0ca067b490b22d677f81966R371), which then can complicate V8's job in producing efficient bytecode.

Does it impact streams (net, http, fs) as they are? Are those faster (or slower) because of this?

—
Reply to this email directly or view it on GitHub.

[mscdex]

@chrisdickinson IMHO even if this PR doesn't get landed, I think that we should remove streams' reliance on EventEmitter's implementation details. Doing so would allow optimizations like the one in this PR to not have the problem we currently face.

[calvinmetcalf]

so maybe we land the streams update first and ensure it does not impact performance and then as a step 2/separate thing look into updating events

[mscdex]

There are a couple of performance regressions, one of which I have already solved (and actually turned the regression into a significant improvement in the process). I am working on the other.

[mscdex]

Ok I've worked on this some more the past couple of days and I was able to minimize the impact as much as possible (at least as much as I can tell). Here are the results I'm currently getting with master:

events/ee-add-once.js n=50000000: ./node: 1567000 ./node-old: 1034400 ........................ 51.49%
events/ee-add-remove-once.js n=25000000: ./node: 974470 ./node-old: 713950 ................... 36.49%
events/ee-add-remove.js n=5000000: ./node: 1051600 ./node-old: 1015900 ........................ 3.52%
events/ee-add.js n=10000000: ./node: 1641000 ./node-old: 1647400 ............................. -0.39%
events/ee-emit-multi-args.js n=25000000: ./node: 4593300 ./node-old: 4581800 .................. 0.25%
events/ee-emit-once.js n=25000000: ./node: 6055100 ./node-old: 1981500 ...................... 205.59%
events/ee-emit.js n=50000000: ./node: 6851000 ./node-old: 6642000 ............................. 3.15%
events/ee-listener-count-on-prototype.js n=50000000: ./node: 43865000 ./node-old: 43162000 .... 1.63%
events/ee-listeners-many.js n=15000000: ./node: 3154600 ./node-old: 3153100 ................... 0.05%
events/ee-listeners.js n=90000000: ./node: 15699000 ./node-old: 16339000 ..................... -3.92%
events/ee-remove-all-once.js n=50000000 withType=true: ./node: 6972700 ./node-old: 3310900 .. 110.60%
events/ee-remove-all-once.js n=50000000 withType=false: ./node: 7297800 ./node-old: 3313100 . 120.27%
events/ee-remove-all.js n=60000000 withType=true: ./node: 9970200 ./node-old: 10165000 ....... -1.91%
events/ee-remove-all.js n=60000000 withType=false: ./node: 10459000 ./node-old: 10537000 ..... -0.74%
events/ee-remove.js n=20000000: ./node: 3143700 ./node-old: 3095900 ........................... 1.55%

[mcollina]

I am LGTM once address the breakage this will cause to readable-stream users.

[mscdex]

Just for fun I created an alternative implementation (that instead stores once handler count by event name on a separate object property) that trades a bit of performance on some benchmarks for compatibility with the existing readable stream implementation:

events/ee-add-once.js n=50000000: ./node: 1573700 ./node-old: 1038600 ...................... 51.53%
events/ee-add-remove-once.js n=25000000: ./node: 972150 ./node-old: 758150 ................. 28.23%
events/ee-add-remove.js n=5000000: ./node: 1051500 ./node-old: 1041300 ...................... 0.98%
events/ee-add.js n=10000000: ./node: 1598800 ./node-old: 1643200 ........................... -2.70%
events/ee-emit-multi-args.js n=25000000: ./node: 5583500 ./node-old: 4628200 ............... 20.64%
events/ee-emit-once.js n=25000000: ./node: 5460500 ./node-old: 1911100 .................... 185.73%
events/ee-emit.js n=50000000: ./node: 7919900 ./node-old: 7449800 ........................... 6.31%
events/ee-listener-count-on-prototype.js n=50000000: ./node: 43796000 ./node-old: 43782000 .. 0.03%
events/ee-listeners-many.js n=15000000: ./node: 2910900 ./node-old: 3163200 ................ -7.98%
events/ee-listeners.js n=90000000: ./node: 15780000 ./node-old: 16326000 ................... -3.34%
events/ee-remove-all-once.js n=50000000 withType=true: ./node: 3176900 ./node-old: 3301600 . -3.78%
events/ee-remove-all-once.js n=50000000 withType=false: ./node: 3244500 ./node-old: 3190300 . 1.70%
events/ee-remove-all.js n=60000000 withType=true: ./node: 8985700 ./node-old: 10030000 .... -10.41%
events/ee-remove-all.js n=60000000 withType=false: ./node: 9648600 ./node-old: 10491000 .... -8.03%
events/ee-remove.js n=20000000: ./node: 3057300 ./node-old: 3090200 ........................ -1.07%

[jasnell]

I've added a new blocked label as a way of identifying PR's that cannot progress because they are currently being blocked by some other requirement. In this particular case, this PR appears to be blocked by the potential breakage that it would introduce in the ecosystem. Once #6032 lands and has had some time to marinate in the ecosystem, this can be revisited.

[mcollina]

@jasnell I propose we introduce this in the first cut of v7, and we will deal with the breakage.

[jasnell]

The primary area of concern is with streams right? /cc @nodejs/streams

[mscdex]

@jasnell Pretty much, or anyone else that is directly inserting new events via _events instead of .on()/.once().

[jasnell]

I'm wondering if at some point we should use a different internal for _events and wrap _events in a getter that will print a deprecation message.

[mscdex]

@jasnell That will probably only work for so long until people probably just end up doing a find-replace with whatever the new property name is to "fix" their scripts.

[trevnorris]

ditto on keeping braces the same.

[trevnorris]

but IMHO that's relying on an implementation detail and is wrong

Unfortunately there are more than a few implementation details devs rely on that we can't freely change. This is known to be used in the wild. We should do some brief analysis to see the ecosystem impact because of this. I think APM's might take issue with this.

[jasnell]

ping @mscdex

[mscdex]

@jasnell Still stalled until at least the following are remedied:

• The readable-stream dependency issues can be figured out (basically the issue of people depending on old versions of the module), because these changes make some internal changes that those old versions would not be compatible with. This is really only an issue with people using old readable-stream in modules that takes precedence over the built-in versions.

• Other modules directly accessing ._events fixing their usage (including the module mentioned by @ChALkeR). I suppose we could solve this particular issue by switching to a "private" symbol and converting ._events to a getter/setter with a doc-only deprecation initially.

Additionally, I haven't ran benchmarks to compare these changes with the current .once() implementation which uses .bind().

[mcollina]

The readable-stream dependency issues can be figured out (basically the issue of people depending on old versions of the module), because these changes make some internal changes that those old versions would not be compatible with. This is really only an issue with people using old readable-stream in modules that takes precedence over the built-in versions.

It would be nice to get the breakdown of the download numbers for each version, not sure who to ask at npm. This is fixed on ^2.0.0, and both through2 and bl are fixed. Gulp 4 would be on readable-stream v2, which is good.
A solution might be to deprecate old versions of readable-stream when this fix lands.

@mscdex can we get a slower but compatible way to implement this API (maybe with a getter or a Proxy) that will be compatible?

[ronkorving]

@jasnell #6032 has landed. Do you still consider this PR blocked? (regardless of "stalled")

[fhinkel]

ping @jasnell

[jasnell]

Yes. @mscdex and @mcollina are really the ones to ask tho :)

[mcollina]

I think we might want to update this PR and get a fresh run of CITGM.
Specifically, this breaks anyone using readable-stream < 2.1.0 (or more likely something else depending on it). Do you any of you know how can I fetch how many downloads readable-stream < 2.1.0 has? how many OSS modules are depending on this? cc @seldo.

I'm actually 👍 of making this change in Node.js 8, pending a passing CITGM run. However, a safer approach might be to wait until 8 is cut and we start working on 9.

cc @nodejs/streams.

[fhinkel]

I'd also suggest we run this with V8 60 and not current master (V8 58) to make sure the performance improvements still hold.

[mscdex]

@fhinkel It will probably be awhile yet before something like this could land since the main issue (AFAIK) still is modules/users that depend on older versions of readable-stream (and/or node version combinations) that do not use prependListener().

[mcollina]

@mscdex @fhinkel currently we have 50% coverage of the ecosystem with the new readable-stream. If we want this to land, which figure will we be targeting? 80%, 90%?
I'm happy to try to move the community forward.

[mscdex]

@mcollina I don't know. It's going to be hard to gauge with end users of course (modules will be easier to spot on npm). Does @nodejs/ctc have any opinion?

[mcollina]

@mscdex I am referring to download stats from npm. I helped migrate the mysql module as well (it's still not released yet mysqljs/mysql#1677). A solution might be to backport the fix to the 1.x line of readable-stream.

[mscdex]

@mcollina Ah ok, I was referring to actually collecting data on dependencies usage on modules on npm. I did this once to see how many modules were not using semver specifiers that would include a recent enough readable-stream version. I don't think I have the script anymore though that pulls that data directly from the npm registry.

Even updating readable-stream v1.x may/may not be enough, I don't know how many modules are using a static version. *shrug*

[mcollina]

@mscdex it will be a long process. If you can't find that script, we might rewrite it and actively send PRs over.

[Trott]

@mscdex Should this remain open? Should some or all of the benchmark changes be landed?

[mscdex]

@Trott Yes, the problem/"blocker" is still the same.

[ronkorving]

Not limited to this PR, but I think everything labeled performance should be rebased and get a fresh benchmark run. CrankshaftScript is no longer relevant, and we may even be making things slower.

[BridgeAR]

@mscdex would it be possible to split this PR into two - one without breaking changes and one with? If that would be possible and the one without BC would still yield a performance improvement, I think it would be worth pursuing that instead of having the PR stalled for a very long time.

[BridgeAR]

I am relatively certain that the performance benefit will be much less with newer V8 versions.

As this is one of the oldest PRs and it seems like there is little progress and more and more conflicts are coming up due to changes in the code over time I am going ahead and close this.

@mscdex if you feel like this should stay open, please go ahead and reopen.