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

benchmark: add eventEmitter.once() benchmarks #912

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 21, 2015

No description provided.


bench.start();
for (var i = 0; i < n; i += 1) {
ee.emit('dummy');
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 a very fair benchmark? after all, wouldn't any run after the first run be only to the .on() listeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

just throwing ideas at the wall, but i'm thinking some sort of Object.freeze() (maybe deep-freeze) on the event emitter, which would (in non-strict mode) i believe silently fail when it is changed, leaving us the same object in each loop - thats the only idea that comes to mind that would only benchmark the .emit(), what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

How do const objects behave, would that do the job?

@mscdex mscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Mar 25, 2015
@brendanashworth
Copy link
Contributor

Right now, regarding these new benchmarks, I don't think most of them benchmark much. I don't really have any idea on how to fix that, but I don't think it's fair as-is.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 8, 2015

Well, at this point the benchmarks don't really matter unless #914 is merge-able. However as it stands, we need to know what kind of effect it will have on modules depending on readable-stream (i.e. who is depending on a fixed readable-stream version).

@brendanashworth
Copy link
Contributor

@mscdex perhaps this should just be closed and reopened if it seems fit?

@Qard
Copy link
Member

Qard commented Aug 8, 2015

Let's get @nodejs/benchmarking on this.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@mscdex mscdex closed this Feb 7, 2016
@mscdex mscdex deleted the more-ee-benchmarks-part2 branch February 7, 2016 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants