Skip to content

Add local date time offset monovm prop #7331

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

Merged
merged 4 commits into from
Sep 27, 2022
Merged

Add local date time offset monovm prop #7331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e61dc39
Bump Java API level to leverage faster Date Time Offset API
mdh1418 Sep 2, 2022
f595bf4
Add LocalDateTimeOffset to monovm_props
mdh1418 Sep 2, 2022
874d0c4
Address feedback
mdh1418 Sep 2, 2022
5a5af05
WIP
mdh1418 Sep 26, 2022
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
2 changes: 1 addition & 1 deletion Configuration.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

@steveisok steveisok Sep 2, 2022

Choose a reason for hiding this comment

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

@grendello Is this api level bump ok?

Copy link
Contributor

@grendello grendello Sep 2, 2022

Choose a reason for hiding this comment

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

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.

<!-- The min API level supported by Microsoft.Android.Sdk, should refactor/remove when this value is the same as $(AndroidFirstApiLevel) -->
<AndroidMinimumDotNetApiLevel Condition="'$(AndroidMinimumDotNetApiLevel)' == ''">21</AndroidMinimumDotNetApiLevel>
<AndroidFirstPlatformId Condition="'$(AndroidFirstPlatformId)' == ''">$(AndroidFirstApiLevel)</AndroidFirstPlatformId>
Expand Down
12 changes: 12 additions & 0 deletions src/java-runtime/java/mono/android/MonoPackageManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

import java.io.*;
import java.lang.String;
import java.time.OffsetDateTime;
import java.time.ZoneOffset;
import java.util.Calendar;
import java.util.Locale;
import java.util.HashSet;
import java.util.zip.*;
Expand Down Expand Up @@ -42,6 +45,14 @@ public static void LoadApplication (Context context, ApplicationInfo runtimePack
String dataDir = getNativeLibraryPath (context);
ClassLoader loader = context.getClassLoader ();
String runtimeDir = getNativeLibraryPath (runtimePackage);
int localDateTimeOffset;

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

@jonpryor jonpryor Sep 7, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@mdh1418 mdh1418 Sep 14, 2022

Choose a reason for hiding this comment

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

Just tried it on API 21 emulator
Screen Shot 2022-09-14 at 1 42 58 PM
, 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?

}
else {
localDateTimeOffset = (Calendar.getInstance ().get (Calendar.ZONE_OFFSET) + Calendar.getInstance ().get (Calendar.DST_OFFSET)) / 1000;
}

//
// Should the order change here, src/monodroid/jni/SharedConstants.hh must be updated accordingly
Expand Down Expand Up @@ -106,6 +117,7 @@ public static void LoadApplication (Context context, ApplicationInfo runtimePack
apks,
runtimeDir,
appDirs,
localDateTimeOffset,
loader,
MonoPackageManager_Resources.Assemblies,
Build.VERSION.SDK_INT,
Expand Down
1 change: 1 addition & 0 deletions src/java-runtime/java/mono/android/Runtime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public static native void initInternal (
String[] runtimeApks,
String runtimeDataDir,
String[] appDirs,
int localDateTimeOffset,
ClassLoader loader,
String[] assemblies,
int apiLevel,
Expand Down
4 changes: 2 additions & 2 deletions src/monodroid/jni/mono_android_Runtime.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/monodroid/jni/monodroid-glue-internal.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ namespace xamarin::android::internal
public:
void Java_mono_android_Runtime_register (JNIEnv *env, jstring managedType, jclass nativeClass, jstring methods);
void Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jobject loader,
jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset,
jobject loader, jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jboolean haveSplitApks);
#if !defined (ANDROID)
jint Java_mono_android_Runtime_createNewContextWithData (JNIEnv *env, jclass klass, jobjectArray runtimeApksJava, jobjectArray assembliesJava,
Expand Down
10 changes: 6 additions & 4 deletions src/monodroid/jni/monodroid-glue.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2152,8 +2152,8 @@ MonodroidRuntime::install_logging_handlers ()

inline void
MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jobject loader,
jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset,
jobject loader, jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jboolean haveSplitApks)
{
char *mono_log_mask_raw = nullptr;
Expand All @@ -2180,7 +2180,7 @@ MonodroidRuntime::Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass kl
mono_opt_aot_lazy_assembly_load = application_config.aot_lazy_load ? TRUE : FALSE;

{
MonoVMProperties monovm_props { home };
MonoVMProperties monovm_props { home, localDateTimeOffset };

// NOTE: the `const_cast` breaks the contract made to MonoVMProperties that the arrays it returns won't be
// modified, but it's "ok" since Mono doesn't modify them and by using `const char* const*` in MonoVMProperties
Expand Down Expand Up @@ -2452,6 +2452,7 @@ Java_mono_android_Runtime_init (JNIEnv *env, jclass klass, jstring lang, jobject
runtimeApksJava,
runtimeNativeLibDir,
appDirs,
0,
loader,
assembliesJava,
apiLevel,
Expand All @@ -2462,7 +2463,7 @@ Java_mono_android_Runtime_init (JNIEnv *env, jclass klass, jstring lang, jobject

JNIEXPORT void JNICALL
Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang, jobjectArray runtimeApksJava,
jstring runtimeNativeLibDir, jobjectArray appDirs, jobject loader,
jstring runtimeNativeLibDir, jobjectArray appDirs, jint localDateTimeOffset, jobject loader,
jobjectArray assembliesJava, jint apiLevel, jboolean isEmulator,
jboolean haveSplitApks)
{
Expand All @@ -2473,6 +2474,7 @@ Java_mono_android_Runtime_initInternal (JNIEnv *env, jclass klass, jstring lang,
runtimeApksJava,
runtimeNativeLibDir,
appDirs,
localDateTimeOffset,
loader,
assembliesJava,
apiLevel,
Expand Down
2 changes: 2 additions & 0 deletions src/monodroid/jni/monovm-properties.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@grendello grendello Sep 2, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@mdh1418 mdh1418 Sep 2, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@grendello grendello Sep 2, 2022

Choose a reason for hiding this comment

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

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.

};

MonoVMProperties::property_array MonoVMProperties::_property_values {
SharedConstants::runtime_identifier,
nullptr,
nullptr,
};
11 changes: 9 additions & 2 deletions src/monodroid/jni/monovm-properties.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,30 @@ namespace xamarin::android::internal
{
class MonoVMProperties final
{
constexpr static size_t PROPERTY_COUNT = 2;
constexpr static size_t PROPERTY_COUNT = 3;

constexpr static char RUNTIME_IDENTIFIER_KEY[] = "RUNTIME_IDENTIFIER";
constexpr static size_t RUNTIME_IDENTIFIER_INDEX = 0;

constexpr static char APP_CONTEXT_BASE_DIRECTORY_KEY[] = "APP_CONTEXT_BASE_DIRECTORY";
constexpr static size_t APP_CONTEXT_BASE_DIRECTORY_INDEX = 1;

constexpr static char LOCAL_DATE_TIME_OFFSET_KEY[] = "System.TimeZoneInfo.LocalDateTimeOffset";
constexpr static size_t LOCAL_DATE_TIME_OFFSET_INDEX = 2;

using property_array = const char*[PROPERTY_COUNT];

public:
explicit MonoVMProperties (jstring_wrapper& filesDir)
explicit MonoVMProperties (jstring_wrapper& filesDir, jint localDateTimeOffset)
{
static_assert (PROPERTY_COUNT == N_PROPERTY_KEYS);
static_assert (PROPERTY_COUNT == N_PROPERTY_VALUES);

_property_values[APP_CONTEXT_BASE_DIRECTORY_INDEX] = strdup (filesDir.get_cstr ());

static_local_string<32> localDateTimeOffsetBuffer;
localDateTimeOffsetBuffer.append (localDateTimeOffset);
_property_values[LOCAL_DATE_TIME_OFFSET_INDEX] = strdup (localDateTimeOffsetBuffer.get ());
}

constexpr int property_count () const
Expand Down