[Java.Interop] Use native UTF-8 JNI remaps - #12795
simonrozsival wants to merge 2 commits into
Conversation
Carry stable UTF-8 replacement pointers through remapping results and use them directly for class and member lookup, avoiding UTF-16 round trips. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved remapping wiring, fast-path, diagnostic, and test-coverage issues remain.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 3
Open (4)
What changed in this PR
This pull request adds native UTF-8 pointer support for Java.Interop JNI remapping to reduce managed string conversions.
Changes:
- Adds pointer-backed remapping metadata and lookup overloads.
- Updates instance/static method and field remapping.
- Updates the unshipped API baseline.
| File | Description |
|---|---|
external/Java.Interop/src/Java.Interop/PublicAPI.Unshipped.txt |
Records new UTF-8 pointer APIs. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs |
Adds native pointer lookup overloads. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniTypeManager.cs |
Adds pointer-backed replacement metadata. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs |
Updates static method remapping. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticFields.cs |
Updates static field remapping. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs |
Updates instance method and constructor remapping. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceFields.cs |
Updates instance field remapping. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs |
Adds shared remapping helper dispatch. |
Use native pointers independently for replacement names and signatures, and report effective fallback values when a remapped member lookup fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 2 warnings · 0 suggestions
The mixed UTF-8/string fallback handling added in the follow-up commit resolves the earlier all-or-nothing fast-path issue, and the JNI exception cleanup paths remain consistent with the existing implementation. Before merging, the new public raw-pointer contract should explicitly define its lifetime/ownership requirements, and the pointer-backed and mixed lookup branches need direct regression coverage.
CI is still in progress with no failures reported so far; Java.Interop tests have passed on macOS and Windows.
Generated by Android PR Reviewer for #12795 · copilot · gpt56 · 132.3 AIC · ⌖ 11.4 AIC · ⊞ 21K
Comment /review to run again
| ReadOnlySpan<char> fallbackSignature, | ||
| [NotNullWhen (true)] out JniMethodInfo? method) | ||
| { | ||
| if (info.TargetJniMethodNameUtf8 != IntPtr.Zero) { |
There was a problem hiding this comment.
🤖 TargetJni*Utf8 property, so the pointer/pointer and both mixed pointer/span branches added here are untested; the original all-or-nothing fast-path bug compiled and passed the cited 23 tests for exactly this reason. Please add JniPeerMembersTests coverage using stable unmanaged UTF-8 storage for method and field remaps (instance/static), including an omitted target signature, and cover the constructor signature-pointer path as well.
Rule: Regression coverage for new branches
| targetJniMethodSignatureUtf8 = IntPtr.Zero; | ||
| } | ||
| } | ||
| public IntPtr TargetJniTypeUtf8 { |
There was a problem hiding this comment.
🤖 IntPtr properties do not document the required encoding, NUL termination, ownership, or lifetime. Both the paired string getters and the JNI lookup path dereference/scan the pointer after GetReplacement*InfoCore() has returned, so a custom JniTypeManager can reasonably return a pointer backed by a temporary fixed block or stack buffer and leave a dangling pointer before it is consumed. Please define the stable-lifetime contract in XML docs for all six pointer properties (method and field), or keep the raw-pointer representation behind an internal API that can guarantee ownership.
Rule: Interop pointer lifetime
Motivation
R8 runtime remapping already stores replacement JNI type, method, field, and signature names as stable null-terminated UTF-8 strings in generated native data. Converting those names to managed UTF-16 strings and then encoding them back to UTF-8 for JNI adds avoidable allocations and work on every remapped member lookup.
Approach
JniTypeclass/member lookup paths that accept stable UTF-8 pointers and pass them directly toFindClass,GetMethodID, andGetFieldID.JniPeerMemberswhen available, while retaining the existing string/span fallback for custom type managers and legacy remaps without generated target signatures.Stack
This is the foundational PR for #12796 and is based on the R8 runtime-remapping work in #12692.
Validation
JniPeerMembersTests: 23 passed, 1 skipped.