-
Notifications
You must be signed in to change notification settings - Fork 52
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
jonpryor
wants to merge
7
commits into
main
Choose a base branch
from
dev/jonp/jonp-zero-alloc-member-lookup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Commits on Jul 5, 2024
-
[Java.Interop] Add JniMemberInfoLookup
Context: c6c487b Context: 312fbf4 Context: 2197579 Context: dotnet/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
Configuration menu - View commit details
-
Copy full SHA for d1a9951 - Browse repository at this point
Copy the full SHA d1a9951View commit details -
Configuration menu - View commit details
-
Copy full SHA for 41254f2 - Browse repository at this point
Copy the full SHA 41254f2View commit details -
Configuration menu - View commit details
-
Copy full SHA for 86e15a3 - Browse repository at this point
Copy the full SHA 86e15a3View commit details -
Configuration menu - View commit details
-
Copy full SHA for 6dd933c - Browse repository at this point
Copy the full SHA 6dd933cView commit details
Commits on Jul 9, 2024
-
Configuration menu - View commit details
-
Copy full SHA for cdf4902 - Browse repository at this point
Copy the full SHA cdf4902View commit details -
Merge remote-tracking branch 'origin/main' into dev/jonp/jonp-zero-al…
…loc-member-lookup
Configuration menu - View commit details
-
Copy full SHA for bd7dddd - Browse repository at this point
Copy the full SHA bd7ddddView commit details -
Commit d1a9951 showed that *for just field lookup*, the idea of a `ref struct JniMemberInfoLookup` *might* be a good idea. Now that we've expanded `JniMemberInfoLookup` plumbing to include *method* lookup, we can throw it into `TimingTests.cs` and see how it compares! The answer is that `ref struct`s and `Span<T>` are *not* magical performance sauce with magical JIT support, and this is with CoreCLR! Method Lookup + Invoke Timing: Traditional: 00:00:00.0175778 No caching: 00:00:00.0202369 Dict w/ lock: 00:00:00.0181357 ConcurrentDict: 00:00:00.0220411 JniPeerMembers: 00:00:00.0209174 JPM+Lookup: 00:00:00.0186421 (I)I virtual+traditional: 00:00:00.0000600 (I)I virtual+JniPeerMembers: 00:00:00.0000588 (I)I virtual+JPM+Lookup: 00:00:00.0007137 The new timings are `JPM+Lookup` and `virtual+JPM+Lookup`. // JniPeerMembers return _members.InstanceMethods.InvokeVirtualObjectMethod("toString.()Ljava/lang/String;", this, null); // JPM+Lookup var member = new JniMemberInfoLookup("toString.()Ljava/lang/String;", "toString"u8, "()Ljava/lang/String;"u8); ReadOnlySpan<JniArgumentValue> args = null; return _members.InstanceMethods.InvokeVirtualObjectMethod (member, this, args); We see that JPM+Lookup is 11% *faster* when no arguments are involved. Nice! Throw an argument into the mix: // (I)I virtual+JniPeerMembers var args = stackalloc JniArgumentValue [1]; args [0] = new JniArgumentValue (value); return _members.InstanceMethods.InvokeVirtualInt32Method ("VirtualIntMethod1Args.(I)I", this, args); // (I)I virtual+JPM+Lookup var member = new JniMemberInfoLookup ( "VirtualIntMethod1Args.(I)I", "VirtualIntMethod1Args"u8, "(I)I"u8 ); var args = stackalloc JniArgumentValue [1]; args [0] = new JniArgumentValue (value); return _members.InstanceMethods.InvokeVirtualInt32Method (member, this, new ReadOnlySpan<JniArgumentValue> (args, 1)); and we're now a *whole order of magnitude worse*, taking 12.1x longer. Which quickly makes this idea as-is unworkable. Maybe it's the ReadOnlySpan<T> usage, and if I went back to straight `JniArgumentValue*` values it would be better?
Configuration menu - View commit details
-
Copy full SHA for 840f04a - Browse repository at this point
Copy the full SHA 840f04aView commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.