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.

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

Jump to bottom

Remove JNINativeWrapper.CreateDelegate() Usage From Marshal Methods #1258

Open
Tracked by #1192
jonpryor opened this issue Sep 24, 2024 · 5 comments · May be fixed by #1275
Open
Tracked by #1192

Remove JNINativeWrapper.CreateDelegate() Usage From Marshal Methods #1258

jonpryor opened this issue Sep 24, 2024 · 5 comments · May be fixed by #1275
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Milestone
.NET 10

Comments

@jonpryor
Copy link
Member

jonpryor commented Sep 24, 2024 •
edited
Loading

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() used System.Reflection.Emit to 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.
This was referenced Sep 25, 2024
Draft
Open
@jonpryor
Copy link
Member Author

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
Copy link
Member Author

We should also make this "only use blittable types" change as part of this as well: #1027

@jonpryor
Copy link
Member Author

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:

partial class JniRuntime {
    public virtual void OnUnhandledException (ref JniTransition transition, Exception e)
    {
        transition.SetPendingException (e);
        Debugger.BreakForUserUnhandledException (e);
    }
}

catch blocks would then call:

catch (Exception __e) {
    JniEnvironment.Runtime.OnUnhandledException(ref __envp, __e);
}

The downside is that catch blocks become slightly slower (JniEnvironment.Runtime looks up per-thread TLS data, plus virtual method invocation), but that's in an "exceptional" path, so I'm not sure it'll matter, performance-wise.

@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Sep 30, 2024
@jonpryor jonpryor added this to the .NET 10 milestone Sep 30, 2024
@jonathanpeppers
Copy link
Member

The $(DebuggerSupport) MSBuild property toggles the System.Diagnostics.Debugger.IsSupported trimmer feature flag.

We already set this by default in Release mode:

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 Release mode completely?

@jonpryor jonpryor mentioned this issue Aug 28, 2024
Open
3 tasks
@jpobst jpobst linked a pull request Nov 6, 2024 that will close this issue
Draft
@jonpryor
Copy link
Member Author

@jonathanpeppers: I don't immediately understand your previous comment. System.Reflection.Emit is used in two places:

This 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 via jnimarshalmethod-gen…

@jpobst jpobst mentioned this issue Nov 21, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
.NET 10
Development

Successfully merging a pull request may close this issue.

[generator] Eliminate usage of JNINativeWrapper.CreateDelegate in bindings. dotnet/java-interop
3 participants
@jonpryor @jpobst @jonathanpeppers