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

Fix Windows resolution problems by replacing Data.Time.Clock.POSIX with System.Clock #24

Closed
conklech opened this issue Apr 9, 2013 · 7 comments

Comments

@conklech
Copy link

conklech commented Apr 9, 2013

On Windows, getPOSIXTime uses the C function GetSystemTimeAsFileTime, which gives low-resolution timings. As observed in issue #11, it will return the same value many times in a row. criterion therefore produces low-quality statistics under Windows. QueryPerformanceCounter is the appropriate timing source for benchmarks on Windows; we can access it with Clock.getTime Clock.Monotonic from the clock package. See example data below.

My patch, conklech/criterion@b6f657c, fixes the problem on Windows, but might create compatibility problems on some POSIX systems.

In issue #2, @coreyoconnor gave a similar patch, coreyoconnor/criterion@37b1216, which uses Clock.ProcessCPUTime instead. That has different semantics--which was the goal there. But it's not clear that Clock.ProcessCPUTime gives benchmark-grade data on Windows. It uses GetProcessTimes under the hood; MSDN doesn't explain where the data comes from, and I haven't tested it.

Under POSIX, time calls gettimeofday and clock calls clock_gettime. Cursory Google research indicates that clock_gettime with CLOCK_MONOTONIC is the proper benchmark timesource; gettimeofday can go backwards, for instance when ntp is adjusting the clock.

However, it looks like some systems don't implement CLOCK_MONOTONIC. Perhaps just OS X? I don't know. Someone with better POSIX knowledge should look into compatibility issues with CLOCK_MONOTONIC in non-Win32 environments.

On my Windows 7 laptop, criterion using time (i.e. criterion-0.6.2.1) reports an estimated resolution of about 3.2 msec, with the weird 200% outlier rate reported by others. However, the final report data shows that all measurements share the same two or three values:
image

After dropping in clock and using Clock.Monotonic, the resolution is reported as 4.5 msec but the outlier rate problem is gone, and the final report shows an actual distribution of data:
image

@mikesteele81
Copy link

I'm using Windows 7 64-bit. With this patch I see a similar improvement.

@Toxaris
Copy link

Toxaris commented Aug 10, 2013

What's the status of this? Is unpatched criterion good for benchmarking on Windows by now?

@bos
Copy link
Collaborator

bos commented Sep 4, 2013

Thanks, I've fixed this in 53d3203.

The fix is fairly involved because it needs to work on multiple platforms. I particularly commend you to read the comment at the start of the Windows file.

@bos bos closed this as completed Sep 4, 2013
@mikesteele81
Copy link

Thanks for the update. I'll use criterion within Windows more often, and see how this improves time measurements. It looks like the clock library does things similarly.

At first I thought calling QueryPerformanceFrequency once at the start of testing was the wrong way to go about it. I seem to remember a Microsoft example that called it once before every pair of calls to QueryPerformanceCounter, which indicated to me the performance counter's frequency was not stable.

According to a comment on QueryPerformanceFrequency, the behavior is affected by which timing hardware is available to Windows. Perhaps modern computers all have a stable "TimeStampCounter".

@dreixel
Copy link

dreixel commented Mar 6, 2014

I still see this bug with criterion-0.8.0.1. Is the patch not yet in the latest released version?

@bos
Copy link
Collaborator

bos commented Mar 7, 2014

There is not a release that has this fix. You'll want to use HEAD for a bit if you need it.

@dreixel
Copy link

dreixel commented Mar 7, 2014

Ok, thanks.

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

No branches or pull requests

5 participants