Leap Seconds Support #21420

tarekgh · 2018-12-06T22:33:56Z

Port the leap seconds changes done in the full framework to net core.

tarekgh · 2018-12-06T22:36:43Z

@jkotas this is just porting what we have done in the full framework. Also, I have created a commit for corert we can use when we are mirroring this change there tarekgh/corert@f79f7c8

could you please take a look at both and let me know if you have any comment? I have tested this on coreclr, corert and ProjectN native and Win32. I did the testing on Windows version which support leaps seconds with inserting some fake leap seconds to the system.

src/System.Private.CoreLib/src/System/DateTime.Unix.cs

src/classlibnative/bcltype/system.cpp

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

src/System.Private.CoreLib/src/System/DateTime.Unix.cs

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

src/classlibnative/bcltype/system.h

jkotas · 2018-12-06T23:57:36Z

Is there going to be a matching PR with the tests in CoreFX?

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

tarekgh · 2018-12-07T00:04:30Z

Is there going to be a matching PR with the tests in CoreFX?

We already have tests for DateTime/DateTimeOffset exercise Now, UtcNow. Writing any test specific for testing with leap seconds will not have any effect because we'll have to run on a machine support leap seconds (i.e. RS5) and the system will have a leaps seconds which we don't have any so far. This will be interesting more when we get in the future a leap seconds.

jkotas · 2018-12-07T02:55:23Z

We already have tests

There are two separate problems:

Where/how/when we have machines with the right config available in automation. I understand that this may be not easy to solve momentarily. I assume that you got the right machine yourself.
Tests that hit the new code on the machine with the right config. These tests should be failing before your change and passing after your change. This is the part that I think should be send to CoreFX.

tarekgh · 2018-12-07T03:37:04Z

Tests that hit the new code on the machine with the right config. These tests should be failing before your change and passing after your change. This is the part that I think should be send to CoreFX.

I'll write a test for that but I'll not expect this test will fail now and the possibility to fail in the future will be low. I understand your point and would be good to have it to catch if we would have a problem.

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

jkotas · 2018-12-08T00:10:33Z

src/System.Private.CoreLib/System.Private.CoreLib.csproj

@@ -432,6 +432,7 @@
  </ItemGroup>
  <ItemGroup Condition="'$(TargetsWindows)' == 'true'">
    <Compile Include="$(BclSourcesRoot)\System\DateTime.Windows.cs" />
+    <Compile Include="shared/Interop/Windows/NtDll/Interop.NtQuerySystemInformation.cs" />


This should be in the project in the shared partition

jkotas · 2018-12-08T00:12:14Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs


        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        internal static extern bool SystemTimeToSystemFileTime(ref FullSystemTime time, ref long fileTime);
+        internal static extern bool SystemTimeToSystemFileTime(ref Interop.Kernel32.SYSTEMTIME time, out long fileTime);


The first argument can be in Interop.Kernel32.SYSTEMTIME

jkotas · 2018-12-08T00:12:39Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

-
-        [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
-        internal static extern bool IsLeapSecondsSupportedSystem();
+        internal static extern bool ValidateSystemTime(ref Interop.Kernel32.SYSTEMTIME time, bool localTime);


The first argument can be in

jkotas · 2018-12-08T00:13:49Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

-            FullSystemTime time = new FullSystemTime();
-            if (SystemFileTimeToSystemTime(fileTime, ref time))
+            FullSystemTime time;
+            if (SystemFileTimeToSystemTime(fileTime, out time))


Nit: if (SystemFileTimeToSystemTime(fileTime, out FullSystemTime time))

jkotas · 2018-12-08T00:14:04Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

            FullSystemTime time = new FullSystemTime(ticks);
-            if (SystemTimeToSystemFileTime(ref time, ref fileTime))
+            if (SystemTimeToSystemFileTime(ref time.systemTime, out fileTime))


Nit: if (SystemTimeToSystemFileTime(ref time.systemTime, out long fileTime))

jkotas · 2018-12-08T00:21:13Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

+        internal static extern bool ValidateSystemTime(ref Interop.Kernel32.SYSTEMTIME time, bool localTime);
+
+        [MethodImplAttribute(MethodImplOptions.InternalCall)]
+        internal static extern bool SystemFileTimeToSystemTime(long fileTime, out FullSystemTime time);


Is there a reason why we call some of these things SystemFileTime? There is SYSTEMTIME and FILETIME today, but there is no SystemFileTime today.

I was trying to have it clear FILETIME is always a system file time. if you think this is confusing, I can change it.

jkotas · 2018-12-08T00:27:15Z

src/classlibnative/bcltype/system.cpp

+{
+    FCALL_CONTRACT;
+
+    if (::SystemTimeToFileTime(time, (LPFILETIME) pFileTime))


This does not need if - can just return the value returned by SystemTimeToFileTime.

BOOL ret = ::SystemTimeToFileTime(... FC_RETURN_BOOL(ret);

jkotas · 2018-12-08T00:28:43Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

+            return new DateTime(universalTicks, DateTimeKind.Utc);
+        }
+
+        public long ToFileTimeUtc() {


Nit: { should be on a new line

Those was a copy and paste from other file. I'll fix them.

jkotas · 2018-12-08T00:30:41Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

+            }
+
+            ticks -= FileTimeOffset;
+            if (ticks < 0) {


jkotas · 2018-12-08T00:34:00Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

+                hundredNanoSecond = 0;
+            }
+
+            internal Interop.Kernel32.SYSTEMTIME systemTime;


The coding convention is to place the fields first.

jkotas · 2018-12-08T00:51:25Z

src/System.Private.CoreLib/src/System/DateTime.Windows.cs

+            }
+        }
+
+        public static DateTime FromFileTimeUtc(long fileTime)


The only difference between Windows and Unix versions of FromFileTimeUtc and ToFileTimeUtc is the callout to the special method method for leap seconds. Should we have keep it in the shared and have a dummy method on Unix?

Also, it may be better to call InternalFromFileTime / InternalToFileTime something like FromFileTimeUtcLeapSecondsAware / ToFileTimeUtcLeapSecondAware.

jkotas · 2018-12-08T01:03:29Z

Do you have the number for performance hit that the methods like DateTime.UtcNow will incur on the leap second aware systems with this change?

I think we had it in one of the offline conversations ... it would be useful to have the number on here too.

jkotas · 2018-12-08T01:03:41Z

Looks good to me otherwise

tarekgh · 2018-12-08T01:35:38Z

Do you have the number for performance hit that the methods like DateTime.UtcNow will incur on the leap second aware systems with this change?

I don't have the exact numbers here and also I am using a VM for my leap seconds testing. but I have tested it and we always reporting the time on the same milliseconds boundary. Here example for what I was doing:

            SYSTEMTIME st = new SYSTEMTIME();
            GetSystemTime(ref st);
            DateTime dt = DateTime.UtcNow;
            Console.WriteLine($"<{st.wHour:D2}:{st.wMinute:D2}:{st.wSecond:D2}.{st.wMillisecond:D2}> .... [{dt.Hour:D2}:{dt.Minute:D2}:{dt.Second:D2}.{dt.Millisecond:D2}]");

Note that we still reporting the precise time as we used to do even in leap seconds system. the only extra perf is just the time we use till we return. I can do the measurements later to get the exact numbers. note that we already have a perf tests for this too. anyway, let me know if you want to wait not merging the change till we do that or we case merge and do the measurements later. if we want to wait, I'll hold this changes till next year.

jkotas · 2018-12-08T04:33:05Z

let me know if you want to wait not merging the change till we do that

I do not think you need to block on this.

tarekgh · 2018-12-08T04:40:21Z

test Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

tarekgh · 2018-12-08T04:42:04Z

thanks @jkotas for your review and comments. I will wait preparing the changes for corert too and test it so we'll be ready when we merge. I'll let you know when I have it ready to take a quick look there too.

tarekgh · 2018-12-08T05:33:55Z

here is the delta changes in corert repo too tarekgh/corert@b503742 i have tested it with PN too. after reviewing the delta, I'll squash moth commits in corert to one so it will be easy to use.

tarekgh · 2018-12-08T20:39:56Z

Thanks a lot @jkotas for your review. I have updated the corert too and squashed the commits into one commit tarekgh/corert@389615a

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

tarekgh · 2018-12-08T20:45:35Z

I added a comment too on corert change tarekgh/corert@389615a#r31609074 may be we need to apply.

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

tarekgh · 2018-12-09T06:03:22Z

@Anipik I am noticing there is no corert mirroring PR opened for this one. Is it expected?

Anipik · 2018-12-09T06:06:53Z

@tarekgh it is expected in the sense that there can be only one open mirror pr in a repo at any time.
And we have already have one in corert which hasnt yet merged dotnet/corert#6663
After this PR has been merged the next pr for corert repo will contain this commit

tarekgh · 2018-12-09T20:22:55Z

Thanks @Anipik

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

* Leap Seconds Support * Address Feedback * More feedback addressing

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

* Leap Seconds Support * Address Feedback * More feedback addressing Commit migrated from dotnet/coreclr@98e4995

Leap Seconds Support

88b479d

jkotas reviewed Dec 6, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/classlibnative/bcltype/system.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/classlibnative/bcltype/system.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

jkotas reviewed Dec 6, 2018

View reviewed changes

src/classlibnative/bcltype/system.h Outdated Show resolved Hide resolved

jkotas reviewed Dec 7, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 7, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

danmoseley reviewed Dec 7, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated Show resolved Hide resolved

Address Feedback

79a4612

jkotas reviewed Dec 8, 2018

View reviewed changes

src/System.Private.CoreLib/src/System/DateTime.Windows.cs Outdated

}

ticks -= FileTimeOffset;

if (ticks < 0) {

Copy link

Member

jkotas Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

jkotas reviewed Dec 8, 2018

View reviewed changes

More feedback addressing

beba619

jkotas approved these changes Dec 8, 2018

View reviewed changes

tarekgh merged commit 98e4995 into dotnet:master Dec 8, 2018

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Dec 8, 2018

Leap Seconds Support (dotnet/coreclr#21420)

75707e7

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

stephentoub pushed a commit to dotnet/corefx that referenced this pull request Dec 9, 2018

Leap Seconds Support (dotnet/coreclr#21420)

d7925a7

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

tarekgh mentioned this pull request Dec 9, 2018

Add DateTime Leap Seconds Tests dotnet/corefx#33939

Merged

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Dec 10, 2018

Leap Seconds Support (dotnet/coreclr#21420)

d9c5b9e

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

jkotas pushed a commit to dotnet/corert that referenced this pull request Dec 10, 2018

Leap Seconds Support (dotnet/coreclr#21420)

d99d777

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

morganbr pushed a commit that referenced this pull request Dec 14, 2018

Leap Seconds Support (#21420)

42b968c

* Leap Seconds Support * Address Feedback * More feedback addressing

jlennox pushed a commit to jlennox/corefx that referenced this pull request Dec 16, 2018

Leap Seconds Support (dotnet/coreclr#21420)

e3b8f38

* Leap Seconds Support * Address Feedback * More feedback addressing Signed-off-by: dotnet-bot <[email protected]>

tarekgh deleted the LeapSeconds branch March 25, 2019 15:30

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Leap Seconds Support (dotnet/coreclr#21420)

5918211

* Leap Seconds Support * Address Feedback * More feedback addressing Commit migrated from dotnet/coreclr@98e4995

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leap Seconds Support #21420

Leap Seconds Support #21420

tarekgh commented Dec 6, 2018

tarekgh commented Dec 6, 2018

jkotas commented Dec 6, 2018

tarekgh commented Dec 7, 2018

jkotas commented Dec 7, 2018

tarekgh commented Dec 7, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

tarekgh Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018

tarekgh Dec 8, 2018

jkotas Dec 8, 2018

jkotas Dec 8, 2018 •

edited

Loading

jkotas Dec 8, 2018 •

edited

Loading

jkotas commented Dec 8, 2018

jkotas commented Dec 8, 2018

tarekgh commented Dec 8, 2018

jkotas commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 9, 2018

Anipik commented Dec 9, 2018

tarekgh commented Dec 9, 2018

Leap Seconds Support #21420

Leap Seconds Support #21420

Conversation

tarekgh commented Dec 6, 2018

tarekgh commented Dec 6, 2018

jkotas commented Dec 6, 2018

tarekgh commented Dec 7, 2018

jkotas commented Dec 7, 2018

tarekgh commented Dec 7, 2018

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

Choose a reason for hiding this comment

jkotas Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas commented Dec 8, 2018

jkotas commented Dec 8, 2018

tarekgh commented Dec 8, 2018

jkotas commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 8, 2018

tarekgh commented Dec 9, 2018

Anipik commented Dec 9, 2018

tarekgh commented Dec 9, 2018

jkotas Dec 8, 2018 •

edited

Loading

jkotas Dec 8, 2018 •

edited

Loading