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

Timing calculations are wrong #28

Closed
gdamore opened this issue Dec 28, 2019 · 0 comments · Fixed by #30
Closed

Timing calculations are wrong #28

gdamore opened this issue Dec 28, 2019 · 0 comments · Fixed by #30

Comments

@gdamore
Copy link
Contributor

gdamore commented Dec 28, 2019

There are a few issues with the timing calculations:

  • A rounding / type conversion error can lead to wildly incorrect values (I had one report that came up at 7 seconds, but really took something on the order of a few dozen milliseconds). This was seen on Windows. Preserving the values as nanoseconds and only doing a division at the end gives better results. (And it appears that dividing by 1e9 is better than multiplying by 1e-9 if you want to avoid loss due to casting bugs.)

  • The Linux MONONTONIC_RAW clock should not really be used. Besides not being portable, the values are not guaranteed to be microseconds -- and really you do want any adjtime adjustments because those reflect real-world timekeeping. (This makes it not as good for reproducible benchmarking, but the cpu clock is better for that anyway.)

  • There is an order of magnitude bug in the POSIX timings, leading to times being reported as 1/10th their actual value (this lead to a lot of confusion for me at one point). Hint, 1 billionth is not 10e-9, but rather 1e-9.

I have a PR coming that addresses these.

gdamore added a commit to gdamore/acutest that referenced this issue Dec 28, 2019
fixes mity#28 Timing calculations are wrong
fixes mity#29 TEST_CASE should evaluate as a statement (no trailing semicolon)
@mity mity closed this as completed in #30 Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant