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.

Sign up for GitHub

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

[libs][TimeZoneInfo] Add switch for unsorted GetSystemTimeZones #88794

Closed
wants to merge 1 commit into from
Closed

[libs][TimeZoneInfo] Add switch for unsorted GetSystemTimeZones #88794

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ internal enum TimeZoneInfoOptions
[TypeForwardedFrom("System.Core, Version=3.5.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
public sealed partial class TimeZoneInfo : IEquatable<TimeZoneInfo?>, ISerializable, IDeserializationCallback
{
internal static bool Unsorted { get; } = AppContextConfigHelper.GetBooleanConfig("System.TimeZoneInfo.SystemTimeZonesUnsorted", "DOTNET_SYSTEM_TIME_ZONE_INFO_SYSTEM_TIME_ZONES_UNSORTED");
Copy link
Member

Choose a reason for hiding this comment

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

DOTNET_SYSTEM_TIME_ZONE_INFO_SYSTEM_TIME_ZONES_UNSORTED

That is too long. maybe DOTNET_SYSTEM_TIME_ZONES_UNSORTED would be good enough.

Copy link
Member

Choose a reason for hiding this comment

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

word UNSORTED is confusing too. maybe we can use DOTNET_SYSTEM_TIME_ZONES_SORT_USING_ID?

Copy link
Member Author

@mdh1418 mdh1418 Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From the note in the description (I realized I didnt put it in the description... just a comment, sorry), I was aiming to implement the most performant variant of GetSystemTimeZones(), which also avoids the cost of sorting completely.

Copy link
Member

Choose a reason for hiding this comment

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

The API is documented to return sorted timezones. How are people going to figure out when it is safe to set this switch and that it is not going to introduce subtle breaks in their app?

Copy link
Member

Choose a reason for hiding this comment

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

I do not think we should be introducing this config switch. The config switch has major usability issues.

If we believe that it is important to avoid the sorting for perf, we should introduce the API overload that skips the sorting as proposed in the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the default behavior of the switch when unset is to go with the sorted variation, I was imagining that only the users with performance in mind would look for the switch and enable it. As for when it would be safe to set, if this switch just changes the order of the timezones in the collection, wouldn't it be on the user's responsibility if they found the switch and enabled it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind switching to the API overload. I was curious from @tarekgh's comment, would it be a longer process because it would go into API review as it changes a documented API?

Copy link
Member

Choose a reason for hiding this comment

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

The user would need to review usage of this time zone API in all libraries used by their app and repeat this review with every update of libraries used by their app to validate that the switch is not breaking them. It is very poor experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, it would affect all instances of GetSystemTimeZones because it would either be an app config or environment variable set. Whereas if we switched to an overload, it would only affect the instances where the overload is supplied as false?

Copy link
Member

Choose a reason for hiding this comment

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

Right

Copy link
Member

Choose a reason for hiding this comment

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

Right

private enum TimeZoneInfoResult
{
Success = 0,
Expand Down Expand Up @@ -133,6 +134,7 @@ public DateTimeKind GetCorrespondingKind(TimeZoneInfo? timeZone)
public ReadOnlyCollection<TimeZoneInfo>? _readOnlySystemTimeZones;
public Dictionary<string, TimeZoneInfo>? _timeZonesUsingAlternativeIds;
public bool _allSystemTimeZonesRead;
public bool _systemTimeZonesSorted;
}

// used by GetUtcOffsetFromUtc (DateTime.Now, DateTime.ToLocalTime) for max/min whole-day range checks
Expand Down Expand Up @@ -886,31 +888,43 @@ public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones()

lock (cachedData)
{
if (cachedData._systemTimeZonesSorted)
{
Debug.Assert(cachedData._readOnlySystemTimeZones != null);
return cachedData._readOnlySystemTimeZones;
}

if (cachedData._readOnlySystemTimeZones == null)
{
PopulateAllSystemTimeZones(cachedData);
cachedData._allSystemTimeZonesRead = true;
}

if (cachedData._systemTimeZones != null)
{
// return a collection of the cached system time zones
TimeZoneInfo[] array = new TimeZoneInfo[cachedData._systemTimeZones.Count];
cachedData._systemTimeZones.Values.CopyTo(array, 0);

// sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user
Array.Sort(array, static (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;
});
if (cachedData._systemTimeZones == null)
{
cachedData._readOnlySystemTimeZones = ReadOnlyCollection<TimeZoneInfo>.Empty;
}
else if (Unsorted)
{
List<TimeZoneInfo> unsortedSystemTimeZones = new List<TimeZoneInfo>(cachedData._systemTimeZones.Values);
cachedData._readOnlySystemTimeZones = new ReadOnlyCollection<TimeZoneInfo>(unsortedSystemTimeZones);
}
else
Copy link
Member

@tarekgh tarekgh Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why all this change? isn't enough only change line 924 to test Unsorted and decide to use the Id or DisplayName.

I don't think we need to return unsorted list at all. I think the perf hit coming from display name which is we try to avoid here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to cover the scenario in which a user grabs an unsorted collection and then changes their mind and grabs a sorted collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

With that in mind, for the second call, the scenarios in which cachedData._readOnlySystemTimeZones != null are

  1. Empty case
  2. Unsorted Case
  3. Sorted Case

And the topmost early return I was thinking if users already did the work to calculate the sorted collection, only return the sorted collection if there are future calls, ignoring calls to get an unsorted collection (I dont know what value there would be in allowing users to get the unsorted collection after already having a sorted collection as at that point they already hit the performance cost).

Copy link
Member Author

@mdh1418 mdh1418 Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I forgot to mention that a note in the description comment was that I figured since part of the point of this version was to be the most performant version of GetSystemTimeZones that it would just avoid sorting altogether, so the users wont incur any extra performance cost. It turned out though, that the unsorted list is mostly sorted by IDs except for having UTC at the beginning.

{
cachedData._systemTimeZonesSorted = true;
// return a collection of the cached system time zones
TimeZoneInfo[] array = new TimeZoneInfo[cachedData._systemTimeZones.Count];
cachedData._systemTimeZones.Values.CopyTo(array, 0);

cachedData._readOnlySystemTimeZones = new ReadOnlyCollection<TimeZoneInfo>(array);
}
else
// sort and copy the TimeZoneInfo's into a ReadOnlyCollection for the user
Array.Sort(array, static (x, y) =>
{
cachedData._readOnlySystemTimeZones = ReadOnlyCollection<TimeZoneInfo>.Empty;
}
// 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<TimeZoneInfo>(array);
}
}

Expand Down