Repository navigation
Remove JNINativeWrapper.CreateDelegate() Usage From Marshal Methods #1258
Description
Context: dotnet/runtime#108211
Context: dotnet/android#9306
Context: dotnet/android#9309
Context: https://github.com/xamarin/monodroid/commit/3e9de5a51bd46263b08365ef18bed1ae472122d8
Consider this marshal method and related infrastructure::
namespace Java.Util.Functions {
public partial interface IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
static Delegate? cb_accept_Accept_I_V;
#pragma warning disable 0169
[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
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;
}
[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
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);
}
#pragma warning restore 0169
}
}Why do we have JNINativeWrapper.CreateDelegate()? (Closely related: why isn't there a try/catch block in n_Accept_I()? Though part of that is "we didn't think of it.")
The answer is around the "unhandled exception" experience when a debugger is attached: the debugger breaks at the "active" throw site. If the type will be caught by a catch block, then you won't get an "unhandled exception" notification, which made customers sad.
Which means that for a good debugger experience, we can't have catch blocks catching exceptions; if we did, then the exceptions would be handled!
Meanwhile, we must catch and marshal exceptions back to Java, otherwise we'll corrupt the JVM during stack unwind!
Where we wound up was a terrible middle:
- If a debugger is attached, then basically have no exception handling. (Wild oversimplification, but close enough for here.)
- If a debugger isn't attached, then all exceptions are caught.
JNINativeWrapper.CreateDelegate()usedSystem.Reflection.Emitto bridge these two worlds.
However, .NET 9 introduces System.Diagnostics.DebuggerDisableUserUnhandledExceptionsAttribute:
If a .NET Debugger is attached that supports the BreakForUserUnhandledException(Exception) API, the debugger won't break on user-unhandled exceptions when the exception is caught by a method with this attribute, unless BreakForUserUnhandledException(Exception) is called.
Thus, the proposal: update the above marshal method related infrastructure to instead be:
namespace Java.Util.Functions {
public partial interface IIntConsumerInvoker : global::Java.Lang.Object, IIntConsumer {
static Delegate? cb_accept_Accept_I_V;
#pragma warning disable 0169
[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
static Delegate GetAccept_IHandler ()
{
return cb_accept_Accept_I_V ?? (cb_accept_Accept_I_V = new _JniMarshal_PPI_V (n_Accept_I));
}
[global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
[global::System.Diagnostics.DebuggerDisableUserUnhandledExceptionsAttribute)]
static void n_Accept_I (IntPtr jnienv, IntPtr native__this, int value)
{
var __envp = new global::Java.Interop.JniTransition (jnienv);
try {
var __this = global::Java.Lang.Object.GetObject<Java.Util.Functions.IIntConsumer> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
__this.Accept (value);
}
catch (Exception e) {
__envp.SetPendingException (e);
Debugger.BreakForUserUnhandledException (e);
}
finally {
__envp.Dispose ();
}
}
#pragma warning restore 0169
}
}This entirely removes JNINativeWrapper.CreateDelegate() and in turn System.Reflection.Emit from the marshal method codepath, which should improve app startup.
Activity
jonpryor commentedon Sep 25, 2024
In order for this approach to be viable, MonoVM needs to implement support for
Debugger.BreakForUserUnhandledException(); see also: dotnet/runtime#108211 (comment)Hopefully we can get that support for .NET 10.
jonpryor commentedon Sep 25, 2024
We should also make this "only use blittable types" change as part of this as well: #1027
jonpryor commentedon Sep 27, 2024
Question: should there be any "extensibility" in here, to reduce the need to change (implicit) ABI after this? I'm thinking something along the lines of:
catchblocks would then call:The downside is that
catchblocks become slightly slower (JniEnvironment.Runtimelooks up per-thread TLS data, plusvirtualmethod invocation), but that's in an "exceptional" path, so I'm not sure it'll matter, performance-wise.jonathanpeppers commentedon Oct 21, 2024
The
$(DebuggerSupport)MSBuild property toggles theSystem.Diagnostics.Debugger.IsSupportedtrimmer feature flag.We already set this by default in
Releasemode:One thing we could do, is solve this issue for Release mode by using this trimmer flag, MSBuild property, etc.? We could avoid System.Reflection.Emit for
Releasemode completely?jonpryor commentedon Nov 13, 2024
@jonathanpeppers: I don't immediately understand your previous comment.
System.Reflection.Emitis used in two places:JNINativeWrapper.CreateDelegate()Mono.Android.Export.dll: https://github.com/dotnet/android/blob/70948d5eaa72091389940fdd5a0eb828c85bdd60/src/Mono.Android.Export/CallbackCode.cs#L634-L650This issue #1258 would remove the call to
JNINativeWrapper.CreateDelegate(), and thus remove the need for System.Reflection.Emit entirely for new bindings in .NET 10.This would not impact
[ExportAttribute]usage /Mono.Android.Export; the only way I know of to attempt to fix that would be viajnimarshalmethod-gen…Bump to dotnet/java-interop@2d48efe7 (#9589)
[generator] Remove `JNINativeWrapper.CreateDelegate` from bindings (d…
[generator] Remove `JNINativeWrapper.CreateDelegate` from bindings (#…