Implement Crystal::System::Time for win32 #5422

straight-shoota · 2017-12-21T00:13:50Z

compute_utc_offset: Always returns the current UTC offset for now.
There seems to be no easy method to access UTC offset at an arbitrary
point in time.
The offset calculation will change anyway with Add time zones support #5324
compute_utc_seconds_and_nanoseconds: Precision is currently only
100 nanoseconds because FILE_TIME has no higher resolution.
monotonic: Seems to work as expected. Precision can very depending on performance frequency.

RX14 · 2017-12-21T00:19:23Z

src/lib_c/x86_64-windows-msvc/c/time.cr

@@ -0,0 +1,33 @@
+lib LibC


Seems that all these definitions are in winbase.h, so perhaps they should be in winbase.cr?

RX14 · 2017-12-21T00:21:41Z

src/lib_c/x86_64-windows-msvc/c/sys/types.cr

+  alias Word = UInt16
+  alias WChar = UInt16
+  alias DWord = UInt32
+  alias BOOL = Int32


These are all in winapi.cr (which I actually think should be removed, would love to head the core team's opinion on this)

That depends on where they're expected to be defined in windows includes. AFAIK there is no winapi.h.

DWORDis declared in IntSafe.h, WCHAR in WinNT.h, WORD and BOOL in WinDef.h

https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx

Well I don't think they should be in sys/types (that's a header from the c99 standard), and if you are going to define these types in the PR, they should at least be removed from winapi.cr

I'll put them in their respective locations.

RX14 · 2017-12-21T00:23:17Z

src/lib_c/x86_64-windows-msvc/c/time.cr

+    daylight_bias : Long
+  end
+
+  fun get_time_zone_information = GetTimeZoneInformation(tz_info : TimeZoneInformation*) : DWord


The standard for functions (which I argued against) was to prefix them with an _ so that their case could be kept the same. See winapi.cr.

Okay, I didn't know that. I'll change it, allthough it seems more convenient to "crystalize" internal names. But there are pros and cons for both styles.

You don't need the _ anymore, just use GetTimeZoneInformation.

Opinion is to match syscalls 1:1. We don't have to go through crystal-like namings: they're not crystal methods, but Windows functions. Windows uses GetTimeZoneInformation not get_time_zone_information, let's respect that. It also makes automatically generating bindings (precomputed or on-the-fly) easier, and cleans up bindings by avoiding useless aliases.

RX14 · 2017-12-21T00:29:27Z

src/crystal/system/win32/time.cr

+    {(since_epoch / 10_000_000).to_i64 + WINDOWS_EPOCH_IN_SECONDS, since_epoch.remainder(10_000_000).to_i32 * 100}
+  end
+
+  @@performance_frequency = uninitialized Int64


@@performance_frequency = begin ret = LibC._QueryPerformanceFrequency(out frequency) raise Errno.new("QueryPerformanceFrequency") if ret == 0 frequency end

should work.

Or just a plain:

ret = LibC.QueryPerformanceFrequency(out frequency) raise Errno.new("QueryPerformanceFrequency") if ret == 0 @@performance_frequency = frequency

Or should it be lazy evaluated like mach_timebase_info for unix/macos?

I would prefer @RX14's version so you dont have a ret variable floating around

RX14 · 2017-12-21T00:36:00Z

src/crystal/system/win32/time.cr

+    LibC.get_system_time_as_file_time(out filetime)
+    since_epoch = (filetime.high_date_time.to_u64 << 32) | filetime.low_date_time.to_u64
+
+    {(since_epoch / 10_000_000).to_i64 + WINDOWS_EPOCH_IN_SECONDS, since_epoch.remainder(10_000_000).to_i32 * 100}


would be nice to name these constants: FILETIME_TICKS_PER_SECOND and NANOSECONDS_PER_FILETIME_TICK

FILETIME does not really represent abstract "ticks" but 100 nanosecond intervals. But I guess it's fine as a name and I don't have any better idea.

RX14 · 2017-12-21T00:36:15Z

src/crystal/system/win32/time.cr

+      raise Errno.new("query_performance_counter")
+    end
+
+    {ticks / @@performance_frequency, (ticks.remainder(1_000_000_000) * 1_000_000_000 / @@performance_frequency).to_i32}


ditto on the naming the constants.

RX14 · 2017-12-21T00:39:30Z

You should be able to at least account for daylight time using StandardDate/StandardBias and DaylightDate/DaylistBias

RX14 · 2017-12-21T00:41:16Z

src/lib_c/x86_64-windows-msvc/c/time.cr

+  end
+
+  fun get_time_zone_information = GetTimeZoneInformation(tz_info : TimeZoneInformation*) : DWord
+  fun get_system_time_as_file_time = GetSystemTimeAsFileTime(time : Filetime*)


Looks like you want GetSystemTimePreciseAsFileTime actually. It's fine that the precision is 100ns, it's fairly impossible to synchronise the system clock to be more accurate than that anyway.

Hm, I don't know if it makes any difference. Both return a FILETIME structure with 100 ns resolution and the values returned by both functions seem to be equally precise (at least on my system).
GetSystemTimePreciseAsFileTime is only available on Windows 8+. I don't know which windows versions Crystal should support but it should easily be older ones.

I've taken a look at Go's implementation and they use GetSystemTimePreciseAsFileTime as well. Might need some more investigation about the actual difference.

We should use the more precise one if it's there. The old one is remarkably inaccurate: see https://github.com/dotnet/coreclr/issues/5061.

Should we have a fallback for API prior to Windows 8?

Yes we should do the same library open and function pointer trick that the coreclr does in the pr linked from that issue.

RX14 · 2017-12-21T00:43:39Z

Thanks so much for this PR! It's largely a great PR, although it seems that we need to document (and agree that we want) some more conventions related to windows naming.

straight-shoota · 2017-12-21T09:26:49Z

Yeah, I was just trying to fit everything as I was going along. It seems, a few conventions exists that I wasn't aware of. Some guidance would be really helpfull for ongoing efforts to implement windows bindings.

straight-shoota · 2017-12-21T09:32:45Z

About copmputing the UTC offset: It could be more sophisticated. We could even use GetTimeZoneInformationForYear and use the dates to calculate the real offset at seconds. But this will need some work and I don't think it's worth bothering right now as the entire time zone handling needs an overhaul anyway. We can continue to improve once #5324 gets merged (or a different path is chosen).
For now, the entire time zone handling is very limited anyway so it shouldn't be a big issue that it always uses the current offset.

ysbaddaden · 2017-12-21T10:03:24Z

Some guidance would be really helpfull for ongoing efforts to implement windows bindings

Just match namings 1:1 for types, functions and struct fields as much as possible. If Windows uses UPPERCASE, then use UPPERCASE, if it uses CamelCase then use CamelCase, and so on. The only exception is lowercased type names, which must be transformed. Habit in bindings is to CamelCase them (e.g. size_t -> SizeT).

Last but not least: binding functions can start with an uppercase letter since some versions back, so LibC.GetTimeZoneInformation or LibC.LLVMDisposeModule work nicely.

straight-shoota · 2017-12-21T10:11:25Z

@ysbaddaden Thanks, that's the kind of guidance I was looking for 👍

bew · 2017-12-21T10:51:28Z

src/lib_c/x86_64-windows-msvc/c/winapi.cr

-  fun _GetTimeZoneInformation = GetTimeZoneInformation(tz_info : TIME_ZONE_INFORMATION*) : DWord
-  fun _GetSystemTimeAsFileTime = GetSystemTimeAsFileTime(time : FILETIME*)
+  fun get_time_zone_information = GetTimeZoneInformation(tz_info : TIME_ZONE_INFORMATION*) : DWord
+  fun get_system_time_as_file_time = GetSystemTimeAsFileTime(time : FILETIME*)


What's the difference with winbase.cr did I miss something ? Imo these modifications in this file are wrong

Indeed, that file was commited on error.

RX14 · 2017-12-21T21:50:33Z

src/crystal/system/win32/time.cr

  def self.compute_utc_offset(seconds : Int64) : Int32
-    raise NotImplementedError.new("Crystal::System::Time.compute_utc_offset")
+    ret = LibC.GetTimeZoneInformation(out zone_information)


Let's at least have a TODO on ignoring seconds.

straight-shoota · 2017-12-21T22:13:28Z

I've moved the type declarations to files corresponding to the header names and added some comments.

Specs are failing due to formatter error #5432

RX14 · 2017-12-21T22:41:54Z

src/crystal/system/win32/time.cr

@@ -10,6 +10,8 @@ module Crystal::System::Time
  NANOSECONDS_PER_SECOND    = 1_000_000_000
  FILETIME_TICKS_PER_SECOND = NANOSECONDS_PER_SECOND / NANOSECONDS_PER_FILETIME_TICK

+  # TODO: For now, this method returns the UTC offset currently in place.
+  # A proper implementation is required once Crystal's Time Zone API has been revised (see issue #5324)


I'd prefer not to mention unmerged PRs as if it's a certainty they will be merged. Just say that this is not a complete implementation.

RX14 · 2017-12-22T13:47:40Z

src/crystal/system/win32/time.cr

  end

  def self.compute_utc_seconds_and_nanoseconds : {Int64, Int32}
-    raise NotImplementedError.new("Crystal::System::Time.compute_utc_seconds_and_nanoseconds")
+    LibC.GetSystemTimePreciseAsFileTime(out filetime)


In the future we should use windows's dlopen equivalent to fallback to GetSystemTimeAsFileTime. Can we either do that in this PR (see this) or leave a TODO?

@RX14 that approach will be required if we want to support anything older than Windows 8 (function was introduced then).

Considering a few other places might require such approach, perhaps adding a TODO will be recommended.

Cheers.

Should this check be at the callsite or in the bindings definition?
But I think I'd rather leave the implementation out of this PR.

The dlopen and method exists check would want to be cached, yes. Leaving it for later should be fine.

We might want to create a helper API/macro for this eventually, not sure.

RX14 · 2017-12-22T21:24:46Z

src/crystal/system/win32/time.cr

  def self.compute_utc_offset(seconds : Int64) : Int32
-    raise NotImplementedError.new("Crystal::System::Time.compute_utc_offset")
+    ret = LibC.GetTimeZoneInformation(out zone_information)
+    raise Errno.new("GetTimeZoneInformation") if ret == -1


Looking at it again, shouldn't this be WinError?

Oh, I didn't know this class even existed... it's not used anywhere until now.

Yes because windows has both errno and GetLastError for the C99 and win32 APIs respectively.

We shouldn't keep WinError or expose these platform-specific APIs at 1.0 either.

RX14 · 2017-12-22T23:36:03Z

src/lib_c/x86_64-windows-msvc/c/win_nt.cr

@@ -0,0 +1,3 @@
+lib LibC
+  alias WChar = UInt16


Isn't this WCHAR?

RX14 · 2017-12-22T23:36:26Z

src/lib_c/x86_64-windows-msvc/c/win_def.cr

@@ -0,0 +1,4 @@
+lib LibC
+  alias Word = UInt16


Isn't this WORD?

RX14 · 2017-12-22T23:36:33Z

src/lib_c/x86_64-windows-msvc/c/int_safe.cr

@@ -0,0 +1,3 @@
+lib LibC
+  alias DWord = UInt32


Isn't this DWORD?

True. I was just copying that from winapi.cr. Changing that will require to change it in that file as well. Not sure if that should be in this PR... WDYT?

Ugh, leave it as-is for now and i'll send a PR to rip out winapi.cr and redo that to the "new" specs.

straight-shoota · 2017-12-25T18:21:02Z

This should probably wait for #5448. I'll rebase once it gets merged.

RX14 · 2017-12-25T21:10:05Z

We can merge in either order. I'd love a second review, or a lease to merge PRs which only touch code for windows with only one review.

* `compute_utc_offset`: Always returns the **current** UTC offset for now. There seems to be no easy method to access UTC offset at an arbitrary point in time. * `compute_utc_seconds_and_nanoseconds`: Precision is currently only 100 nanoseconds because `FILE_TIME` has no higher resolution. * `monotonic`

straight-shoota · 2018-01-02T22:49:34Z

Rebased on master including #5448 and squashed.

* `compute_utc_offset`: Always returns the **current** UTC offset for now. There seems to be no easy method to access UTC offset at an arbitrary point in time. * `compute_utc_seconds_and_nanoseconds`: Precision is currently only 100 nanoseconds because `FILE_TIME` has no higher resolution. * `monotonic`

RX14 requested changes Dec 21, 2017

View reviewed changes

bew reviewed Dec 21, 2017

View reviewed changes

straight-shoota force-pushed the jm-windows-time branch 2 times, most recently from 8210796 to 3e250d0 Compare December 21, 2017 19:33

straight-shoota mentioned this pull request Dec 21, 2017

Coordinate porting to Windows #5430

Open

22 tasks

straight-shoota force-pushed the jm-windows-time branch from 3e250d0 to 5d41ce9 Compare December 21, 2017 21:38

RX14 approved these changes Dec 21, 2017

View reviewed changes

straight-shoota force-pushed the jm-windows-time branch from 5d41ce9 to b2e3a00 Compare December 21, 2017 22:00

RX14 requested changes Dec 21, 2017

View reviewed changes

straight-shoota force-pushed the jm-windows-time branch 2 times, most recently from 2f8d810 to d18942a Compare December 22, 2017 12:55

RX14 requested changes Dec 22, 2017

View reviewed changes

RX14 reviewed Dec 22, 2017

View reviewed changes

RX14 approved these changes Dec 22, 2017

View reviewed changes

RX14 requested changes Dec 22, 2017

View reviewed changes

RX14 approved these changes Dec 23, 2017

View reviewed changes

straight-shoota force-pushed the jm-windows-time branch from 17317af to 2896cd5 Compare January 2, 2018 22:46

RX14 approved these changes Jan 3, 2018

View reviewed changes

asterite approved these changes Jan 3, 2018

View reviewed changes

RX14 merged commit 5c60ae2 into crystal-lang:master Jan 3, 2018

RX14 added kind:feature topic:stdlib platform:windows labels Jan 3, 2018

RX14 added this to the Next milestone Jan 3, 2018

straight-shoota deleted the jm-windows-time branch January 3, 2018 12:50

Implement Crystal::System::Time for win32 #5422

Implement Crystal::System::Time for win32 #5422

Conversation

straight-shoota commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Dec 21, 2017

straight-shoota commented Dec 21, 2017

straight-shoota commented Dec 21, 2017

ysbaddaden commented Dec 21, 2017

straight-shoota commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Dec 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota Dec 22, 2017 • edited

Choose a reason for hiding this comment

RX14 Dec 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Dec 25, 2017

RX14 commented Dec 25, 2017

straight-shoota commented Jan 2, 2018

RX14 Dec 21, 2017 •

edited

straight-shoota Dec 21, 2017 •

edited

RX14 Dec 22, 2017 •

edited

straight-shoota Dec 22, 2017 •

edited

RX14 Dec 22, 2017 •

edited