Add local date time offset monovm prop #7331

mdh1418 · 2022-09-02T15:09:45Z

Follows up dotnet/runtime#74459 for dotnet/runtime#71004

On dotnet/runtime, DateTimeOffset.Now is now updated to leverage an App Context's property as a fast path result if provided. In order to improve startup performance for applications that rely on DateTimeOffset.Now during startup will need to set System.TimeZoneInfo.LocalDateTimeOffset as the appropriate value.
The original issue reported a perf timing of 277ms, and after dotnet/runtime#74459, has an average perf timing of 21.818ms.

Testing

Created an Android app
Added _ = DateTimeOffset.Now; to OnCreate

The timing of DateTimeOffset.Now with dotnet-trace was 1.18ms + 17.67ms = 18.85ms

This PR does the following:

Bump AndroidJavaRuntimeApiLevel to 26 to allow for java.time.OffsetDateTime and java.time.ZoneOffset.
Adds the System.TimeZoneInfo.LocalDateTimeOffset property to MonoVMProperties passed into the mono runtime to enable DateTimeOffset.Now fast path.

src/java-runtime/java/mono/android/MonoPackageManager.java

grendello · 2022-09-02T15:23:58Z

src/monodroid/jni/monovm-properties.cc

@@ -5,9 +5,11 @@ using namespace xamarin::android::internal;
 MonoVMProperties::property_array MonoVMProperties::_property_keys {
 	RUNTIME_IDENTIFIER_KEY,
 	APP_CONTEXT_BASE_DIRECTORY_KEY,
+	LOCAL_DATE_TIME_OFFSET_KEY,


I hope that one day we can replace this dynamically allocated property with a field in the MonoCoreRuntimeProperties struct... It would make the code even faster.

How much are we saving if we could leverage MonoCoreRuntimeProperties struct over adding another property key/value pair?

There isn't any convenient way to put any of the MonoCoreRuntimeProperties into AppContext on the runtime side. This is because none of the properties in MonoCoreRuntimeProperties need to make it to the managed side. Another thing we would have to do is maybe differentiate between properties that need to make it to managed side or not to then specifically add into the AppContext.

Since populating AppContext is necessary for startup to finish, would the savings done by leveraging MonoCoreRuntimeProperties over adding a new key/value pair here be greater than the cost of needing to convert it to a string property in the runtime and any possible filtering before its added to the App Context?

The biggest savings on XA side come from not having to convert an integer into a string and not duplicating that string with strdup. These obviously aren't big savings, but it's something that needlessly consumes memory and processor time (since that string will be parsed back into integer a short while later). I wonder if AppContext can be populated lazily? That is, could there be code to retrieve the value(s) from MonoCoreRuntimeProperties on demand (using an icall or p/invoke) instead of pre-populating it?

Also, looking at AppContext docs, its GetData(string) method returns an object? - do we absolutely have to convert the integer retrieved here to string, then use Convert.ToInt32 after retrieving it from GetData?
This just looks like a wasteful way of doing it... After all, we're running in the same process, so we can access variables (sort of) directly without having to proxy them via strings. There was a similar issue sometime ago with p/invoke overrides, where initially we had to convert a pointer to a hexadecimal string, pass it to the runtime which would then parse that string and cast it back into a pointer...

On desktop these string operations may not matter, on mobile it's an entirely different matter - every nanosecond saved counts.

src/monodroid/jni/monovm-properties.hh

steveisok · 2022-09-02T17:57:07Z

Configuration.props

@@ -25,7 +25,7 @@
    <!-- Should correspond to the first value from `$(API_LEVELS)` in `build-tools/api-xml-adjuster/Makefile` -->
    <AndroidFirstFrameworkVersion Condition="'$(AndroidFirstFrameworkVersion)' == ''">v4.4</AndroidFirstFrameworkVersion>
    <AndroidFirstApiLevel Condition="'$(AndroidFirstApiLevel)' == ''">19</AndroidFirstApiLevel>
-    <AndroidJavaRuntimeApiLevel Condition="'$(AndroidJavaRuntimeApiLevel)' == ''">21</AndroidJavaRuntimeApiLevel>
+    <AndroidJavaRuntimeApiLevel Condition="'$(AndroidJavaRuntimeApiLevel)' == ''">26</AndroidJavaRuntimeApiLevel>


@grendello Is this api level bump ok?

Yes, this is just for build time, so that the code which uses API 26 interfaces can be compiled. At runtime a check is made whether the API in question can be called. Bumping it here doesn't prevent running on older devices.

jonpryor · 2022-09-07T16:18:35Z

src/java-runtime/java/mono/android/MonoPackageManager.java

+				int localDateTimeOffset;
+
+				if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) {
+					localDateTimeOffset = OffsetDateTime.now().getOffset().getTotalSeconds();


Have we tested this on an API-21 device or emulator? We may need to move this expression into a separate method; I am not sure.

Just tried it on API 21 emulator

, I dont have an API 21 device. It looks like it works fine? Unless I did something wrong, why might it not work on API-21 device/emulator if we have the runtime API check? How would moving it to a separate method remedy this?

mdh1418 · 2022-09-14T18:31:51Z

/azp run

mdh1418 · 2022-09-14T18:32:12Z

/azp run Xamarin.Android-PR

azure-pipelines · 2022-09-14T18:32:26Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2022-09-14T18:32:32Z

Azure Pipelines successfully started running 1 pipeline(s).

jonathanpeppers · 2022-09-16T13:41:43Z

src/java-runtime/java/mono/android/Runtime.java

-	public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables);
+	public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, int localDateTimeOffset, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables);


I think adding a parameter to init here is breaking the Designer tests -- they call this API directly.

Can we just pass a 0 or some default value through? I don't think it matters if the behavior for DateTime.Now is off a little bit when rendering layouts in the designer.

@jonathanpeppers Ah, I didnt realize that was the root of the failures on the unit tests. I'm not actually sure what the corresponding designer tests are, but do any actually leverage DateTimeOffset.Now?

Whats the easiest way to confirm that adding a 0 or default parameter on the designer's init call will fix the tests? If it does solve the problem, then would the correct order be to merge this pr knowing that it will break tests and then merge a pr on designer to add the parameter?

Or would it be better to make another init like having both

public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables); public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, int localDateTimeOffset, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables);

I would guess that no designer tests use DateTimeOffset.Now. It doesn't seem like something you would do in a custom UserControl-like scenario.

But they do call this init() method with a known set of parameters -- we can't change that or the designer will crash on startup.

jonathanpeppers

The designer tests look to be passing now, should everyone else take another review?

mdh1418 · 2022-09-27T14:47:31Z

Yes, thanks for helping me figure out that init was causing a problem! I realized the only reason I had changed init was to pass in the localDateTimeOffset into Java_mono_android_Runtime_initInternal, but I think as you mentioned, that can just be defaulted to 0 for anything calling init.

@grendello @jonpryor

jonpryor · 2022-09-27T17:49:17Z

[monodroid, java-runtime] Add local date time offset monovm prop (#7331)

Context: https://github.com/dotnet/runtime/pull/74459
Context: https://github.com/dotnet/runtime/issues/71004
Context: https://github.com/dotnet/runtime/pull/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

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

* main: Localized file check-in by OneLocBuild Task: Build definition ID 11410: Build ID 6746307 (dotnet#7414) [Mono.Android] Add [Flags] to Android.Nfc.NfcReaderFlags. (dotnet#7410) [monodroid, java-runtime] Add local date time offset monovm prop (dotnet#7331)

mdh1418 added 2 commits September 2, 2022 10:51

Bump Java API level to leverage faster Date Time Offset API

e61dc39

Add LocalDateTimeOffset to monovm_props

f595bf4

mdh1418 requested review from jonpryor and grendello as code owners September 2, 2022 15:09

grendello requested changes Sep 2, 2022

View reviewed changes

steveisok reviewed Sep 2, 2022

View reviewed changes

Address feedback

874d0c4

grendello approved these changes Sep 2, 2022

View reviewed changes

jonpryor reviewed Sep 7, 2022

View reviewed changes

This was referenced Sep 14, 2022

Xamarin.AndroidDesigner.Tests failing with The session is not in a state where custom controls can be used #7375

Closed

Xamarin.Android.Build.Tests macOS DotNetBuildLibrary(False,True) failed #7376

Closed

jonathanpeppers requested changes Sep 16, 2022

View reviewed changes

WIP

5a5af05

jonathanpeppers approved these changes Sep 27, 2022

View reviewed changes

grendello approved these changes Sep 27, 2022

View reviewed changes

jonpryor merged commit 7739a5c into dotnet:main Sep 27, 2022

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add local date time offset monovm prop #7331

Add local date time offset monovm prop #7331

mdh1418 commented Sep 2, 2022

grendello Sep 2, 2022

mdh1418 Sep 2, 2022

grendello Sep 2, 2022

steveisok Sep 2, 2022

grendello Sep 2, 2022

jonpryor Sep 7, 2022

mdh1418 Sep 14, 2022

mdh1418 commented Sep 14, 2022

mdh1418 commented Sep 14, 2022

azure-pipelines bot commented Sep 14, 2022

azure-pipelines bot commented Sep 14, 2022

jonathanpeppers Sep 16, 2022

mdh1418 Sep 21, 2022

jonathanpeppers Sep 21, 2022

jonathanpeppers left a comment

mdh1418 commented Sep 27, 2022

jonpryor commented Sep 27, 2022 •

edited

Loading

		public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables);
		public static native void init (String lang, String[] runtimeApks, String runtimeDataDir, String[] appDirs, int localDateTimeOffset, ClassLoader loader, String[] externalStorageDirs, String[] assemblies, String packageName, int apiLevel, String[] environmentVariables);

Add local date time offset monovm prop #7331

Add local date time offset monovm prop #7331

Conversation

mdh1418 commented Sep 2, 2022

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdh1418 commented Sep 14, 2022

mdh1418 commented Sep 14, 2022

azure-pipelines bot commented Sep 14, 2022

azure-pipelines bot commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanpeppers left a comment

Choose a reason for hiding this comment

mdh1418 commented Sep 27, 2022

jonpryor commented Sep 27, 2022 • edited Loading

jonpryor commented Sep 27, 2022 •

edited

Loading