Skip to content

benchmark: Improve event performance tests.#14052

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
Closed
bmeurer wants to merge 1 commit into from
Closed

benchmark: Improve event performance tests. #14052

bmeurer wants to merge 1 commit into from

Conversation

@bmeurer
Copy link
Member

@bmeurerbmeurer 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-botnodejs-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

@mcollinamcollina 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
MemberAuthor

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
MemberAuthor

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

@TimothyGuTimothyGu 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
MemberAuthor

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

@mcollinamcollina 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]>
@addaleaxaddaleax 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

MylesBorins commented Aug 15, 2017

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@targostargostargos approved these changes

+1 more reviewer

@TimothyGuTimothyGuTimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmarkIssues and PRs related to the benchmark subsystem.eventsIssues 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