Skip to content
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.

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

[Mono.Android] Use C#9 Function Pointer backend for JNI #8157

Closed
wants to merge 3 commits into from

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Jun 28, 2023

Context: dotnet/java-interop@312fbf4

As per dotnet/java-interop@312fbf43:

With a Release build, the Average Invocation time for
JIFunctionPointersTiming takes 97% of the time as JIPinvokeTiming,
i.e. is 3% faster.

Let's see if that's true on Android: use the C#9 Function Pointer
backend within Java.Interop.dll.

@jonpryor jonpryor requested a review from grendello as a code owner June 28, 2023 22:21
@grendello
Copy link
Contributor

grendello commented Jun 29, 2023

The general direction is fine, but the p/invoke tables include file must be generated again using this utility, so that the _TestPinvokeTables target doesn't fail. For some reason your committed changes to the include file fail the test

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I compared this PR vs main, the average displayed time of 10 runs on a Pixel 5:

Before:
Average(ms): 752.7
Std Err(ms): 2.51241362482817
Std Dev(ms): 7.94494947889678
After:
Average(ms): 749.8
Std Err(ms): 3.34597602435456
Std Dev(ms): 10.5809052332754

And then the app looks like it contains a Java.Interop.dll with the new code path:
image

So, this is probably worth it for a small startup boost, small app size improvement.

@jonpryor
Copy link
Member Author

jonpryor commented Jul 5, 2023

MSBuild Emulator Tests > macOS > Tests > WearOS is failing, with an assertion failure!

* Assertion at /__w/1/s/src/mono/mono/mini/interp/transform.c:3792, condition `csignature->call_convention == MONO_CALL_DEFAULT || csignature->call_convention == MONO_CALL_C' not met, function:interp_transform_call, Interpreter supports only cdecl pinvoke on x86

which would appear to be from here: https://github.com/dotnet/runtime/blob/eaa9717d90115cea43b4cdd7a2a49e6d3c3d780e/src/mono/mono/mini/interp/transform.c#L3792

The offending test is: https://github.com/xamarin/xamarin-android/blob/69b4ab056badcb5d25a5faf39d08baeb1b8b7010/tests/MSBuildDeviceIntegration/Tests/DebuggingTest.cs#L383-L555

I'm not sure why this is only asserting on Wear OS and not on the other emulator environments.

@jonpryor jonpryor added the do-not-merge PR should not be merged. label Jul 5, 2023
@jonpryor
Copy link
Member Author

jonpryor commented Jul 5, 2023

@jonpryor wrote:

I'm not sure why this is only asserting on Wear OS and not on the other emulator environments.

It looks like the Wear OS tests are using x86, while the default ABI is x86_64 unless on an arm mac:

https://github.com/xamarin/xamarin-android/blob/69b4ab056badcb5d25a5faf39d08baeb1b8b7010/build-tools/scripts/TestApks.targets#L21-L22

Furthermore, the assert is wrapped in a #ifdef TARGET_X86 block.

As for "why is the interpreter used?" -- hence interp/transform.c in the assertion message -- that's because it's enabled by default in Debug builds:

https://github.com/xamarin/xamarin-android/blob/69b4ab056badcb5d25a5faf39d08baeb1b8b7010/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets#L51

I don't think that we can merge this until MonoVM Interpreter supports C#9 function pointers to native functions on x86 (or we drop support for x86, which feels very unlikely).

@jonpryor
Copy link
Member Author

jonpryor commented Jul 5, 2023

@BrzVlad: would it be possible to bring the #ifdef and the comment into alignment on:

https://github.com/dotnet/runtime/blob/eaa9717d90115cea43b4cdd7a2a49e6d3c3d780e/src/mono/mono/mini/interp/transform.c#L3790-L3793

and change the #ifdef TARGET_x86 to #if defined(TARGET_X86) && defined(TARGET_WINDOWS)? That way (presumably) we could avoid that assertion on Unixy platforms?

@BrzVlad
Copy link
Member

BrzVlad commented Jul 6, 2023

@jonpryor I don't see why any x86 call convention wouldn't work with interpreter pinvoke slow path. Opened PR dotnet/runtime#88466 which is untested, but I expect to would make everything work here.

Context: dotnet/java-interop@312fbf4

As per dotnet/java-interop@312fbf43:

> With a Release build, the Average Invocation time for
> JIFunctionPointersTiming takes 97% of the time as JIPinvokeTiming,
> i.e. is 3% faster.

Let's see if that's true *on Android*: use the C#9 Function Pointer
backend within `Java.Interop.dll`.
Thanks to the glory of build logs (thanks f59a3c2!) we see that
`Java.Interop.dll` is built in `msbuild-20230630T153002-leeroy-all.binlog`
and is *probably* built via `Mono.Android.Runtime.csproj`, *not* via
`Mono.Android.csproj`, and these project references are inconsistent.

Instead of "hunting down" every `Java.Interop.csproj` reference and
adding `%(AdditionalProperties)`, instead update
`external/Java.Interop.override.props` so that `$(Standalone)`=True
is set for *all* `Java.Interop.csproj` builds.  This should ensure
consistency, and also ensure that we're actually building the
configuration that we want.
@jonpryor
Copy link
Member Author

jonpryor commented Aug 1, 2023

Superseded by PR #8234.

@jonpryor jonpryor closed this Aug 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge PR should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants