From 785504ce1849e870e7f24d8a13d9bbd416e0d127 Mon Sep 17 00:00:00 2001 From: Justin Van Patten Date: Fri, 23 Dec 2016 16:24:28 -0800 Subject: [PATCH] TimeZoneInfo Cleanup Some cleanup in CoreRT's TimeZoneInfo implementation: - The CachedData locking is currently inconsistent. Some code is using a private Lock object with LockHolder.Hold, while other code is locking against the CachedData instance. This change removes the private Lock object and just locks against the CachedData instance throughout, which is consistent with the CoreCLR implementation. - TimeZoneInfoComparer is being used in two places in CoreRT to sort the list of system time zones, whereas it's only used in one place in CoreCLR. This change refactors the code to have a shared implementation of GetSystemTimeZones in CoreRT that handles the sorting in one place, allowing a single Comparison lambda to be used, which matches the CoreCLR implementation. Now there is a single shared GetSystemTimeZones implementation which calls a private static PopulateAllSystemTimeZones method, which is implemented separately for Win32/WinRT/Unix. - The WinRT implementation of FindSystemTimeZoneById was accessing the cached data without any locking. Added locking. --- .../src/System/TimeZoneInfo.Unix.cs | 7 +- .../src/System/TimeZoneInfo.Win32.cs | 55 ++--------- .../src/System/TimeZoneInfo.WinRT.cs | 42 +++++--- .../src/System/TimeZoneInfo.cs | 97 ++++++++++--------- 4 files changed, 93 insertions(+), 108 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs b/src/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs index aa235f124a9..3fcaa177642 100644 --- a/src/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs +++ b/src/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs @@ -15,9 +15,11 @@ using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Diagnostics; using System.Diagnostics.Contracts; using System.Globalization; using System.Runtime.CompilerServices; +using System.Threading; namespace System { @@ -142,11 +144,10 @@ public static TimeZoneInfo FindSystemTimeZoneById(string id) throw new NotImplementedException(); } - public static ReadOnlyCollection GetSystemTimeZones() + private static void PopulateAllSystemTimeZones(CachedData cachedData) { + Debug.Assert(Monitor.IsEntered(cachedData)); // UNIXTODO - if (s_cachedData._allSystemTimeZonesRead) - s_cachedData._allSystemTimeZonesRead = true; throw new NotImplementedException(); } diff --git a/src/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs b/src/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs index 69ad37dcd90..cd0285de0a5 100644 --- a/src/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs +++ b/src/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs @@ -83,61 +83,22 @@ public AdjustmentRule[] GetAdjustmentRules() return (AdjustmentRule[])_adjustmentRules.Clone(); } - // - // GetSystemTimeZones - - // - // returns a ReadOnlyCollection containing all valid TimeZone's - // from the local machine. The entries in the collection are sorted by - // 'DisplayName'. - // - // This method does *not* throw TimeZoneNotFoundException or - // InvalidTimeZoneException. - // - // - // - // - public static ReadOnlyCollection GetSystemTimeZones() + private static void PopulateAllSystemTimeZones(CachedData cachedData) { - CachedData cachedData = s_cachedData; + Debug.Assert(Monitor.IsEntered(cachedData)); - lock (cachedData) + using (RegistryKey reg = RegistryKey.GetBaseKey(RegistryKey.HKEY_LOCAL_MACHINE).OpenSubKey(c_timeZonesRegistryHive, writable: false)) { - if (cachedData._readOnlySystemTimeZones == null) + if (reg != null) { - using (RegistryKey reg = RegistryKey.GetBaseKey(RegistryKey.HKEY_LOCAL_MACHINE).OpenSubKey( - c_timeZonesRegistryHive, - false - )) + foreach (string keyName in reg.GetSubKeyNames()) { - if (reg != null) - { - foreach (string keyName in reg.GetSubKeyNames()) - { - TimeZoneInfo value; - Exception ex; - TryGetTimeZone(keyName, false, out value, out ex, cachedData); // populate the cache - } - } - cachedData._allSystemTimeZonesRead = true; - } - - List list = new List(); - if (cachedData._systemTimeZones != null) - { - // return a collection of the cached system time zones - foreach (var pair in cachedData._systemTimeZones) - { - list.Add(pair.Value); - } + TimeZoneInfo value; + Exception ex; + TryGetTimeZone(keyName, false, out value, out ex, cachedData); // populate the cache } - - // sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user - list.Sort(new TimeZoneInfoComparer()); - - cachedData._readOnlySystemTimeZones = new ReadOnlyCollection(list); } } - return cachedData._readOnlySystemTimeZones; } // -------- SECTION: constructors -----------------* diff --git a/src/System.Private.CoreLib/src/System/TimeZoneInfo.WinRT.cs b/src/System.Private.CoreLib/src/System/TimeZoneInfo.WinRT.cs index 4d3ed2e12cb..9b61daf1261 100644 --- a/src/System.Private.CoreLib/src/System/TimeZoneInfo.WinRT.cs +++ b/src/System.Private.CoreLib/src/System/TimeZoneInfo.WinRT.cs @@ -50,9 +50,20 @@ public AdjustmentRule[] GetAdjustmentRules() return (AdjustmentRule[])_adjustmentRules.Clone(); } - public static ReadOnlyCollection GetSystemTimeZones() + private static void PopulateAllSystemTimeZones(CachedData cachedData) { - return s_cachedData.GetOrCreateReadonlySystemTimes(); + Debug.Assert(Monitor.IsEntered(cachedData)); + + uint index = 0; + TIME_DYNAMIC_ZONE_INFORMATION tdzi; + while (Interop.mincore.EnumDynamicTimeZoneInformation(index, out tdzi) != Interop.mincore.Errors.ERROR_NO_MORE_ITEMS) + { + TimeZoneInformation timeZoneInformation = new TimeZoneInformation(tdzi); + TimeZoneInfo value; + Exception e; + TimeZoneInfoResult result = TryGetTimeZone(ref timeZoneInformation, false, out value, out e, cachedData); + index++; + } } public static TimeZoneInfo FindSystemTimeZoneById(string id) @@ -76,21 +87,26 @@ public static TimeZoneInfo FindSystemTimeZoneById(string id) TimeZoneInfo value; CachedData cache = s_cachedData; - // Use the current cache if it exists - if (cache._systemTimeZones != null) + + lock (cache) { - if (cache._systemTimeZones.TryGetValue(id, out value)) + // Use the current cache if it exists + if (cache._systemTimeZones != null) { - return value; + if (cache._systemTimeZones.TryGetValue(id, out value)) + { + return value; + } } - } - // See if the cache was fully filled, if not, fill it then check again. - if (!cache.AreSystemTimesEnumerated) - { - cache.EnumerateSystemTimes(); - if (cache._systemTimeZones.TryGetValue(id, out value)) + // See if the cache was fully filled, if not, fill it then check again. + if (!cache._allSystemTimeZonesRead) { - return value; + PopulateAllSystemTimeZones(cache); + cachedData._allSystemTimeZonesRead = true; + if (cache._systemTimeZones != null && cache._systemTimeZones.TryGetValue(id, out value)) + { + return value; + } } } throw new ArgumentException(String.Format(SR.Argument_TimeZoneNotFound, id)); diff --git a/src/System.Private.CoreLib/src/System/TimeZoneInfo.cs b/src/System.Private.CoreLib/src/System/TimeZoneInfo.cs index 4917953d127..10695274bff 100644 --- a/src/System.Private.CoreLib/src/System/TimeZoneInfo.cs +++ b/src/System.Private.CoreLib/src/System/TimeZoneInfo.cs @@ -84,11 +84,10 @@ internal enum TimeZoneInfoResult private class CachedData { private volatile TimeZoneInfo _localTimeZone; - private Lock _lock = new Lock(); private TimeZoneInfo CreateLocal() { - using (LockHolder.Hold(_lock)) + lock (this) { TimeZoneInfo timeZone = _localTimeZone; if (timeZone == null) @@ -203,39 +202,6 @@ public bool Equals(OrdinalIgnoreCaseString other) public volatile ReadOnlyCollection _readOnlySystemTimeZones; public bool _allSystemTimeZonesRead; - public void EnumerateSystemTimes() - { - LowLevelListWithIList enumeratedTimeZoneList = new LowLevelListWithIList(); - uint index = 0; - TIME_DYNAMIC_ZONE_INFORMATION tdzi; - while (Interop.mincore.EnumDynamicTimeZoneInformation(index, out tdzi) != Interop.mincore.Errors.ERROR_NO_MORE_ITEMS) - { - TimeZoneInformation timeZoneInformation = new TimeZoneInformation(tdzi); - TimeZoneInfo value; - Exception e; - TimeZoneInfoResult result = TryGetTimeZone(ref timeZoneInformation, false, out value, out e, s_cachedData); - if (result == TimeZoneInfoResult.Success) - { - enumeratedTimeZoneList.Add(value); - } - index++; - } - TimeZoneInfo[] array = enumeratedTimeZoneList.ToArray(); - Array.Sort(array, new TimeZoneInfoComparer()); - _readOnlySystemTimeZones = new ReadOnlyCollection(array); - } - - public bool AreSystemTimesEnumerated { get { return _readOnlySystemTimeZones != null; } } - - public ReadOnlyCollection GetOrCreateReadonlySystemTimes() - { - if (_readOnlySystemTimeZones == null) - { - EnumerateSystemTimes(); - } - return _readOnlySystemTimeZones; - } - private static TimeZoneInfo GetCurrentOneYearLocal() { // load the data from the OS @@ -967,6 +933,57 @@ public override int GetHashCode() return StringComparer.OrdinalIgnoreCase.GetHashCode(_id); } + // + // GetSystemTimeZones - + // + // returns a ReadOnlyCollection containing all valid TimeZone's + // from the local machine. The entries in the collection are sorted by + // 'DisplayName'. + // + // This method does *not* throw TimeZoneNotFoundException or + // InvalidTimeZoneException. + // + public static ReadOnlyCollection GetSystemTimeZones() + { + CachedData cachedData = s_cachedData; + + lock (cachedData) + { + if (cachedData._readOnlySystemTimeZones == null) + { + PopulateAllSystemTimeZones(cachedData); + cachedData._allSystemTimeZonesRead = true; + + List list; + if (cachedData._systemTimeZones != null) + { + // return a collection of the cached system time zones + list = new List(cachedData._systemTimeZones.Count); + foreach (var pair in cachedData._systemTimeZones) + { + list.Add(pair.Value); + } + } + else + { + // return an empty collection + list = new List(); + } + + // sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user + list.Sort((x, y) => + { + // sort by BaseUtcOffset first and by DisplayName second - this is similar to the Windows Date/Time control panel + int comparison = x.BaseUtcOffset.CompareTo(y.BaseUtcOffset); + return comparison == 0 ? string.CompareOrdinal(x.DisplayName, y.DisplayName) : comparison; + }); + + cachedData._readOnlySystemTimeZones = new ReadOnlyCollection(list); + } + } + return cachedData._readOnlySystemTimeZones; + } + // // HasSameRules - // @@ -1913,16 +1930,6 @@ internal static Boolean UtcOffsetOutOfRange(TimeSpan offset) } } - private class TimeZoneInfoComparer : IComparer - { - int System.Collections.Generic.IComparer.Compare(TimeZoneInfo x, TimeZoneInfo y) - { - // sort by BaseUtcOffset first and by DisplayName second - this is similar to the Windows Date/Time control panel - int comparison = x.BaseUtcOffset.CompareTo(y.BaseUtcOffset); - return comparison == 0 ? String.Compare(x.DisplayName, y.DisplayName, StringComparison.Ordinal) : comparison; - } - } - // ----- SECTION: private serialization instance methods ----------------*