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

[std/times]getTime now uses high resolution API on windows #17901

Merged
merged 9 commits into from
Jun 23, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Apr 30, 2021

Function Return type Resolution Timezone
GetLocalTime SYSTEM_TIME (1ms) ~10-15 ms Local
GetSystemTime SYSTEM_TIME (1ms) ~10-15 ms UTC
GetSystemTimeAsFileTime FILE_TIME (0.1us) ~10-15 ms UTC
GetSystemTimePreciseAsFileTime FILE_TIME (0.1us) 0.1 us UTC
       
GetTickCount Int32 (1ms) ~10-15 ms n/a
GetTickCount64 Int64 (1ms) ~10-15 ms n/a
QueryPerformanceCounter Int64 (ticks) 0.1 us n/a

see https://stackoverflow.com/questions/34557407/queryperformancecounter-or-getsystemtimepreciseasfiletime-when-using-sntp

getTime on Unix (except Macos) uses high resolution, so make it more consistent on windows. Eventually Macos should use high resolution API too.

@rockcavera
Copy link
Contributor

The problem is how random.initRand() is initialized. The state is acquired through times.getTime(), which when requested in a short time, will always return the same value. I always thought this implementation was wrong. It would be okay to use monotimes.getMonoTime(), which always returns a different value.

About std/tempfiles, does the implementation really need to start a new rng state every time? I do not think so.

@ringabout ringabout changed the title [std/times]fix #17898(use high resolution times on windows) [std/times]fix partly #17898(use high resolution times on windows) Jun 1, 2021
@ringabout ringabout changed the title [std/times]fix partly #17898(use high resolution times on windows) [std/times]related to #17898(use high resolution times on windows) Jun 1, 2021
@ringabout ringabout changed the title [std/times]related to #17898(use high resolution times on windows) [std/times]related to #17898(getTime now use high resolution API on windows) Jun 1, 2021
@ringabout ringabout requested a review from timotheecour June 2, 2021 03:03
lib/pure/times.nim Outdated Show resolved Hide resolved
lib/pure/times.nim Outdated Show resolved Hide resolved
@ringabout ringabout changed the title [std/times]related to #17898(getTime now use high resolution API on windows) [std/times]getTime now use high resolution API on windows Jun 2, 2021
@ringabout ringabout changed the title [std/times]getTime now use high resolution API on windows [std/times]getTime now uses high resolution API on windows Jun 2, 2021
lib/pure/times.nim Outdated Show resolved Hide resolved
@ringabout ringabout added the Ready For Review (please take another look): ready for next review round label Jun 23, 2021
@ringabout ringabout requested a review from Varriount June 23, 2021 16:07
@ringabout ringabout closed this Jun 23, 2021
@ringabout ringabout reopened this Jun 23, 2021
@timotheecour timotheecour merged commit 496bd79 into nim-lang:devel Jun 23, 2021
@timotheecour
Copy link
Member

timotheecour commented Jun 30, 2021

@xflywind see #18403 which had to revert getSystemTimePreciseAsFileTime because it's not supported in windows 7; we should find a clean way to detect this to enable it when available, ideas welcome (ideally ones that generalize to other similar cases)

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants