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

timeval: use QueryPerformanceCounter on Windows #3318

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@pps83
Copy link
Contributor

commented Nov 27, 2018

see #3309

There is confusing info floating around that QueryPerformanceCounter can leap etc, which might have been true long time ago, but no loner the case nowadays (perhaps starting from WinXP?). Also, boost and std::chrono::steady_clock use QueryPerformanceCounter in a similar way.

@pps83 pps83 force-pushed the pps83:master-time branch from 55d963e to d627736 Nov 27, 2018

@MarcelRaad

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Thanks! We have a helper function for verifying the Windows version:

* Curl_verify_windows_version()

This is because VerifyVersionInfo is only available starting from Windows 2000, the Windows 8 and later SDKs warn about GetVersionInfoEx and it doesn't work without a manifest, and UWP/WinRT supports neither.

@pps83 pps83 force-pushed the pps83:master-time branch 3 times, most recently from 34bd0a4 to 3394a1e Nov 27, 2018

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

can you please restart "LGTM analysis: C/C++ — Analysis failed (could not build the merge commit)" step.

@pps83 pps83 force-pushed the pps83:master-time branch from 3394a1e to 8194667 Nov 28, 2018

@jay

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

When you force push it should restart

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Yes, I had to come up with some random change to force push something new :)

@gvanem

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

How does QueryPerformanceCounter() behave on e.g. a laptop in low-power mode? I'd expect the CPU-frequency to drop and Curl_now() to behave strange.

How about timespec_get (&tspec_val, TIME_UTC) as a base? This function has been in the WindowsKit for some time. I assume it is much more monotonic.

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

AFAIK QueryPerformanceCounter is guaranteed to provide consistent results for quite some time. It does not use true CPU frequency. It used to have some of these issues long time ago, so in this change I set it to use QPC only starting from Vista (even though QPC is available starting from win95 probably)

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Still investigating, see https://stackoverflow.com/questions/44020619/queryperformancecounter-on-multi-core-processor-under-windows-10-behaves-erratic

Strange, they mention that TSC is used for QPC timer, however, I haven't seen that. AFAIK QPC always uses something else, as I've never seen that QPF returns some huge values closely related to CPU frequency; instead I've always seen values around 1000 times smaller than CPU frequency.

Also, keep in mind originally I simply matched implementation of std::chrono::steady_clock, that would be ridiculous if std::steady_clock was more like erratic_clock. Airplanes could fall, bank machines could throw out money with such bugs :)
I was aware about those QPC issues, but always thought it was resolved now and when I wanted to make this change I actually stepped through steady_clock and it simply uses QPC timer with no attempts to "align" or "check" returned clock values.

@pps83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

How does QueryPerformanceCounter() behave on e.g. a laptop in low-power mode? I'd expect the CPU-frequency to drop and Curl_now() to behave strange.

How about timespec_get (&tspec_val, TIME_UTC) as a base? This function has been in the WindowsKit for some time. I assume it is much more monotonic.

No idea. You'd need to step into these functions to see what they use internally (what API). There aren't many choices. GetSystemTimeAsFileTime is very likely candidate here, but it returns calendar time (e.g. user can change system time and your clock will jump, same applies for auto time adjustments done by the system).

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

@MarcelRaad @gvanem @jay I'll appreciate your input and guidance on where to go with this PR. Users will certainly appreciate higher resolution timers.

@bagder bagder requested review from MarcelRaad, gvanem and jay Dec 22, 2018

@MarcelRaad
Copy link
Member

left a comment

Looks good to me! I haven't encountered any issues with QPC/QPF on Windows Vista / Server 2008 and later myself yet.

@MarcelRaad

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@gvanem

How does QueryPerformanceCounter() behave on e.g. a laptop in low-power mode? I'd expect the CPU-frequency to drop and Curl_now() to behave strange.

The result of QueryPerformanceFrequency remains constant until the next system boot.

@gvanem

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

@MarcelRaad Okay, then this look all good to me.

@jay

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

AFAICT from Acquiring high-resolution time stamps Windows Vista+ will automatically select a source for the QPC that is not subject to synchronization issues across cores. Specifically it says that "the performance counter results are consistent across all processors in multi-core and multi-processor systems, even when measured on different threads or processes" with the two noted exceptions, neither of which affect us here. If there is a bug in the user's TSC or some other source Windows Vista+ should detect and avoid it, and if it's not then QPC is not working as documented. Though that stackoverflow report sounds credible I don't think it's significant enough to block this.

@jay jay closed this in e9ababd Dec 23, 2018

@jay

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

Thanks

@lock lock bot locked as resolved and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.