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

Jump to bottom

[libs][Unix][perf] Lazily initialize TimeZoneInfo names and order GetSystemTimeZones by Ids #88368

Merged
merged 17 commits into from
Jul 10, 2023
Merged

[libs][Unix][perf] Lazily initialize TimeZoneInfo names and order GetSystemTimeZones by Ids #88368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0935950
[libs] Remove unnecessary assignment TZifHead
mdh1418 Jul 3, 2023
032b737
[libs][perf] Add lazy initialization for TimeZoneInfo names
mdh1418 Jul 3, 2023
4a00b1f
Reduce comparisons for UTC alias and remove static array allocation
mdh1418 Jul 3, 2023
0e95c88
[libs] Lazy init display names for utc aliases
mdh1418 Jul 3, 2023
580a765
[libs][perf] Order system time zones by id
mdh1418 Jul 3, 2023
4adb545
Directly compare numerical value
mdh1418 Jul 4, 2023
ec41439
Fix internal field naming
mdh1418 Jul 5, 2023
b43beab
Remove TryPopulateTimeZoneDisplayNamesFromGlobalizationData
mdh1418 Jul 5, 2023
e933ea0
Make lazy initialization methods static on windows
mdh1418 Jul 5, 2023
25ea2bb
Revert "[libs][perf] Order system time zones by id"
mdh1418 Jul 5, 2023
b0cd70b
Fix lazy initialization for Minimal Globalization Data
mdh1418 Jul 5, 2023
b453345
Avoid lazy initialization where internal display name fields are set …
mdh1418 Jul 5, 2023
6d113e0
Fix CreateLocal not preserving lazy initialized names
mdh1418 Jul 5, 2023
e60c1a4
Prevent unintended lazy initialization in CreateCustomTimeZone
mdh1418 Jul 5, 2023
3ffa389
Make UICulture a property
mdh1418 Jul 5, 2023
a47ea8c
Substitute null name properties with empty string in default constructor
mdh1418 Jul 6, 2023
8d4bc4f
Assert not reached in Invariant mode
mdh1418 Jul 6, 2023
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
Prev Previous commit
Next Next commit
Substitute null name properties with empty string in default constructor
  • Loading branch information
@mdh1418
mdh1418 committed Jul 6, 2023
commit a47ea8ca28b72bdf33f3f83c52b8575710d0d3a3
12 changes: 9 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,24 @@ public AdjustmentRule[] GetAdjustmentRules()
private static string? PopulateDisplayName()
{
// Keep window's implementation to populate via constructor
// This should not be reached
Debug.Assert(false);
return null;
}

private static string? PopulateStandardDisplayName()
{
// Keep window's implementation to populate via constructor
// This should not be reached
Debug.Assert(false);
return null;
}

private static string? PopulateDaylightDisplayName()
{
// Keep window's implementation to populate via constructor
// This should not be reached
Debug.Assert(false);
return null;
}

Copy link
Member

Choose a reason for hiding this comment

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

would these ever be called on Windows? if not, then at least assert or throw exception here to catch any future wrong use of 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.

It looks like there could be cases where _displayName is still null, like when using the public APIs CreateCustomTimeZone. So if for some reason someone used TimeZoneInfo myTzi = CreateCustomTimeZone(<id>, <offset>, null, null, null, <adjustmentRules>) and later invoked myTzi.DisplayName, this would be hit.

I think in this case, we should honor that the internal fields have not been set and still return null?
But thats a good point.... I wouldn't want TimeZoneInfo objects created by CreateCustomTimeZone to have their fields unexpectedly populated (and incur the performance hit) if they intended for them to have a null value.

Would the best way to check that be by having some sort of boolean to track whether or not it had been set as null on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that CreateCustomTimeZone sets the names to string.Empty if they were null, these shouldn't be hit.

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 added Debug.Assert(false); in the window's implementation of these methods.

I modified the primary constructor in TimeZoneInfo.cs to have ?? string.Empty for the internal fields (and removed the modifications from CreateCustomTimeZone as they will eventually flow to this constructor.

private TimeZoneInfo(
                string id,
                TimeSpan baseUtcOffset,
                string? displayName,
                string? standardDisplayName,
                string? daylightDisplayName,
                AdjustmentRule[]? adjustmentRules,
                bool disableDaylightSavingTime,
                bool hasIanaId = false)

In doing so, I believe for windows the only other TimeZoneInfo constructor that is of interest is

runtime/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Win32.cs

Line 113 in cd9d541

private TimeZoneInfo(in TIME_ZONE_INFORMATION zone, bool dstDisabled)
, which sets the fields to non null values, so we should not hit these Populate* methods on Windows (unless I'm missing something else)

Moreover, this doesn't get used anywhere does it?

runtime/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs

Lines 1106 to 1117 in cd9d541

private TimeZoneInfo(SerializationInfo info, StreamingContext context)
{
ArgumentNullException.ThrowIfNull(info);
_id = (string)info.GetValue("Id", typeof(string))!; // Do not rename (binary serialization)
_displayName = (string?)info.GetValue("DisplayName", typeof(string)); // Do not rename (binary serialization)
_standardDisplayName = (string?)info.GetValue("StandardName", typeof(string)); // Do not rename (binary serialization)
_daylightDisplayName = (string?)info.GetValue("DaylightName", typeof(string)); // Do not rename (binary serialization)
_baseUtcOffset = (TimeSpan)info.GetValue("BaseUtcOffset", typeof(TimeSpan))!; // Do not rename (binary serialization)
_adjustmentRules = (AdjustmentRule[]?)info.GetValue("AdjustmentRules", typeof(AdjustmentRule[])); // Do not rename (binary serialization)
_supportsDaylightSavingTime = (bool)info.GetValue("SupportsDaylightSavingTime", typeof(bool))!; // Do not rename (binary serialization)
}

Copy link
Member

Choose a reason for hiding this comment

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

this doesn't get used anywhere does it?

I recall it can be used by some serialization engines like Data Contract serialization. Please keep it for compat.

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 even though it is a private constructor and isn't referenced in the TimeZoneInfo.*.cs?

Expand Down Expand Up @@ -918,9 +924,9 @@ private static TimeZoneInfoResult TryGetTimeZoneFromLocalMachine(string id, out
value = new TimeZoneInfo(
id,
new TimeSpan(0, -(defaultTimeZoneInformation.Bias), 0),
displayName,
standardName,
daylightName,
displayName ?? string.Empty,
standardName ?? string.Empty,
daylightName ?? string.Empty,
adjustmentRules,
disableDaylightSavingTime: false);

Expand Down
24 changes: 12 additions & 12 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 @@ -989,9 +989,9 @@ private TimeZoneInfo(

_id = id;
_baseUtcOffset = baseUtcOffset;
_displayName = displayName;
_standardDisplayName = standardDisplayName;
_daylightDisplayName = disableDaylightSavingTime ? null : daylightDisplayName;
_displayName = displayName ?? string.Empty;
_standardDisplayName = standardDisplayName ?? string.Empty;
_daylightDisplayName = disableDaylightSavingTime ? string.Empty : daylightDisplayName ?? string.Empty;
_supportsDaylightSavingTime = adjustmentRulesSupportDst && !disableDaylightSavingTime;
_adjustmentRules = adjustmentRules;

Expand All @@ -1012,9 +1012,9 @@ public static TimeZoneInfo CreateCustomTimeZone(
return new TimeZoneInfo(
id,
baseUtcOffset,
displayName ?? string.Empty,
standardDisplayName ?? string.Empty,
standardDisplayName ?? string.Empty,
displayName,
standardDisplayName,
standardDisplayName,
adjustmentRules: null,
disableDaylightSavingTime: false,
hasIanaId);
Expand All @@ -1034,9 +1034,9 @@ public static TimeZoneInfo CreateCustomTimeZone(
return CreateCustomTimeZone(
id,
baseUtcOffset,
displayName ?? string.Empty,
standardDisplayName ?? string.Empty,
daylightDisplayName ?? string.Empty,
displayName,
standardDisplayName,
daylightDisplayName,
adjustmentRules,
disableDaylightSavingTime: false);
}
Expand All @@ -1063,9 +1063,9 @@ public static TimeZoneInfo CreateCustomTimeZone(
return new TimeZoneInfo(
id,
baseUtcOffset,
displayName ?? string.Empty,
standardDisplayName ?? string.Empty,
daylightDisplayName ?? string.Empty,
displayName,
standardDisplayName,
daylightDisplayName,
adjustmentRules,
disableDaylightSavingTime,
hasIanaId);
Expand Down