[libs][Unix][perf] Lazily initialize TimeZoneInfo names and order GetSystemTimeZones by Ids #88368
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @dotnet/area-system-datetime Issue DetailsFixes dotnet/android#8050 The Example timing breakdown This PR looks to improve the performance of APIs invoking a large number of
On average (on my machine, Mac mini M1, 2020 with 16GB memory running on OS 13.4 using Android Studio Emulator), GetSystemTimeZones OrderingBefore(click to expand)
After(click to expand)
|
This was referenced Jul 4, 2023
grendello
approved these changes
Jul 4, 2023
tarekgh
reviewed
Jul 4, 2023
| @@ -875,7 +902,7 @@ public static ReadOnlyCollection<TimeZoneInfo> GetSystemTimeZones() | |||
| { | |||
| // 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; | |||
| return comparison == 0 ? string.CompareOrdinal(x.Id, y.Id) : comparison; | |||
There was a problem hiding this comment.
Sorting by DisplayName will hurt performance considerably because this line may be the first call to lazily load the DisplayNames. When ordering by Id, this sort operation takes 1ms, when ordering by DisplayName, this sort operation takes >400ms (on my machine, for slow android phones it will take even longer).
I am wondering what percentage of users that use GetSystemTimeZones need the timezones to be sorted on DisplayName, if a majority of users don't need to use DisplayName, I think it would be unfair to make all users who grab GetSystemTimeZones for other properties to need to take this significant perf hit. Loading StandardName and DaylightName together take 1/9th of the time that DisplayName takes overall (based on timings within TryPopulateTimeZoneDisplayNamesFromGlobalizationData). Would it make sense to have a separate function like GetSystemTimeZones + GetSystemTimeZonesSortedByDisplayName?
There was a problem hiding this comment.
This is a breaking change. The behavior is documented.
The collection returned by this method is sorted by UTC offset and, for time zones that have the same UTC offset, by the display name using the current culture.
There was a problem hiding this comment.
Would adding another API for users that want to grab all system time zones (regardless of ordering) but care more about performance make sense?
There was a problem hiding this comment.
Apps usually call GetSystemTimeZones once and not in the startup code paths. I never saw any perf concern with that. Why is the perf concern here? do you have a real app/service suffer from the perf because of that? it should be one call cost and then forget it. by the way, the majority of usage of this API is the apps displaying the list of time zones in the UI allow users to select one. It is reasonable to pay the cost of getting the display name.
There was a problem hiding this comment.
And I don't think we need to expose a new API except if we have evidence there is a perf issue blocking apps/services.
There was a problem hiding this comment.
There was this original issue that this PR aims to remedy dotnet/android#8050.
There was a problem hiding this comment.
I posted a question to the guy who opened the issue there. It is not clear to me why he needs to enumerate all time zones to do time zone conversions.
tarekgh
reviewed
Jul 4, 2023
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Jul 4, 2023
tarekgh
reviewed
Jul 4, 2023
| // Keep window's implementation to populate via constructor | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Now that CreateCustomTimeZone sets the names to string.Empty if they were null, these shouldn't be hit.
There was a problem hiding this comment.
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
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah even though it is a private constructor and isn't referenced in the TimeZoneInfo.*.cs?
ghost
added
the
needs-author-action
ghost
removed
the
needs-author-action
mdh1418
force-pushed
the
android_perf_timezoneinfo
branch
from
e522b25
to
e933ea0
Compare
This was referenced Jul 5, 2023
mdh1418
force-pushed
the
android_perf_timezoneinfo
branch
from
d46701a
to
b453345
Compare
tarekgh
reviewed
Jul 5, 2023
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Jul 5, 2023
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs
Outdated
Show resolved
Hide resolved
tarekgh
reviewed
Jul 5, 2023
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.FullGlobalizationData.Unix.cs
Outdated
Show resolved
Hide resolved
This was referenced Jul 5, 2023
mdh1418
commented
Jul 5, 2023
Comment on lines
+88
to
+90
| timeZone.DisplayName, | ||
| timeZone.StandardName, | ||
| timeZone.DaylightName, |
There was a problem hiding this comment.
Needed to invoke the lazy initialization because _displayName would not be populated otherwise.
mdh1418
force-pushed
the
android_perf_timezoneinfo
branch
from
7bad4cd
to
e60c1a4
Compare
tarekgh
approved these changes
Jul 5, 2023
There was a problem hiding this comment.
LGTM. ensure clean CI and addressing #88368 (review).
Thanks @mdh1418.
mdh1418
force-pushed
the
android_perf_timezoneinfo
branch
from
5fd4e14
to
a47ea8c
Compare
tarekgh
reviewed
Jul 6, 2023
src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.FullGlobalizationData.Unix.cs
Show resolved
Hide resolved
|
Failures were known #88499 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes dotnet/android#8050
The
TimeZoneInfo(byte[] data, string id, bool dstDisabled)constructor on Unix sets internal fields used inDisplayName,StandardName, andDaylightNameby querying the timezone database viaTryPopulateTimeZoneDisplayNamesFromGlobalizationData. For APIs that invoke that constructor for every timezone (>400 timezones on Android) likeGetSystemTimeZones()this operation is costly.On average (on my machine, Mac mini M1, 2020 with 16GB memory running on OS 13.4 using Android Studio Emulator),
TryPopulateTimeZoneDisplayNamesFromGlobalizationDatatakes ~66% (719.31ms) of the timeGetSystemTimeZonestakes (~1.091s). More specifically,GetFullValueForDisplayNameFieldtakes ~93% (669ms) of the time ofTryPopulateTimeZoneDisplayNamesFromGlobalizationData, with the InteropGetDisplayName.Example timing breakdown
This PR looks to improve the performance of APIs invoking a large number of
TimeZoneInfoconstructors by:DisplayName,StandardName, andDaylightNameOrdering timezones by Id inGetSystemTimeZones()to avoid perf hit fromDisplayNamecall.On average (on my machine, Mac mini M1, 2020 with 16GB memory running on OS 13.4 using Android Studio Emulator),
Timing
TimeZoneInfo.GetSystemTimeZones()with the changes in this PR when sorting based onDisplayName:GetSystemTimeZones: ~677msTimeZoneInfo(byte[] data, string id, bool dstDisabled): ~115.92ms