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: allow no duration in benchmark tests #13110

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 18, 2017

Imprecision in process.hrtime() in some situations can result in a zero
duration being used as a denominator in benchmark tests. This would
almost certainly never happen in real benchmarks. It is only likely in
very short benchmarks like the type we run in our test suite to just
make sure that the benchmark code is runnable.

So, if the environment variable that we use in tests to indicate "allow
ludicrously short benchmarks" is set, convert a zero duration for
a benchmark to 1 nano-second.

Fixes: #13102

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test benchmark http

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels May 18, 2017
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label May 18, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Makes sense

@mscdex
Copy link
Contributor

mscdex commented May 19, 2017

I think this should probably be prefixed with benchmark: instead.

@mscdex mscdex removed http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels May 19, 2017
Imprecision in process.hrtime() in some situations can result in a zero
duration being used as a denominator in benchmark tests. This would
almost certainly never happen in real benchmarks. It is only likely in
very short benchmarks like the type we run in our test suite to just
make sure that the benchmark code is runnable.

So, if the environment variable that we use in tests to indicate "allow
ludicrously short benchmarks" is set, convert a zero duration for
a benchmark to 1 nano-second.

Fixes: nodejs#13102
@Trott Trott force-pushed the precision-is-not-accuracy branch from 8bf68d5 to fb352ae Compare May 19, 2017 03:15
@Trott Trott changed the title test: allow no duration in benchmark tests benchmark: allow no duration in benchmark tests May 19, 2017
@Trott
Copy link
Member Author

Trott commented May 19, 2017

I think this should probably be prefixed with benchmark: instead.

@mscdex Sure thing. Updated the PR title and the first line of the commit message as requested.

@Trott
Copy link
Member Author

Trott commented May 20, 2017

Trott added a commit to Trott/io.js that referenced this pull request May 22, 2017
Imprecision in process.hrtime() in some situations can result in a zero
duration being used as a denominator in benchmark tests. This would
almost certainly never happen in real benchmarks. It is only likely in
very short benchmarks like the type we run in our test suite to just
make sure that the benchmark code is runnable.

So, if the environment variable that we use in tests to indicate "allow
ludicrously short benchmarks" is set, convert a zero duration for
a benchmark to 1 nano-second.

PR-URL: nodejs#13110
Fixes: nodejs#13102
Fixes: nodejs#12433
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 22, 2017

Landed in c3067b5

@Trott Trott closed this May 22, 2017
jasnell pushed a commit that referenced this pull request May 23, 2017
Imprecision in process.hrtime() in some situations can result in a zero
duration being used as a denominator in benchmark tests. This would
almost certainly never happen in real benchmarks. It is only likely in
very short benchmarks like the type we run in our test suite to just
make sure that the benchmark code is runnable.

So, if the environment variable that we use in tests to indicate "allow
ludicrously short benchmarks" is set, convert a zero duration for
a benchmark to 1 nano-second.

PR-URL: #13110
Fixes: #13102
Fixes: #12433
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request May 23, 2017
Imprecision in process.hrtime() in some situations can result in a zero
duration being used as a denominator in benchmark tests. This would
almost certainly never happen in real benchmarks. It is only likely in
very short benchmarks like the type we run in our test suite to just
make sure that the benchmark code is runnable.

So, if the environment variable that we use in tests to indicate "allow
ludicrously short benchmarks" is set, convert a zero duration for
a benchmark to 1 nano-second.

PR-URL: #13110
Fixes: #13102
Fixes: #12433
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@Trott Trott deleted the precision-is-not-accuracy branch January 13, 2022 22:45
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

process.hrtime() unreliable?
7 participants