[Experimental] Add alternative way of resolving UCO function pointers for Marshal Methods #9805
Conversation
simonrozsival
requested review from
grendello,
dellis1972,
jonathanpeppers and
jonpryor
as code owners
simonrozsival
force-pushed
the
dev/simonrozsival/managed-get-function-pointer
branch
from
0be14c1 to
408d8f6
Compare
| if (!EnableManagedMarshalMethodsLookup) { | ||
| RewriteMarshalMethods (state, brokenExceptionTransitionsEnabled); | ||
| state.Classifier.AddSpecialCaseMethods (); | ||
| } else { | ||
| // We need to run `AddSpecialCaseMethods` before `RewriteMarshalMethods` so that we can see the special case | ||
| // methods (such as TypeManager.n_Activate_mm) when generating the managed lookup tables. | ||
| state.Classifier.AddSpecialCaseMethods (); | ||
| state.ManagedMarshalMethodsLookupInfo = new ManagedMarshalMethodsLookupInfo (Log); | ||
| RewriteMarshalMethods (state, brokenExceptionTransitionsEnabled); | ||
| } |
There was a problem hiding this comment.
I did not observe any functional changes when swapping the order of AddSpecialCaseMethods and RewriteMarshalMethods. I also don't see a reason that the code in AddSpecialCaseMethods could not run before RewriteMarshalMethods. Nevertheless, I thought it might not be a good idea to change the existing behavior and only change the order when this (experimental) feature is enabled.
| @@ -103,6 +105,15 @@ public void Rewrite (bool brokenExceptionTransitions) | |||
| } | |||
| } | |||
|
|
|||
| if (managedMarshalMethodsLookupInfo is not null) { | |||
| // TODO the code should probably go to different assemblies than Mono.Android (to avoid recursive dependencies) | |||
| var rootAssembly = resolver.Resolve ("Mono.Android") ?? throw new InvalidOperationException ($"[{targetArch}] Internal error: unable to load the Mono.Android assembly"); | |||
There was a problem hiding this comment.
Having the root lookup table in Mono.Android might be weird - the assembly will now reference every other assembly in the project and many of those other assemblies will reference Mono.Android back. As @jonathanpeppers suggested, it might be a better idea to move this to a separate standalone assembly which doesn't reference it back. I think that won't be a trivial change, so I'm leaving this change for later and I would like to receive feedback on the rest of the PR first.
There was a problem hiding this comment.
It might be weird, I'm inclined to agree with @jonathanpeppers, but this idea requires that ManagedMarshalMethodsLookupTable.GetFunctionPointer be accessible from JNIEnvInit.Initialize(). It would thus need to be in either Mono.Android.dll itself or a dependency we can reasonably update, which would be Mono.Android.Runtime.dll.
Neither are "great".
The other alternative is to follow the pattern of 70bd636 / #9760:
- Introduce a
Microsoft.Android.Runtime.CoreCLR.dll, which itself contains the CoreCLR version ofJNIEnvInit.Initialize(). (Probably a some initialization method which chains toJNIEnvInit.InitializeJniRuntime().) Microsoft.Android.Runtime.CoreCLR.dllhas all the "glue": it references all the assemblies (post linking), contains theJava.Interop.ManagedMarshalMethodsLookupTabletype andManagedMarshalMethodsLookupTable.GetFunctionPointer()method, etc., etc.
I like this idea, but I'm not sure it's really any different than what's done here: throw everything into Mono.Android.dll.
There was a problem hiding this comment.
I like the idea of separating the startup logic into an assembly that isn't referenced by anything else in the solution. On the other hand, the code in this PR works as is. I suggest opening an issue to revisit this after #9572 is merged to avoid complex merge conflicts.
simonrozsival
changed the title
|
@simonrozsival for |
|
@grendello could you please clarify what would be in the array and what would the sorting key be? do you imagine having a The only thing I'd like to avoid is changing the signature of |
|
@simonrozsival a trick I use in a few places is that I have an array of ints (say, hashes) which I search through using binary search and, if a match is found, I get an index of this match. The index then is used to access another array with a more complex structure, that is the entry which contains the information I need. You could use the same approach, it works surprisingly well. |
There was a problem hiding this comment.
There must be some tests that assert against application_config:
CheckMonoComponentsMask(False,False,True,1)
Invalid 'application_config' field count in environment file 'D:\a_work\1\a\TestRelease\02-18_11.13.43\temp\CheckMonoComponentsMaskFalseFalseTrue1\obj\Debug\android\environment.armeabi-v7a.ll'
Expected: 26
But was: 27
They probably just need to be updated.
The benefit of the switch statement is that there's no init cost - with an array, something has to fill the array. With code, there's no such need, the code is just there. With R2R we can AOT this code and thus there's no startup cost. |
|
Re #1 name collision problem - if we're generating this method in IL (not C#) we can produce so called "unspeakable" name, that is an identifier which is invalid in C#. Roslyn does this for its generated code - the character |
| @@ -34,6 +34,7 @@ | |||
| <type fullname="Android.Runtime.XmlReaderPullParser" /> | |||
| --> | |||
| <type fullname="Java.Interop.TypeManager*JavaTypeManager" /> | |||
| <type fullname="Java.Interop.ManagedMarshalMethodsLookupTable" preserve="methods" /> | |||
There was a problem hiding this comment.
Our app size regression tests passed, so this is probably ok. Does the new method remain in Mono now?
There was a problem hiding this comment.
It does. Currently I think this is the only way to implement it, since this class and its UCO methods are only referenced from the C++ code using mono embedding APIs. I don't think it is a big deal because this class is just a tiny stub when we don't generate the additional IL.
I am looking for a better way to implement this at the moment. I was thinking about doing the following:
- Drop the code I added to
src/native/mono/monodroid/xamarin-android-app-context.cc - Add a (proper) feature switch that corresponds to the MSBuild property
- Extend
JNIEnvInit.Initialize- Return the UCO fnptr for the
ManagedMarshalMethodsLookupTable.GetFunctionPointermethod if the flag is enabled - likely piggyback onJnienvInitializeArgs* argsand add an "out" property
- Return the UCO fnptr for the
- If we got the function pointer back from
JNIEnvInit.Initialize, pass that down toxamarin_app_init, otherwise use the C++ implementation
This would be trimmable and it would allow us to drop that ugly piece code from xamarin-android-app-context.cc.
The only problem I see is that we're calling xamarin_app_init already in MonodroidRuntime::mono_runtime_init, before we can even call into managed code. I wasn't able to find a case when the special get_function_pointer_at_startup method we use at this stage of startup is used, but I assume this coveres some edge case and it shouldn't be ignored. @grendello do you know in which scenario we need the get_function_pointer_at_startup?
There was a problem hiding this comment.
get_function_pointer_at_startup is a slight perf enhancement where it doesn't take a lock (actually it's an atomic) when storing function pointer in the cache. We can do that in MonoVM because we know there's just a single thread used while we're initializing the application. Atomic writes can be somewhat costly on some devices, thus the approach.
There was a problem hiding this comment.
Extend JNIEnvInit.Initialize
Return the UCO fnptr for the ManagedMarshalMethodsLookupTable.GetFunctionPointer method if the flag is enabled - likely piggyback on JnienvInitializeArgs* args and add an "out" property
+1 on this
There was a problem hiding this comment.
@simonrozsival if you manage to go down that road, we should also consider fetching RegisterJniNatives method pointer the same way:
There was a problem hiding this comment.
@grendello I looked at the code and I think I understand what it does. When I added logging to get_function_pointer_at_startup and get_function_pointer_at_runtime, I was only seeing logs from get_function_pointer_at_runtime when running a .NET MAUI app. Is there some special feature I would need to use in my app that would require fetching a fnptr at startup (opening the app using a deep link? or via some shortcut? widgets?).
| @@ -1332,5 +1332,24 @@ public void NativeAOTSample () | |||
| throw; | |||
| } | |||
| } | |||
|
|
|||
| [Test] | |||
| public void AppStartsWithManagedMarshalMethodsLookupEnabled () | |||
There was a problem hiding this comment.
This new test is running successfully:
|
Really nice work, well done! I have only two minor comments:
|
|
@ivanpovazan You could use https://github.com/grendello/XAPerfTestRunner to run a sample app with both this PR and
|
src/Mono.Android/Java.Interop/ManagedMarshalMethodsLookupTable.cs
Outdated
Show resolved
Hide resolved
| @@ -132,3 +132,48 @@ MonodroidRuntime::get_function_pointer_at_runtime (uint32_t mono_image_index, ui | |||
| { | |||
| get_function_pointer<true> (mono_image_index, class_index, method_token, target_ptr); | |||
| } | |||
|
|
|||
| get_function_pointer_fn | |||
There was a problem hiding this comment.
Given the intent to have something that works with CoreCLR, which doesn't have an embedding API, the existence of this method at all is slightly concerning.
Sketching a bit, what seems like it should work and would allow removing this method and related MonoVM embedding API calls would be to update src/Mono.Android/Android.Runtime/JNIEnvInit.cs to contain:
partial class JNIEnvInit {
[DllImport ("xamarin-app")]
static extern unsafe void xamarin_app_init (IntPtr env, delegate unmanaged <uint, uint, uint, out IntPtr, void> get_function_pointer);
[UnmanagedCallersOnly]
internal static unsafe void Initialize (JnienvInitializeArgs* args)
{
// …
delegate unmanaged <uint, uint, uint, out IntPtr, void> get_function_pointer =
Java.Interop.ManagedMarshalMethodsLookupTable.GetFunctionPointer;
xamarin_app_init (IntPtr.Zero, get_function_pointer);
// …
}
}This only requires that P/Invokes work, which they should.
There was a problem hiding this comment.
I agree that this way of resolving the unmanaged delegate isn't great. I discussed one idea I had in an earlier comment but the p/invoke might be cleaner.
Side note: for this particular case, CoreCLR has an "embedding API" aptly named hdt_get_function_pointer
|
Elaborating on this comment, the following patch appears to work when building and running diff --git a/external/Java.Interop b/external/Java.Interop
index 2a7183a1c..c1db895c0 160000
--- a/external/Java.Interop
+++ b/external/Java.Interop
@@ -1 +1 @@
-Subproject commit 2a7183a1c0d0f072b90fa836dd39d015d7a8c78d
+Subproject commit c1db895c07c275954ce73f0006a86141af258f60
diff --git a/samples/HelloWorld/HelloWorld/HelloWorld.DotNet.csproj b/samples/HelloWorld/HelloWorld/HelloWorld.DotNet.csproj
index 996e0ddb2..6b14ec133 100644
--- a/samples/HelloWorld/HelloWorld/HelloWorld.DotNet.csproj
+++ b/samples/HelloWorld/HelloWorld/HelloWorld.DotNet.csproj
@@ -3,6 +3,7 @@
<TargetFramework>$(DotNetAndroidTargetFramework)</TargetFramework>
<OutputType>Exe</OutputType>
<RootNamespace>HelloWorld</RootNamespace>
+ <SupportedOSPlatformVersion>24</SupportedOSPlatformVersion>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\HelloLibrary\HelloLibrary.DotNet.csproj" />
diff --git a/samples/HelloWorld/HelloWorld/Properties/AndroidManifest.xml b/samples/HelloWorld/HelloWorld/Properties/AndroidManifest.xml
index 3561f38df..30677a34c 100644
--- a/samples/HelloWorld/HelloWorld/Properties/AndroidManifest.xml
+++ b/samples/HelloWorld/HelloWorld/Properties/AndroidManifest.xml
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android" android:versionCode="1" android:versionName="1.0" package="com.xamarin.android.helloworld">
- <uses-sdk android:minSdkVersion="21" />
+ <uses-sdk android:minSdkVersion="24" />
<application android:allowBackup="true" android:icon="@mipmap/icon" android:label="@string/app_name" android:extractNativeLibs="false">
</application>
</manifest>
diff --git a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs
index 7e93806c2..cd4e56b1a 100644
--- a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs
+++ b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs
@@ -86,6 +86,9 @@ namespace Android.Runtime
ValueManager = runtime.ValueManager;
}
+ [DllImport ("xamarin-app")]
+ static extern unsafe void xamarin_app_init (IntPtr env, delegate* unmanaged <int, int, int, IntPtr, void> get_function_pointer);
+
[UnmanagedCallersOnly]
internal static unsafe void Initialize (JnienvInitializeArgs* args)
{
@@ -101,6 +104,9 @@ namespace Android.Runtime
ManagedMarshalMethodsLookupEnabled = args->managedMarshalMethodsLookupEnabled;
java_class_loader = args->grefLoader;
+ delegate* unmanaged <int, int, int, IntPtr, void> get_function_pointer = &ManagedMarshalMethodsLookupTable.GetFunctionPointer;
+ xamarin_app_init (args->env, get_function_pointer);
+
BoundExceptionType = (BoundExceptionType)args->ioExceptionType;
androidRuntime = new AndroidRuntime (args->env, args->javaVm, args->grefLoader, args->Loader_loadClass, args->jniAddNativeMethodRegistrationAttributePresent != 0);
ValueManager = androidRuntime.ValueManager;
diff --git a/src/Mono.Android/Java.Interop/ManagedMarshalMethodsLookupTable.cs b/src/Mono.Android/Java.Interop/ManagedMarshalMethodsLookupTable.cs
index c621a6dad..11b234f76 100644
--- a/src/Mono.Android/Java.Interop/ManagedMarshalMethodsLookupTable.cs
+++ b/src/Mono.Android/Java.Interop/ManagedMarshalMethodsLookupTable.cs
@@ -10,7 +10,7 @@ namespace Java.Interop;
internal class ManagedMarshalMethodsLookupTable
{
[UnmanagedCallersOnly]
- static unsafe void GetFunctionPointer (int assemblyIndex, int classIndex, int methodIndex, IntPtr target)
+ internal static unsafe void GetFunctionPointer (int assemblyIndex, int classIndex, int methodIndex, IntPtr target)
{
try
{
@@ -25,6 +25,7 @@ internal class ManagedMarshalMethodsLookupTable
catch (Exception ex)
{
AndroidEnvironment.UnhandledException (ex);
+ AndroidEnvironment.FailFast ("GetFunctionPointer failed: should not be reached");
}
}
diff --git a/src/native/mono/monodroid/monodroid-glue.cc b/src/native/mono/monodroid/monodroid-glue.cc
index a7d20c185..8878fc9b8 100644
--- a/src/native/mono/monodroid/monodroid-glue.cc
+++ b/src/native/mono/monodroid/monodroid-glue.cc
@@ -697,7 +697,7 @@ MonodroidRuntime::mono_runtime_init ([[maybe_unused]] JNIEnv *env, [[maybe_unuse
if (!application_config.managed_marshal_methods_lookup_enabled) {
xamarin_app_init (env, get_function_pointer_at_startup);
} else {
- xamarin_app_init (env, managed_marshal_method_lookup);
+ // Done in JNIEnvInit.Initialize()
}
}
#endif // def RELEASE && def ANDROID && def NET |
|
@jonpryor thanks for the suggestion with the P/Invoke. I will certainly explore it. It's not clear to me why this change requires a bump for the minimum API level from 21 to 24. Could you elaborate on that? |
The bump of |
This PR aims at creating an alternative to the current implementation of
MonodroidRuntime::get_function_pointerwhich does not use Mono embedding APIs. It is inspired by what xamarin/xamarin-macios does in its "ManagedStaticRegistrar".The way this alternative function pointer lookup method works is that it generates IL of several lookup tables as part of
MarshalMethodsAssemblyRewriter.Each UCO method is given three indexes:
(uint assemblyIndex, uint classIndex, uint methodIndex). These values are later baked into the generated LLVM IR code inMarshalMethodsNativeAssemblyGenerator.The indexes are used in three different switches generated in different parts of the app:
GetFunctionPointer(int methodIndex)method is added to each class which has marshal methods UCOsmethodIndexand returns the unamanged function pointer to the given UCO.__GetFunctionPointer__.Java.Interop.__ManagedMarshalMethodsLookupTable__public class with theGetFunctionPointer(int classIndex, int methodIndex)is added to each assembly which contains classes with marshal methodscassIndexand calls methods from (1).GetFunctionPointer(int methodIndex)on private nested classes (and also protected and private protected). These methods cannot be called directly due to visibility constraint, so additional proxy method(s) are added to their parent class(es) to forward the calls to the nested classes.intindex and are initially part of one huge switch statement, while we are already splitting the lookup into three levels by the assembly and class.Java.Interop.ManagedMarshalMethodsLookupTable.GetFunctionPointer(int assemblyIndex, int classIndex, int methodIndex)inMono.Androidis overwritten:assemblyIndexand calls methods from (2).The "type mapping" part (parts 2 and 3) should later be replaced by a trimmable solution based on dotnet/runtime#110691
This alternative lookup can be enabled by setting
$(_AndroidUseManagedMarshalMethodsLookup)=truealongside with$(AndroidEnableMarshalMethods)=true. It is off by default for Mono and it should be turned on by default with other runtimes when marshal methods are also enabled since they don't have Mono embedding APIs.When testing with Mono, I saw very similar performance as the existing
get_function_pointermethod has. The managed implementation was almost as fast (on emulator: median 77μs vs 73μs, average 158μs vs 114μs), but I am not confident I did the meassurements the right way (I just used chrono high_resolution_clock). If performance of this feature is a concern in the comments, I will figure out the proper way to do the meassurements).I am not set on the naming of the feature. Feedback is very welcome.
/cc @ivanpovazan @vitek-karas