Skip to content

[Mono.Android] Extend JNINativeWrapper.CreateBuiltInDelegate exploration #9309

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

Closed
wants to merge 1 commit into from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 17, 2024

Note: This is just a POC exploration for discussion, it is not intended to be committed.
Note: All performance numbers mentioned are run on Android Emulator on a DevBox, so they are somewhat inflated.

Run a Release version of the dotnet new android template and it takes 1.385s to start up. Now add the following code to MainActivity.cs:

protected override void OnApplyThemeResource (Resources.Theme? theme, int resId, bool first)
{
    base.OnApplyThemeResource (theme, resId, first);
}

Compile and run the app again, now it takes 1.608s to start up, an increase of 223ms. What gives?

It turns out that part of #6657 is a "marketing" performance optimization. By ensuring every delegate needed by our template is "built-in" we never hit a delegate that needs System.Runtime.Emit. However once a user adds most any other code they will hit a delegate that isn't "built-in" which needs SRE and they will take the considerable perf hit of initializing SRE and generating the first delegate.

To avoid this, what if we took the concept of "built-in" delegates and formalized and expanded it? That is, each version of netX.0-android would contain a known set of built-in delegates that libraries could depend on.

If we scan Mono.Android plus the ~630 AndroidX/etc. libraries we currently bind, we find that there are 1037 unique delegates in our "ecosystem". This PR adds all of them to JNINativeWrapper.CreateBuiltInDelegate, thus avoiding initializing SRE and taking the performance hit.

Tradeoffs

Per our apk-diff unit test on CI (BuildReleaseArm64), adding an additional 1000 delegates and wrapper functions comes at a cost in .apk size of ~61 KB:

61,440 Package size difference 0.58% (of 10,579,211)

Unfortunately, JNINativeWrapper.CreateBuiltInDelegate acts as a "choke-point" method in that it references every built-in delegate and wrapper, so unused ones cannot be removed by the trimmer.

Enhancements

In the long run, we can avoid the .apk size increase by making this process trimmer friendly. In addition to JNINativeWrapper.CreateDelegate we could expose the individual delegate creation methods:

public class JNINativeWrapper {
  public _Jni_Marshal_PP_V CreateBuiltInDelegate_PP_V (_Jni_Marshal_PP_V dlg) { ... }
  public _Jni_Marshal_PP_J CreateBuiltInDelegate_PP_J (_Jni_Marshal_PP_J dlg) { ... }
  public _Jni_Marshal_PP_Z CreateBuiltInDelegate_PP_Z (_Jni_Marshal_PP_Z dlg) { ... }
  etc...
}

Because the list of built-in delegates is a per-.NET level contract, a binding library targeting net10.0-android knows that it can replace calls to JNINativeWrapper.CreateDelegate with calls to explicit delegate creator methods:

// net9.0-android
JNINativeWrapper.CreateDelegate (new _JniMarshal_PP_L (n_GetText1));

// net10.0-android
JNINativeWrapper.CreateDelegate_PP_L (new _JniMarshal_PP_L (n_GetText1));

This provides 2 benefits:

  • It performs better by avoiding the large switch statement in JNINativeWrapper.CreateBuiltInDelegate.
  • It is trimmer-friendly. In a future where all binding libraries have been updated and the generic JNINativeWrapper.CreateDelegate is no longer called, all unused delegates and wrappers will be trimmed out of the final application.

What About SRE?

One will note this doesn't actually eliminate the original problem of needing SRE, it just makes it much less likely. If a user binds a library with a delegate we've never seen before they'll fall back to SRE. We can eliminate this by adding any missing delegates to their application assembly.

Before we compile the user application, we need to scan their referenced assemblies for any delegates not part of the supported "built-in" set. We then need to generate the C# delegate wrappers and add the generated code to their application. Lastly we need to pass a reference to this fall-back method to JNINativeWrapper on app startup so it can use the fall-back.

// In the user's app
class AdditionalDelegates
{
  public static Delegate CreateAdditionalDelegate (Delegate dlg, Type delegateType)
  {
    switch (delegateType.Name) {
      case nameof (Wrap_JniMarshal_PPZZ_Z):
        return new Wrap_JniMarshal_PPZZ_Z (Unsafe.As<_JniMarshal_PPIJIJIJIJ_V> (dlg)._JniMarshal_PPIJIJIJIJ_V);
    }
  }

  internal static bool Wrap_JniMarshal_PPZZ_Z (this _JniMarshal_PPZZ_Z callback, IntPtr jnienv, IntPtr klazz, bool p0, bool p1) { ... }
}

Then at app startup we pass this to JNINativeWrapper:

JNINativeWrapper.RegisterAdditionalDelegateCreator (AdditionalDelegates.CreateAdditionalDelegate);

And JNINativeWrapper.CreateDelegate uses it as a fallback if CreateBuiltInDelegate fails:

var result = CreateBuiltInDelegate (dlg, delegateType);

if (result != null)
  return result;

var result = _additionalCreator (dlg, delegateType);

if (result != null)
  return result;

SRE Is Gone Now?

Almost!

There is still a case where SRE would be used: if the user is referencing a Classic binding assembly built before we changed from using Action<T>/Func<T> to _Jni_Marshal_* delegates. This feels like an acceptable limitation. We could add a build warning when this case is detected if desired.

So SRE would be gone for pure .NET for Android applications.

@jpobst jpobst changed the title [Mono.Android] Extend JNINativeWrapper.CreateBuiltInDelegate [Mono.Android] Extend JNINativeWrapper.CreateBuiltInDelegate exploration Sep 17, 2024
@jonpryor
Copy link
Contributor

Similar to #9306, I would prefer dotnet/runtime#108211.

The question is, when can dotnet/runtime#108211 be implemented?

@jonpryor
Copy link
Contributor

dotnet/runtime#108211 has an implementation for CoreCLR, but not yet MonoVM: dotnet/runtime#108211 (comment)

Assuming that MonoVM gets support for Debugger.BreakForUserUnhandledException() in the .NET 10 timeframe, we could update our bindings to make use of it. See: dotnet/java-interop#1258

Making this change would mean that JNINativeWrapper.CreateDelegate() & JNINativeWrapper.CreateBuiltInDelegate() would no longer be used in .NET 10+ binding assemblies, and would be more performant as well.

@jpobst jpobst closed this Dec 3, 2024
@jpobst jpobst deleted the more-builtin-delegates branch December 3, 2024 22:30
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Dec 4, 2024
…1275)

Fixes: #1258

Context: c8f3e51
Context: 176240d
Context: dotnet/runtime#108211
Context: dotnet/android#9306
Context: dotnet/android#9309
Context: https://github.com/xamarin/monodroid/commit/3e9de5a51bd46263b08365ef18bed1ae472122d8
Context: dotnet/android@8bc7a3e

The [Java Native Interface][0] allows native code to be associated
with a [Java `native` method declaration][1], either by way of
[`Java_`-prefixed native functions][2], or via function pointers
provided to [`JNIEnv::RegisterNatives()`][3].

Both `Java_`-prefixed native functions and function pointers must
refer to C-callable functions with appropriate
[native method arguments][4].

A *Marshal Method* is a:

 1. Method or delegate which is C-callable,
 2. Accepting the appropriate Java Native Method arguments,
 3. Is responsible for marshaling parameter and return types, and
 3. *Delegates* the call to an appropriate managed method override.

We have multiple different Marshal Method implementations running
around, including:

  * XamarinAndroid1 and XAJavaInterop1 Marshal Methods, in which the
    Marshal Method is an `n_`-prefixed method in (roughly-ish) the
    same scope as the method that would be delegated to.
  * `jnimarshalmethod-gen`: see 176240d
  * LLVM Marshal Methods, which use LLVM-IR to emit `Java_`-prefixed
    native functions; see dotnet/android@8bc7a3e8.

Which brings us to the current XAJavaInterop1 Marshal Methods
implementation.  Consider the [`java.util.function.IntConsumer`][5]
interface:

	// Java
	public /* partial */ interface IntConsumer {
	  void accept(int value);
	}

With `generator --codegen-target=XAJavaInterop1` -- used by
.NET for Android -- `IntConsumer` is bound as `IIntConsumer`:

	namespace Java.Util.Functions {

	  // Metadata.xml XPath interface reference: path="/api/package[@name='java.util.function']/interface[@name='IntConsumer']"
	  [Register ("java/util/function/IntConsumer", "", "Java.Util.Functions.IIntConsumerInvoker", ApiSince = 24)]
	  public partial interface IIntConsumer : IJavaObject, IJavaPeerable {
	    [Register ("accept", "(I)V", "GetAccept_IHandler:Java.Util.Functions.IIntConsumerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", ApiSince = 24)]
	    void Accept (int value);
	  }

	  [Register ("java/util/function/IntConsumer", DoNotGenerateAcw=true, ApiSince = 24)]
	  internal partial class IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
	    static Delegate? cb_accept_Accept_I_V;
	    static Delegate GetAccept_IHandler ()
	    {
	      if (cb_accept_Accept_I_V == null)
	        cb_accept_Accept_I_V = JNINativeWrapper.CreateDelegate (new _JniMarshal_PPI_V (n_Accept_I));
	      return cb_accept_Accept_I_V;
	    }

	    static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
	    {
	      var __this = global::Java.Lang.Object.GetObject<Java.Util.Functions.IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
	      __this.Accept (value);
	    }
	  }
	}

The Marshal Method is `IIntConsumerInvoker.n_Accept_I()`.

We also have a *Connector Method*.  A Connector Method is a `static`
method matching the signature of `Func<Delegate>`.  The name of the
connector method is mentioned in the 3rd `connector` parameter of
`RegisterAttribute` on the interface method.

During [Java Type Registration][6], all Connector methods for a type
are looked up and invoked, and the `Delegate` instances returned from
all those connector method invocations are provided to
`JNIEnv::RegisterNatives()`.

There are static and runtime issues with connector method and marshal
method implementations until now:

 1. Java Native Methods, and thus Marshal Methods, *must* conform to
    the C ABI.  C does not support exceptions.  C# *does*.

    What happens when `__this.Accept(value)` throws?

 2. The answer to (1) is in the connector method, via the
    `JNINativeWrapper.CreateDelegate()` invocation.
    [`JNINativeWrapper.CreateDelegate()`][7] uses
    System.Reflection.Emit to *wrap* the Marshal Method with a
    try/catch block.

At runtime, the intermixing of (1) and (2) will result in registering
a method similar to the following with `JNIEnv::RegisterNatives()`:

	static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
	{
	  JNIEnv.WaitForBridgeProcessing ();
	  try {
	    var __this = ava.Lang.Object.GetObject<IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
	    __this.Accept (value);
	  }
	  catch (Exception e) when (!Debugger.IsAttached) {
	    AndroidEnvironment.UnhandledException (e);
	  }
	}

which presents a further two problems:

 1. System.Reflection.Emit is used, which possibly slows down type
    registration and won't work with NativeAOT.
 2. The `catch` block only executes when you're *not* debugging!

Which means that if you're debugging the app, and an exception is
thrown, you are now potentially unwinding the stack frame through a
JNI boundary, which can *corrupt JVM state*, possibly resulting in an
[app abort or crash][8].  ([***Why?!***][9])

This has been how things work since the beginning.

.NET 9 introduces some features that allow us to rethink all this:

  * [`DebuggerDisableUserUnhandledExceptionsAttribute`][10]
  * [`Debugger.BreakForUserUnhandledException(Exception)`][11]

> If a .NET Debugger is attached that supports the
> [BreakForUserUnhandledException(Exception)][11] API, the debugger
> won't break on user-unhandled exceptions when the exception is
> caught by a method with this attribute, unless
> [BreakForUserUnhandledException(Exception)][11] is called.

Embrace .NET 9, remove the possible need for System.Reflection.Emit,
and fully prevent possible JVM corruption by updating connector
methods and marshal methods to instead be:

	namespace Java.Util.Functions {

	  internal partial class IIntConsumerInvoker {
	    static Delegate? cb_accept_Accept_I_V;
	    static Delegate GetAccept_IHandler ()
	    {
	      return cb_accept_Accept_I_V ??= new _JniMarshal_PPI_V (n_Accept_I);
	    }

	    [DebuggerDisableUserUnhandledExceptions]
	    static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
	    {
	      if (!JniEnvironment.BeginMarshalMethod (jnienv, out var __envp, out var __r))
	        return;

	      try {
	        var __this = Java.Lang.Object.GetObject<IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
	        __this.Accept (value);
	      } catch (global::System.Exception __e) {
	        __r?.OnUserUnhandledException (ref __envp, __e);
	      } finally {
	        JniEnvironment.EndMarshalMethod (ref __envp);
	      }
	    }
	  }
	}

This removes the call to `JNINativeWrapper.CreateDelegate()` and it's
implicit use of System.Reflection.Emit, properly wraps *everything*
in a `try`/`catch` block so that exceptions are properly caught and
marshaled back to Java if necessary, and integrates properly with
expected "first chance exception" semantics.

The *downside* is that this requires the "new debugger backend" to
work, which at the time of this writing is only used by VSCode.
As this code will only be used for .NET 10+ (2025-Nov), this is fine.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/jniTOC.html
[1]: http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#compiling_loading_and_linking_native_methods
[2]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#resolving_native_method_names
[3]: http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#RegisterNatives
[4]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#native_method_arguments
[5]: https://developer.android.com/reference/java/util/function/IntConsumer
[6]: https://github.com/dotnet/android/wiki/Blueprint#java-type-registration
[7]: https://github.com/dotnet/android/blob/65906e0b7b2f471fcfbd07e7e01b68169c25d9da/src/Mono.Android/Android.Runtime/JNINativeWrapper.cs#L29-L105
[8]: dotnet/android#8608 (comment)
[9]: dotnet/android#4877
[10]: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debuggerdisableuserunhandledexceptionsattribute?view=net-9.0
[11]: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debugger.breakforuserunhandledexception?view=net-9.0#system-diagnostics-debugger-breakforuserunhandledexception(system-exception)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants