[Android][libs] Introduce GetLocalUtcOffset temporary fast result #74459

mdh1418 · 2022-08-23T21:25:49Z

On Android, time zone data is loaded through AndroidTZData, and the time to load significantly impacts startup performance should one call an API reliant on AndroidTzDataInstance in the startup sequence.
e.g. Displaying DateTimeOffset.Now -> TimeZoneInfo.GetLocalUtcOffset -> cachedData.Local -> cachedData.CreateLocal -> GetLocalTimeZone -> GetLocalTimeZoneCore -> GetTimeZone -> AndroidTzDataInstance.GetTimeZoneData

We can leverage Java API to obtain an offset that is good enough for GetLocalUtcOffset and pass that during the mono runtime startup via monovm_initialize or monovm_initialize_preparsed as a "fast path". In the background, we load AndroidTZData, as it is necessary for handling any time zone changes during the app's duration.

Testing through Android Sample App

Console.WriteLine($"The current time is `{DateTimeOffset.Now}`");
Thread.Sleep(1000);
var temp = DateTimeOffset.Now;
Console.WriteLine($"Now the current time is `{temp}`");

The current time is `8/23/2022 4:26:22 PM -04:00`
Now the current time is `8/23/2022 4:26:24 PM -04:00`

speedscope: https://gist.github.com/mdh1418/473d097cbd1d7b1187c9d83ddb8da11b

First call to DateTimeOffset.Now (Fast Path) 145.66ms

Second call to DateTimeOffset.Now (After AndroidTZData loaded) 2.67ms

This PR does the following:

Splits DateTimeOffset.Now into an Android and non Android implementation
Modifies DateTimeOffset.Now on Android (1st call only) to start a background thread to load AndroidTZData and return a fast result passed in from Java side
Introduces System.TimeZoneInfo.LocalDateTimeOffset property to be passed from Java either as an AppContext property or as a part of MonoCoreRuntimeProperties
Avoid possible ICU loading akin to https://github.com/xamarin/java.interop/pull/1001/files

dotnet-issue-labeler · 2022-08-23T21:25:54Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

mdh1418 · 2022-08-23T21:28:06Z

@grendello @lambdageek Would it be preferred to pass the Offset obtained through Java API as an AppContext key/value property or as part of MonoCoreRuntimeProperties? If the latter, how should that be grabbed in the library side?

jonathanpeppers · 2022-08-23T21:34:59Z

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs

+                        _loadAndroidTZData = new Thread(() => {
+                            _localUtcOffset = GetCacheLocalUtcOffset(dateTime, flags);
+                            Thread.Sleep(1000);
+                        });
+                        _loadAndroidTZData.IsBackground = true;
+                        _loadAndroidTZData.Start();


@grendello what are your thoughts here on starting a Thread?

The thread idea was my suggestion and I think it's going to work quite OK, provided it's a bare thread as above and not part of a threadpool or TPL in general. It shouldn't hurt startup performance too much, although what I'd do would be to let it sleep first for 1 second and then start loading the TZ. The idea is that it should start after the main startup sequence of the app, and 1s should be enough time.

If 1s isn't enough time, is this going to make launching the app even slower?

grendello · 2022-08-23T21:46:00Z

@grendello @lambdageek Would it be preferred to pass the Offset obtained through Java API as an AppContext key/value property or as part of MonoCoreRuntimeProperties? If the latter, how should that be grabbed in the library side?

I'd prefer it to be part of MonoCoreRuntimeProperties, it'd be faster to set and we can obtain the offset at startup from our Java code that invokes our netive runtime.

ghost · 2022-08-23T22:03:48Z

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #71004

On Android, time zone data is loaded through AndroidTZData, and the time to load significantly impacts startup performance should one call an API reliant on AndroidTzDataInstance in the startup sequence.
e.g. Displaying DateTimeOffset.Now -> TimeZoneInfo.GetLocalUtcOffset -> cachedData.Local -> cachedData.CreateLocal -> GetLocalTimeZone -> GetLocalTimeZoneCore -> GetTimeZone -> AndroidTzDataInstance.GetTimeZoneData

We can leverage Java API to obtain an offset that is good enough for GetLocalUtcOffset and pass that during the mono runtime startup via monovm_initialize or monovm_initialize_preparsed as a "fast path". In the background, we load AndroidTZData, as it is necessary for handling any time zone changes during the app's duration.

Testing through Android Sample App

Console.WriteLine($"The current time is `{DateTimeOffset.Now}`");
Thread.Sleep(1000);
var temp = DateTimeOffset.Now;
Console.WriteLine($"Now the current time is `{temp}`");

The current time is `8/23/2022 4:26:22 PM -04:00`
Now the current time is `8/23/2022 4:26:24 PM -04:00`

speedscope: https://gist.github.com/mdh1418/473d097cbd1d7b1187c9d83ddb8da11b

First call to DateTimeOffset.Now (Fast Path) 145.66ms

Second call to DateTimeOffset.Now (After AndroidTZData loaded) 2.67ms

This PR does the following:

Splits GetLocalUtcOffset across TimeZoneInfo.*.cs
Modifies GetLocalUtcOffset on Android (1st call only) to start a background thread to load AndroidTZData and return a fast result passed in from Java side
Introduces LOCAL_DATE_TIME_OFFSET property to be passed from Java either as an AppContext property or as a part of MonoCoreRuntimeProperties
Avoid possible ICU loading akin to https://github.com/xamarin/java.interop/pull/1001/files

Author:	mdh1418
Assignees:	mdh1418
Labels:	`area-System.Runtime`, `os-android`
Milestone:	-

lambdageek · 2022-08-24T14:00:10Z

@grendello @lambdageek Would it be preferred to pass the Offset obtained through Java API as an AppContext key/value property or as part of MonoCoreRuntimeProperties? If the latter, how should that be grabbed in the library side?

I'd prefer it to be part of MonoCoreRuntimeProperties, it'd be faster to set and we can obtain the offset at startup from our Java code that invokes our netive runtime.

I think it shouldn't be in MonoCoreRuntimeProperties 😁

it's faster to set in XA, but in mono we will still have to convert it to a string property in order to populate the managed AppContext. So we're just pushing the cost down one layer

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs

grendello · 2022-08-24T14:25:51Z

@grendello @lambdageek Would it be preferred to pass the Offset obtained through Java API as an AppContext key/value property or as part of MonoCoreRuntimeProperties? If the latter, how should that be grabbed in the library side?

I'd prefer it to be part of MonoCoreRuntimeProperties, it'd be faster to set and we can obtain the offset at startup from our Java code that invokes our netive runtime.

I think it shouldn't be in MonoCoreRuntimeProperties grin

it's faster to set in XA, but in mono we will still have to convert it to a string property in order to populate the managed AppContext. So we're just pushing the cost down one layer

That's the whole point, though :) To make the startup sequence of XA faster - what happens after the app is running can be slower without hurting the perceived app performance.

lambdageek · 2022-08-24T14:33:23Z

That's the whole point, though :) To make the startup sequence of XA faster - what happens after the app is running can be slower without hurting the perceived app performance.

This won't make the startup faster. We need to populate the AppContext during startup: we call mono_runtime_install_appctx_properties from mono_runtime_init_checked which is called from mono_jit_init_version.

src/native/public/mono/jit/details/mono-private-unstable-types.h

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs

Leverage a boolean flag instead of && Start the background thread to avoid deadlocking

mdh1418 · 2022-09-01T04:23:17Z

@grendello, I tweaked a little bit to get the thread starting after the lock to work. Without a condition, it could crash with some test like

Console.WriteLine($"The current time is `{DateTimeOffset.Now}`");
var test = DateTimeOffset.Now;
Console.WriteLine($"Now the current time is `{test}`");
Thread.Sleep(1000);
var hwang = DateTimeOffset.Now;
Console.WriteLine($"Now the current time is `{hwang}`");
Thread.Sleep(500);
var temp = DateTimeOffset.Now;
Console.WriteLine($"Now the current time is `{temp}`");

mdh1418 · 2022-09-01T04:23:31Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-09-01T04:23:44Z

Azure Pipelines failed to run 1 pipeline(s).

…_improvement

mdh1418 · 2022-09-01T14:44:41Z

/azp run runtime-extra platforms

azure-pipelines · 2022-09-01T14:44:49Z

No pipelines are associated with this pull request.

mdh1418 · 2022-09-01T14:45:10Z

Checks didn't run because of the migration, should run this time after merging main

mdh1418 · 2022-09-01T15:48:04Z

/azp run runtime-extra platforms

azure-pipelines · 2022-09-01T15:48:10Z

No pipelines are associated with this pull request.

mdh1418 · 2022-09-01T20:47:46Z

#74952
#71887
#74896

The Android libraries tests didnt have any failures in a previous CI build, and the commits since then were only small performance gain modifications.

mdh1418 · 2022-09-01T20:49:18Z

/backport to release/7.0

github-actions · 2022-09-01T20:49:32Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2974849069

github-actions · 2022-09-01T20:51:51Z

@mdh1418 an error occurred while backporting to release/7.0, please check the run log for details!

Error: @mdh1418 is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=mdh1418

mdh1418 · 2022-09-01T20:55:45Z

/backport to release/7.0

github-actions · 2022-09-01T20:55:58Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2974882914

…fast result (#74965) Backport of #74459 to release/7.0 Introduces fast path for DateTimeOffset.Now on Android that allows us to load tzdata in the background. This improves performance from around 227ms to 21.81ms.

Context: dotnet/runtime#74459 Context: dotnet/runtime#71004 Context: dotnet/runtime#54845 dotnet/runtime#54845 added support TimeZoneInfo support for Android. This was found to slow down initial use of `DateTimeOffset.Now` by ~277ms (dotnet/runtime#17004). dotnet/runtime#74459 started reducing this startup overhead by adding support for a new `System.TimeZoneInfo.LocalDateTimeOffset` property on `AppContext`: if `System.TimeZoneInfo.LocalDateTimeOffset` is set, it is used as the initial local DateTime Offset value. Update `mono.android.MonoPackageManager.LoadApplication()` and `libmono-android*.so` so that the `System.TimeZoneInfo.LocalDateTimeOffset` AppContext property is set during process startup. This can reduce `DateTimeOffset.Now` startup overhead from ~277ms to ~50ms. Note: `System.TimeZoneInfo.LocalDateTimeOffset` is fastest on API-26+ (Android 8.0+) targets, as it uses [`java.time.ZoneOffset.getTotalSeconds()`][0]. Note: `$(AndroidJavaRuntimeApiLevel)` is updated to API-26 so that `ZoneOffset.getTotalSeconds()` can be used. Care should be taken within `src/java-runtime` to ensure that Android API use is appropriately protected behind runtime version checks. [0]: https://developer.android.com/reference/kotlin/java/time/ZoneOffset#gettotalseconds

mdh1418 requested review from marek-safar, lambdageek, vargaz and SamMonoRT as code owners August 23, 2022 21:25

ghost assigned mdh1418 Aug 23, 2022

mdh1418 requested review from grendello and jonathanpeppers August 23, 2022 21:26

mdh1418 added 7 commits August 23, 2022 17:31

Avoid ICU loading via StringComparison.Ordinal

a3f0e9f

Pass OffsetDateTime and monotonic clock through monovm

a44084a

WIP Leverage monovm_initialize_preparsed

1edaeee

Initial perf improvement implementation

0712f57

Cleanup GetLocalUtcOffset

96bf37f

Remove Kernel Monotonic Time app context key

582c359

Cleanup GetLocalUtcOffset

9a2a8a2

mdh1418 force-pushed the android_tzdata_perf_improvement branch from 7c6f200 to 9a2a8a2 Compare August 23, 2022 21:32

jonathanpeppers reviewed Aug 23, 2022

View reviewed changes

danmoseley added area-System.Runtime os-android labels Aug 23, 2022

karelz mentioned this pull request Aug 24, 2022

mono minijit runtime tests failing with coreclr_initialize failed - Error: 0x80004005 #72114

Closed

jonathanpeppers reviewed Aug 24, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated Show resolved Hide resolved

Sleep background thread first, then start loading AndroidTZData

f352b3a

lambdageek reviewed Aug 24, 2022

View reviewed changes

src/native/public/mono/jit/details/mono-private-unstable-types.h Outdated Show resolved Hide resolved

lambdageek reviewed Aug 24, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.Unix.Android.cs Outdated Show resolved Hide resolved

Introduce small performance gains

ef7729e

Leverage a boolean flag instead of && Start the background thread to avoid deadlocking

mdh1418 force-pushed the android_tzdata_perf_improvement branch from c03c802 to ef7729e Compare September 1, 2022 04:18

Merge remote-tracking branch 'upstream/main' into android_tzdata_perf…

55fcccc

…_improvement

Bump boolean flag outside of background thread body

1cfc352

mdh1418 merged commit 81f8e64 into dotnet:main Sep 1, 2022

marek-safar deleted the android_tzdata_perf_improvement branch September 1, 2022 20:48

github-actions bot mentioned this pull request Sep 1, 2022

[release/7.0] [Android][libs] Introduce DateTimeOffset.Now temporary fast result #74965

Merged

mdh1418 mentioned this pull request Sep 2, 2022

Add local date time offset monovm prop dotnet/android#7331

Merged

simonrozsival mentioned this pull request Sep 7, 2022

[Android] Avoid using OffsetDateTime on APIs 25 and lower #75196

Merged

ghost locked as resolved and limited conversation to collaborators Oct 2, 2022

jeffhandley added area-System.DateTime and removed area-System.Runtime labels Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android][libs] Introduce GetLocalUtcOffset temporary fast result #74459

[Android][libs] Introduce GetLocalUtcOffset temporary fast result #74459

mdh1418 commented Aug 23, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Aug 23, 2022

mdh1418 commented Aug 23, 2022

jonathanpeppers Aug 23, 2022

grendello Aug 23, 2022

eerhardt Aug 25, 2022

grendello commented Aug 23, 2022

ghost commented Aug 23, 2022

Testing through Android Sample App

First call to DateTimeOffset.Now (Fast Path) 145.66ms

Second call to DateTimeOffset.Now (After AndroidTZData loaded) 2.67ms

This PR does the following:

lambdageek commented Aug 24, 2022

grendello commented Aug 24, 2022

lambdageek commented Aug 24, 2022 •

edited

Loading

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

[Android][libs] Introduce GetLocalUtcOffset temporary fast result #74459

[Android][libs] Introduce GetLocalUtcOffset temporary fast result #74459

Conversation

mdh1418 commented Aug 23, 2022 • edited Loading

Testing through Android Sample App

First call to DateTimeOffset.Now (Fast Path) 145.66ms

Second call to DateTimeOffset.Now (After AndroidTZData loaded) 2.67ms

This PR does the following:

dotnet-issue-labeler bot commented Aug 23, 2022

mdh1418 commented Aug 23, 2022

jonathanpeppers Aug 23, 2022

Choose a reason for hiding this comment

grendello Aug 23, 2022

Choose a reason for hiding this comment

eerhardt Aug 25, 2022

Choose a reason for hiding this comment

grendello commented Aug 23, 2022

ghost commented Aug 23, 2022

Testing through Android Sample App

First call to DateTimeOffset.Now (Fast Path) 145.66ms

Second call to DateTimeOffset.Now (After AndroidTZData loaded) 2.67ms

This PR does the following:

lambdageek commented Aug 24, 2022

grendello commented Aug 24, 2022

lambdageek commented Aug 24, 2022 • edited Loading

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

mdh1418 commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

mdh1418 commented Aug 23, 2022 •

edited

Loading

lambdageek commented Aug 24, 2022 •

edited

Loading