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

[Java.Interop] Add JniMemberInfoLookup #1208

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonpryor
Copy link
Member

Context: c6c487b
Context: 312fbf4
Context: 2197579
Context: xamarin/xamarin-android#7276

There is a desire to remove the "marshal-ilgen" component from .NET Android, which is responsible for all non-blittable type marshaling within P/Invoke (and related) invocations.

The largest source of such non-blittable parameter marshaling was with string marshaling: JNIEnv::GetFieldID() was "wrapped" by java_interop_jnienv_get_field_id:

JI_API jfieldID java_interop_jnienv_get_field_id (JNIEnv *env, jthrowable *_thrown, jclass type, const char* name, const char* signature);

which was P/Invoked within JniEnvironment.g.cs:

partial class NativeMethods {
    internal static extern unsafe IntPtr java_interop_jnienv_get_field_id (IntPtr jnienv, out IntPtr thrown, jobject type, string name, string signature);
}

and string parameter marshaling is not blittable.

Turns out™ that this particular usage of non-blittable parameter marshaling was fixed and rendered moot by:

  • 312fbf4: C#9 function pointer backend for JNIEnv invocations
  • c6c487b: "Standalone" build config to use C#9 function pointers
  • 2197579: Standalone build config is now the default

That said, this code path felt slightly less than ideal: the "top-level abstraction" for member lookups is an "encoded member", a string containing the name of the member, a ., and the JNI signature of the member, e.g.:

_members.InstanceFields.GetBooleanValue("propogateFinallyBlockExecuted.Z", this)

The "encoded member" would need to be split on ., and with c6c487b the name and signature would be separately passed to Marshal.StringToCoTaskMemUTF8(), which performs a memory allocation and converts the UTF-16 string to UTF-8.

Meanwhile, C# 11 introduced UTF-8 string literals, which allows the compiler to deal with UTF-8 conversion and memory allocation.

Enter `JniMemberInfoLookup``:

public ref struct JniMemberInfoLookup {
    public  string                  EncodedMember   {get;}
    public  ReadOnlySpan<byte>      MemberName      {get;}
    public  ReadOnlySpan<byte>      MemberSignature {get;}

    public JniMemberInfoLookup (string encodedMember, ReadOnlySpan<byte> memberName, ReadOnlySpan<byte> memberSignature);
}

JniMemberInfoLookup removes the need to call
Marshal.StringToCoTaskMemUTF8() entirely, at the cost of a more complicated member invocation:

// Old and busted:
bool value = _members.InstanceFields.GetBooleanValue("propogateFinallyBlockExecuted.Z", this);

// Eventual new hawtness:
var lookup = new JniMemberInfoLookup (
	"propogateFinallyBlockExecuted.Z",
	"propogateFinallyBlockExecuted"u8,
	"Z"u8);
bool value = _members.InstanceFields.GetBooleanValue(lookup, this);

Is It Worth It™? Maybe; see the new
JniFieldLookupTiming.FieldLookupTiming() test, which allocates a new JniPeerMembers instance and invoke
members.InstanceFields.GetFieldInfo(string) and
members.InstanceFields.GetFieldInfo(JniMemberInfoLookup). (A new JniPeerMembers instance is required because GetFieldInfo() caches the field lookup.) Using JniMemberInfoLookup is about 4% faster.

# FieldLookupTiming Timing: looking up JavaTiming.instanceIntField 10000 times
#   .InstanceMethods.GetFieldInfo(string):              00:00:02.2780667
#   .InstanceMethods.GetFieldInfo(JniMemberInfoLookup): 00:00:02.2016146

I'm not sure if this is actually worth it, especially as this will imply an increase in code size.

TODO:

  • Update JniPeerMembers.*.cs to use JniMemberInfoLookup, so that e.g. the above _members.InstanceFields.GetBooleanValue() overload exists.

  • generator changes to use JniMemberInfoLookup

jonpryor and others added 2 commits March 29, 2024 13:51
Context: c6c487b
Context: 312fbf4
Context: 2197579
Context: xamarin/xamarin-android#7276

There is a desire to remove the "marshal-ilgen" component from
.NET Android, which is responsible for all non-blittable type
marshaling within P/Invoke (and related) invocations.

The largest source of such non-blittable parameter marshaling was
with string marshaling: `JNIEnv::GetFieldID()` was "wrapped" by
`java_interop_jnienv_get_field_id`:

	JI_API jfieldID java_interop_jnienv_get_field_id (JNIEnv *env, jthrowable *_thrown, jclass type, const char* name, const char* signature);

which was P/Invoked within `JniEnvironment.g.cs`:

	partial class NativeMethods {
	    internal static extern unsafe IntPtr java_interop_jnienv_get_field_id (IntPtr jnienv, out IntPtr thrown, jobject type, string name, string signature);
	}

and `string` parameter marshaling is *not* blittable.

Turns out™ that this particular usage of non-blittable parameter
marshaling was fixed and rendered moot by:

  * 312fbf4: C#9 function pointer backend for `JNIEnv` invocations
  * c6c487b: "Standalone" build config to use C#9 function pointers
  * 2197579: Standalone build config is now the default

That said, this code path felt slightly less than ideal: the
"top-level abstraction" for member lookups is an "encoded member",
a string containing the name of the member, a `.`, and the JNI
signature of the member, e.g.:

	_members.InstanceFields.GetBooleanValue("propogateFinallyBlockExecuted.Z", this)

The "encoded member" would need to be split on `.`, and with c6c487b
the name and signature would be separately passed to
`Marshal.StringToCoTaskMemUTF8()`, which performs a memory allocation
and converts the UTF-16 string to UTF-8.

Meanwhile, [C# 11 introduced UTF-8 string literals][0], which allows
the compiler to deal with UTF-8 conversion and memory allocation.

Enter `JniMemberInfoLookup``:

	public ref struct JniMemberInfoLookup {
	    public  string                  EncodedMember   {get;}
	    public  ReadOnlySpan<byte>      MemberName      {get;}
	    public  ReadOnlySpan<byte>      MemberSignature {get;}

	    public JniMemberInfoLookup (string encodedMember, ReadOnlySpan<byte> memberName, ReadOnlySpan<byte> memberSignature);
	}

`JniMemberInfoLookup` removes the need to call
`Marshal.StringToCoTaskMemUTF8()` entirely, at the cost of a more
complicated member invocation:

	// Old and busted:
	bool value = _members.InstanceFields.GetBooleanValue("propogateFinallyBlockExecuted.Z", this);

	// Eventual new hawtness:
	var lookup = new JniMemberInfoLookup (
		"propogateFinallyBlockExecuted.Z",
		"propogateFinallyBlockExecuted"u8,
		"Z"u8);
	bool value = _members.InstanceFields.GetBooleanValue(lookup, this);

Is It Worth It™?  *Maybe*; see the new
`JniFieldLookupTiming.FieldLookupTiming()` test, which allocates a new
`JniPeerMembers` instance and invoke
`members.InstanceFields.GetFieldInfo(string)` and
`members.InstanceFields.GetFieldInfo(JniMemberInfoLookup)`.
(A new `JniPeerMembers` instance is required because `GetFieldInfo()`
caches the field lookup.)  Using `JniMemberInfoLookup` is about
4% faster.

	# FieldLookupTiming Timing: looking up JavaTiming.instanceIntField 10000 times
	#   .InstanceMethods.GetFieldInfo(string):              00:00:02.2780667
	#   .InstanceMethods.GetFieldInfo(JniMemberInfoLookup): 00:00:02.2016146

I'm not sure if this is *actually* worth it, especially as this will
imply an increase in code size.

TODO:

  * Update `JniPeerMembers.*.cs` to use `JniMemberInfoLookup`, so
    that e.g. the above `_members.InstanceFields.GetBooleanValue()`
    overload exists.

  * `generator` changes to use `JniMemberInfoLookup`

[0]: https://learn.microsoft.com/dotnet/csharp/whats-new/csharp-11#utf-8-string-literals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant