Skip to content

Conversation

@Trott
Copy link
Member

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

@TrottTrott 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-botnodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label May 18, 2017
Copy link
Contributor

@refackrefack 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

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

@mscdexmscdex 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
@TrottTrottforce-pushed the precision-is-not-accuracy branch from 8bf68d5 to fb352aeCompareMay 19, 2017 03:15
@TrottTrott changed the title test: allow no duration in benchmark testsbenchmark: allow no duration in benchmark testsMay 19, 2017
@Trott
Copy link
MemberAuthor

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
MemberAuthor

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#13110Fixes: nodejs#13102Fixes: nodejs#12433 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in c3067b5

@TrottTrott 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: #13110Fixes: #13102Fixes: #12433 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
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: #13110Fixes: #13102Fixes: #12433 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@jasnelljasnell mentioned this pull request May 28, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@TrottTrott deleted the precision-is-not-accuracy branch January 13, 2022 22:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues 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

@Trott@mscdex@refack@bnoordhuis@joyeecheung@MylesBorins@nodejs-github-bot