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.

Sign up for GitHub

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

Jump to bottom

benchmark: Improve event performance tests. #14052

Closed
wants to merge 1 commit into from
Closed

benchmark: Improve event performance tests. #14052

wants to merge 1 commit into from

Conversation

bmeurer
Copy link
Member

@bmeurer bmeurer commented Jul 3, 2017

Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

@bmeurer
benchmark: Improve event performance tests.
35e9220 
Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. events Issues and PRs related to the events subsystem / EventEmitter. labels Jul 3, 2017
mcollina
mcollina approved these changes Jul 3, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM. For completeness, can you include the result before and after on master?

@bmeurer
Copy link
Member Author

bmeurer commented Jul 3, 2017

Not sure what you mean by that?

@mcollina
Copy link
Member

mcollina commented Jul 3, 2017

The output of running those benchmarks before and after the change you are proposing.

@bmeurer
Copy link
Member Author

bmeurer commented Jul 3, 2017

Oh, sure, here you go.

Before:

events/ee-add-remove.js
events/ee-add-remove.js n=250000: 848,698.3262555514

events/ee-emit-multi-args.js
events/ee-emit-multi-args.js n=2000000: 5,374,930.594864961

events/ee-emit.js
events/ee-emit.js n=2000000: 7,320,512.249184381

events/ee-listener-count-on-prototype.js
events/ee-listener-count-on-prototype.js n=50000000: 74,376,361.09205651

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 3,751,912.787045397

events/ee-listeners.js
events/ee-listeners.js n=5000000: 22,128,329.85005392

events/ee-once.js
events/ee-once.js n=20000000: 3,601,372.729697265

After:

events/ee-add-remove.js
events/ee-add-remove.js n=250000: 837,678.6629533422

events/ee-emit-multi-args.js
events/ee-emit-multi-args.js n=2000000: 5,403,667.907105654

events/ee-emit.js
events/ee-emit.js n=2000000: 7,393,322.197610017

events/ee-listener-count-on-prototype.js
events/ee-listener-count-on-prototype.js n=50000000: 66,343,020.996016376

events/ee-listeners-many.js
events/ee-listeners-many.js n=5000000: 6,519,582.274038669

events/ee-listeners.js
events/ee-listeners.js n=5000000: 28,619,631.881473146

events/ee-once.js
events/ee-once.js n=20000000: 3,542,028.4211871717

@mcollina
Copy link
Member

mcollina commented Jul 3, 2017

Thanks @bmeurer!

TimothyGu
TimothyGu approved these changes Jul 4, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM. Cf. #12657 (comment).

@bmeurer
Copy link
Member Author

bmeurer commented Jul 4, 2017

ACK.

@mcollina
Copy link
Member

mcollina commented Jul 5, 2017

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/8985/

@mcollina
Copy link
Member

mcollina commented Jul 5, 2017

Landed as 0011eb2

@mcollina mcollina closed this Jul 5, 2017
mcollina pushed a commit that referenced this pull request Jul 5, 2017
@bmeurer @mcollina
benchmark: Improve event performance tests.
0011eb2 
Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.

PR-URL: #14052
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@bmeurer @addaleax
benchmark: Improve event performance tests.
d1e9e40 
Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.

PR-URL: #14052
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@bmeurer @addaleax
benchmark: Improve event performance tests.
65a2e80 
Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.

PR-URL: #14052
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@bmeurer @Fishrock123
benchmark: Improve event performance tests.
9dbeea0 
Currently most of the event tests only test a single event type,
which might let those benchmark take fast-paths (in V8) that aren't
taken in realistic use cases, and thus the benchmarks are not good
proxies of real world uses.

PR-URL: #14052
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bmeurer @mcollina @MylesBorins @TimothyGu @targos @nodejs-github-bot