[TrimmableTypeMap] Runtime-only trimmable typemap support#11090
[TrimmableTypeMap] Runtime-only trimmable typemap support#11090simonrozsival wants to merge 19 commits intomainfrom
Conversation
b676200 to
c19872c
Compare
Rebuild the remaining runtime-only portion of PR 11090 on top of main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d5aefaa to
3a8d509
Compare
Remove the accidental fallback chain from TryGetJniName while preserving the cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the trimmable peer path on TrimmableTypeMapTypeManager/JavaMarshalValueManager, make any legacy TypeManager lookup in trimmable mode fail fast, and tighten the proxy caches/state handling for parity with the existing activation flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop silently falling back when trimmable peer activation cannot resolve a generated JavaPeerProxy. In trimmable mode, missing proxy coverage should surface as an explicit runtime error so generator gaps are fixed at the source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collapse the trimmable managed-to-JNI lookup back to a single internal API by keeping TryGetJniNameForManagedType() and removing the duplicate TryGetJniName() entry point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce TrimmableTypeMap surface area by making purely local proxy helpers private and removing the unused TryCreatePeer() wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename GetProxyForPeer() to GetProxyForJavaObject() so the helper more clearly describes what it actually inspects and resolves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 issue, and the PR is not mergeable yet because the required dotnet-android checks are still pending.
⚠️ Error handling:JavaMarshalValueManager.CreatePeer()now bypasses the baseJniValueManagerguards for disposed managers and invalidJniObjectReferencevalues, so the trimmable path can throw the newNotSupportedExceptionwhere the inherited contract would returnnullor raiseObjectDisposedException(src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs:510).
👍 Nice cleanup overall: the runtime-only split is much clearer now, especially the removal of the legacy TypeManager redirect and the narrower TrimmableTypeMap surface.
Review generated by android-reviewer from review guidelines.
Preserve the inherited JniValueManager.CreatePeer() preconditions for disposed managers and invalid JNI references before the trimmable fail-fast proxy check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds runtime-side support for the trimmable typemap path in Mono.Android, shifting managed↔JNI name/type resolution and peer creation away from legacy typemap codepaths and ensuring JniRuntime is published early enough for trimmable initialization.
Changes:
- Route
JNIEnv.TypemapManagedToJavato managedTrimmableTypeMapwhenRuntimeFeature.TrimmableTypeMapis enabled. - Publish the current
JniRuntimeearlier inJNIEnvInitand initializeTrimmableTypeMappost-runtime creation. - Make legacy
TypeManager.GetJavaToManagedTypeCoreexplicitly unreachable in trimmable mode and centralize peer creation viaTrimmableTypeMap.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Adds caches and new logic for resolving proxies/JNI names and creating peers for Java objects in trimmable mode. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs | Switches peer creation in trimmable mode to TrimmableTypeMap.CreatePeer and fails fast when proxy coverage is missing. |
| src/Mono.Android/Java.Interop/TypeManager.cs | Throws an UnreachableException if legacy java→managed lookup is used while trimmable typemap is enabled. |
| src/Mono.Android/Android.Runtime/JNIEnvInit.cs | Sets the current JniRuntime earlier and again after constructing AndroidRuntime, before typemap initialization. |
| src/Mono.Android/Android.Runtime/JNIEnv.cs | Uses managed trimmable typemap resolution for managed→Java type mapping when enabled. |
| var cached = _peerProxyCache.GetOrAdd (className, static (name, self) => { | ||
| if (!self._typeMap.TryGetValue (name, out var mappedType)) { | ||
| return null; | ||
| } | ||
|
|
||
| return mappedType.GetCustomAttribute<JavaPeerProxy> (inherit: false); | ||
| }, this); | ||
|
|
There was a problem hiding this comment.
_peerProxyCache.GetOrAdd can return null when the Java type isn't present in _typeMap, but ConcurrentDictionary cannot store null values. This will throw during hierarchy walking for unknown Java types. Consider caching only successful lookups, or use a non-null sentinel/wrapper to represent a miss.
| var cached = _peerProxyCache.GetOrAdd (className, static (name, self) => { | |
| if (!self._typeMap.TryGetValue (name, out var mappedType)) { | |
| return null; | |
| } | |
| return mappedType.GetCustomAttribute<JavaPeerProxy> (inherit: false); | |
| }, this); | |
| JavaPeerProxy? cached = null; | |
| if (!_peerProxyCache.TryGetValue (className, out cached)) { | |
| if (_typeMap.TryGetValue (className, out var mappedType)) { | |
| cached = mappedType.GetCustomAttribute<JavaPeerProxy> (inherit: false); | |
| if (cached != null) { | |
| cached = _peerProxyCache.GetOrAdd (className, cached); | |
| } | |
| } | |
| } |
| if (proxy is not null && TryGetJniNameForManagedType (targetType, out var targetJniName)) { | ||
| var selfRef = new JniObjectReference (handle); | ||
| var objClass = JniEnvironment.Types.GetObjectClass (selfRef); | ||
| var targetClass = JniEnvironment.Types.FindClass (targetJniName); | ||
| try { | ||
| if (!JniEnvironment.Types.IsAssignableFrom (objClass, targetClass)) { | ||
| proxy = null; | ||
| } | ||
| } finally { | ||
| JniObjectReference.Dispose (ref objClass); | ||
| JniObjectReference.Dispose (ref targetClass); | ||
| } |
There was a problem hiding this comment.
In CreatePeer(), objClass is acquired before FindClass(). If FindClass(targetJniName) throws (e.g., class missing), objClass won't be disposed because the try/finally is entered only after both locals are initialized. Refactor so objClass is always disposed (initialize targetClass to default and assign within the try).
| var targetName = targetType?.AssemblyQualifiedName ?? "<null>"; | ||
| var javaType = JniEnvironment.Types.GetJniTypeNameFromInstance (reference); | ||
| throw new NotSupportedException ( | ||
| $"No generated {nameof (JavaPeerProxy)} was found for Java type '{javaType}' " + | ||
| $"with targetType '{targetName}' while {nameof (RuntimeFeature.TrimmableTypeMap)} is enabled. " + | ||
| $"This indicates a missing trimmable typemap proxy or association and should be fixed in the generator."); | ||
| } |
There was a problem hiding this comment.
When trimmable typemap peer creation fails, this code throws without disposing reference using the provided transfer option. That can leak JNI references on error paths. Dispose reference (or otherwise ensure ownership is released) before throwing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 issue, and the internal dotnet-android pipeline is also currently red due to a CS0234 compile error in JavaMarshalValueManager.cs.
- ❌ API design:
TryGetTargetType()now throws for generated typemap alias entries, so a normal Java→managed lookup on an alias such asjni/name[1]will fail before the#10788follow-up lands (src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs:81).
👍 The split between proxy resolution in TrimmableTypeMap and peer activation in JavaMarshalValueManager is a nice cleanup.
Review generated by android-reviewer from review guidelines.
| if (proxy is null) { | ||
| // Alias typemap entries (for example "jni/name[1]") are not implemented yet. | ||
| // Support for them will be added in a follow-up for https://github.com/dotnet/android/issues/10788. | ||
| throw new NotImplementedException ( |
There was a problem hiding this comment.
🤖 ❌ API design — TryGetTargetType() is part of the probe path used during Java→managed resolution, but this branch now throws NotImplementedException for alias slots like jni/name[1]. Since those aliases are emitted by the typemap generator, a normal lookup will now crash before the follow-up PR lands. Can we keep alias entries non-fatal here (for example, return false until #10788 is implemented) instead of throwing?
Rule: API design
There was a problem hiding this comment.
Thanks — this is intentional for now. We are still in the middle of the trimmable typemap rollout, and alias handling is explicitly being deferred to the follow-up in #10788. I would prefer to keep that work out of this PR rather than expand the scope further.
JNIEnv.TypemapManagedToJavauses managed JNI-name resolution fromTrimmableTypeMapfor the trimmable pathJNIEnvInitpublishes the currentJniRuntimebefore trimmable typemap initialization so managed JNI services and native registration work correctlyTypeManager.GetJavaToManagedTypeis now explicitly unreachable in trimmable mode; lookups should flow throughTrimmableTypeMapTypeManagerJavaMarshalValueManager+TrimmableTypeMaphandle the remaining runtime peer creation/JNI-name resolution after the split and now fail fast if required proxy coverage is missing