Skip to content
Merged
Changes from 1 commit
Commits
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
Address feedback
  • Loading branch information
mdh1418 authored and github-actions committed Sep 1, 2022
commit f85b518791065f064040a9282b110a2712df16d9
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private static int ParseGMTNumericZone(string name)
{
return new TimeZoneInfo(id, TimeSpan.FromSeconds(0), id, name, name, null, disableDaylightSavingTime:true);
}
if (name[0] == 'G' && name[1] == 'M' && name[2] == 'T')
if (name.Length >= 3 && name[0] == 'G' && name[1] == 'M' && name[2] == 'T')
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this changed?

Copy link
Member

Choose a reason for hiding this comment

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

It can be a perf win if we are able to delay loading ICU until after startup. This change doesn't guarantee that, it just helps.

Copy link
Member

Choose a reason for hiding this comment

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

Why would StartsWith("GMT", StringComparison.Ordinal) load ICU?

Copy link
Member

Choose a reason for hiding this comment

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

Apparently, it's fairly easy to get to

if (!GlobalizationMode.Invariant)
{
if (TryGetAppLocalIcuSwitchValue(out string? icuSuffixAndVersion))
{
LoadAppLocalIcu(icuSuffixAndVersion);
}
else
{
int loaded = LoadICU();
if (loaded == 0)
{
Environment.FailFast(GetIcuLoadFailureMessage());
}
}
}
to trigger the load.

{
return new TimeZoneInfo(id, TimeSpan.FromSeconds(ParseGMTNumericZone(name)), id, name, name, null, disableDaylightSavingTime:true);
}
Expand Down Expand Up @@ -144,13 +144,13 @@ private static TimeZoneInfo GetLocalTimeZoneCore()

private static TimeSpan GetCacheLocalUtcOffset(DateTime dateTime, TimeZoneInfoOptions flags)
{
CachedData cachedData = s_cachedData;
return cachedData.Local.GetUtcOffset(dateTime, flags, cachedData);
CachedData cachedData = s_cachedData;
return cachedData.Local.GetUtcOffset(dateTime, flags, cachedData);
}

private static TimeSpan? _localUtcOffset;
private static object _localUtcOffsetLock = new();
private static Thread? _loadAndroidTZData;
private static bool s_androidTZDataLoaded;
private static object s_localUtcOffsetLock = new();
private static Thread? s_loadAndroidTZData;
// Shortcut for TimeZoneInfo.Local.GetUtcOffset
// On Android, loading AndroidTZData while obtaining cachedData.Local is expensive for startup.
// We introduce a fast result for GetLocalUtcOffset that relies on the date time offset being
Expand All @@ -159,26 +159,33 @@ private static TimeSpan GetCacheLocalUtcOffset(DateTime dateTime, TimeZoneInfoOp
// The fast path is initially used, and we start a background thread to get cachedData.Local
internal static TimeSpan GetLocalUtcOffset(DateTime dateTime, TimeZoneInfoOptions flags)
{
if (_localUtcOffset != null) // The background thread finished, the cache is loaded.
if (s_androidTZDataLoaded) // The background thread finished, the cache is loaded.
{
s_loadAndroidTZData = null; // Ensure thread is cleared when cache is loaded
return GetCacheLocalUtcOffset(dateTime, flags);
}

if (_localUtcOffset == null && _loadAndroidTZData == null) // The cache isn't loaded and no background thread has been created
if (!s_androidTZDataLoaded && s_loadAndroidTZData == null) // The cache isn't loaded and no background thread has been created
{
lock (_localUtcOffsetLock)
lock (s_localUtcOffsetLock)
{
// GetLocalUtcOffset may be called multiple times before a cache is loaded and a background thread is running,
// once the lock is available, check for a cache and background thread.
if (_localUtcOffset != null)
if (s_androidTZDataLoaded)
{
s_loadAndroidTZData = null; // Ensure thread is cleared when cache is loaded
return GetCacheLocalUtcOffset(dateTime, flags);

if (_loadAndroidTZData == null)
}
if (s_loadAndroidTZData == null)
{
_loadAndroidTZData = new Thread(() => {
s_loadAndroidTZData = new Thread(() => {
Thread.Sleep(1000);
_localUtcOffset = GetCacheLocalUtcOffset(dateTime, flags);
CachedData cachedData = s_cachedData;
_ = cachedData.Local;
s_androidTZDataLoaded = true;
});
_loadAndroidTZData.IsBackground = true;
_loadAndroidTZData.Start();
s_loadAndroidTZData.IsBackground = true;
s_loadAndroidTZData.Start();
}
}
}
Expand All @@ -187,7 +194,7 @@ internal static TimeSpan GetLocalUtcOffset(DateTime dateTime, TimeZoneInfoOption
if (localDateTimeOffset == null)
return GetCacheLocalUtcOffset(dateTime, flags); // If no offset property provided through monovm app context, default

long localDateTimeOffsetSeconds = Convert.ToInt32(localDateTimeOffset);
int localDateTimeOffsetSeconds = Convert.ToInt32(localDateTimeOffset);
TimeSpan offset = TimeSpan.FromSeconds(localDateTimeOffsetSeconds);
return offset;
}
Expand Down