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 more thorough timers benchmarks #10925

Merged
merged 1 commit into from
Jan 27, 2017

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Jan 20, 2017

Refs: #9493

Adds some new benchmarks to test pooled and unpooled behavior of the insert, cancel, and timeout operations of Node timers.

This is missing an unpooled tests for timeout, since that is far more complex, and I would prefer to do it separately in all likelihood.

cc @misterdjules, @AndreasMadsen, @nodejs/benchmarking

(This patch was made live during https://www.twitch.tv/nodesource/v/117497395 if you'd like to see me working on this in retrospect. :P)

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

benchmark, timer

@Fishrock123 Fishrock123 added benchmark Issues and PRs related to the benchmark subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jan 20, 2017
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. lts-watch-v6.x labels Jan 20, 2017
Copy link
Member

@gareth-ellis gareth-ellis left a comment

Choose a reason for hiding this comment

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

LGTM

});

function main(conf) {
var N = +conf.thousands * 1e3;
Copy link
Member

Choose a reason for hiding this comment

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

exceedingly minor nit... why N? I tend to dislike caps for variables that aren't const

@Fishrock123
Copy link
Contributor Author

ping again the rest @nodejs/benchmarking, do these look ok?

@Fishrock123
Copy link
Contributor Author

Refs: nodejs#9493
PR-URL: nodejs#10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Fishrock123
Copy link
Contributor Author

Oops, missed a variable rename, new Lint CI: https://ci.nodejs.org/job/node-test-linter/6685/

@Fishrock123 Fishrock123 merged commit c74e6fe into nodejs:master Jan 27, 2017
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 27, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
jasnell pushed a commit that referenced this pull request Mar 8, 2017
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
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. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants