[generator] Eliminate generated ThresholdType and ThresholdClass overrides - #12687
simonrozsival wants to merge 6 commits into
Conversation
Use JniPeerMembers metadata for new XA bindings while retaining legacy threshold dispatch for types that still declare threshold getters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Follow-up to 9dcd55f, from a review pass over that change. * Restore the `GC.KeepAlive (this)` calls that a5779e9 ("Prevent premature JNI handle collection", fixes #5405) added to `AbsListView.SetAdapter` and `AdapterViewAnimator.Adapter`. Rewriting those methods to dispatch through `JniPeerMembers` dropped them: `InvokeVirtual*Method()` reads `self.PeerReference` and then passes only the raw handle to JNI, so the peer is not a live GC root for the duration of the native call. `CursorLoader.LoadInBackground` gains the same protection and the `try`/`finally` it needs to hold it. * Compute `XAPeerMembers`' threshold-override detection lazily instead of in the constructor. Detection is a name-based `Type.GetMethod()` lookup, and running it during type initialization made every bound type an app touches pay for reflection on the startup path -- for a result that is `false` for every binding generated after 9dcd55f. It is now computed on the first dispatch decision, and `UsesLegacyVirtualDispatch()` short-circuits on the declaring side so the receiver's lookup is skipped entirely unless legacy bindings are actually in play. Detection intentionally keys on `ThresholdType` only: every generator path that emitted threshold overrides (bound classes, class invokers, interface invokers) emitted it, and `ThresholdClass` was never emitted without it. * Register `Xamarin.Android.LegacyThresholdBinding.csproj` in `Xamarin.Android-Tests.slnx`; it previously built only as a transitive `ProjectReference` of `Xamarin.Android.JcwGen-Tests`. * Split the mixed-generation assertions out of `LegacyVirtualMethodBinding` into `ModernDerivedFromLegacyBinding` so a failure identifies which binding-generation combination broke, and explain what one-time GREFs the `JavaSideActivation` warm-up is excluding from its measurement. Verified: `Mono.Android.csproj` builds clean, JcwGen device suite 41/41 on emulator-5554. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the reflection-based legacy threshold detection and its IL2070 suppression. Old generated bindings return their JniPeerMembers managed type from ThresholdType, while new bindings inherit Object's or Throwable's type. Comparing those values identifies the old shape without reflecting over non-public methods or relying on a trimmer suppression. The comparison also handles a new binding derived from an old one: it inherits the old ThresholdType but replaces JniPeerMembers, so the values differ and metadata-based dispatch is used. Pure old hierarchies keep matching values and retain legacy dispatch. Add focused on-device coverage for the three hand-written wrappers converted from direct threshold access: * AbsListView.SetAdapter, including a managed override that proves the base call dispatches nonvirtually; * AdapterViewAnimator.Adapter on exact and managed-derived types; * CursorLoader.LoadInBackground on exact and managed-derived types, proving a base call does not re-enter the managed override. The wrapper tests share the ThresholdDispatch category for focused execution. Verified on emulator-5554: * ThresholdDispatch runtime tests: 3/3 * trimmed Release CoreCLR JcwGen tests: 41/41 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the internal Object.GetThresholdType() and Throwable.GetThresholdType() forwarding methods with UnsafeAccessor methods in XAPeerMembers. UnsafeAccessorKind.Method emits callvirt: the accessors bind to the permanent protected base getters, while old compiled binding overrides continue to participate in virtual dispatch. Newly generated bindings no longer override ThresholdType, so they reach the base getter and are classified for metadata-based dispatch. Old bindings still override the same base slot and retain legacy dispatch. The protected virtual properties themselves remain for binary compatibility with those assemblies. Remove the now-unused internal GetThresholdClass() forwarding methods as well. Verified on emulator-5554: * ThresholdDispatch runtime tests: 3/3 * trimmed Release CoreCLR JcwGen tests: 41/41 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Make the legacy/new dispatch distinction explicit in generated code: * new XAJavaInterop1 bindings instantiate plain JniPeerMembers and therefore use its metadata-based dispatch directly; * already-compiled bindings continue to instantiate XAPeerMembers, which is retained as the legacy compatibility implementation. XAPeerMembers first checks the receiver's peer-members type. For an XAPeerMembers receiver it then confirms that the normal virtual ThresholdType getter returns the type represented by those peer members. This second check keeps hand-written XAPeerMembers users without generated threshold overrides on metadata dispatch, and handles a new binding deriving from an old one. ThresholdClass is no longer needed by the compatibility path. The only remaining forwarding method invokes ThresholdType through normal C# virtual dispatch; there is no reflection, UnsafeAccessor, or trimmer suppression. Switch the few hand-written modern peer-member declarations to JniPeerMembers, refresh generator goldens, and add a regression for an XAPeerMembers-derived binding without a matching threshold override. Verified: * generator-Tests: 490/490 * ThresholdDispatch device tests: 3/3 * trimmed Release CoreCLR JcwGen tests: 42/42 * Mono.Android build and API checks: clean Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use the simpler XAPeerMembers marker flow requested in review: when the receiver also uses XAPeerMembers, consult its legacy ThresholdType; otherwise fall through to JniPeerMembers metadata dispatch. Keep the existing internal GetThresholdType() name and document its legacy-only purpose instead of encoding that purpose in a renamed API. Also replace the empty warm-up using block in JavaSideActivation with an explicit local and Dispose() call. Verified: trimmed Release CoreCLR JcwGen tests 41/41. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
![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 · 1 warning · 1 suggestion. The metadata-dispatch split is coherent and the mixed-generation Java.Lang.Object coverage is strong, but the stated binary-compatibility guarantee does not cover existing callers that reference one of the removed concrete override getters. The separate Throwable compatibility branch also needs equivalent regression coverage.
CI: Azure build #1583136 is red: both Package Tests macOS > Tests > APKs 1 and APKs 2 failed; the other visible matrix checks and CLA passed. This matches the PR's draft / do-not-merge status and its note that full CI remains outstanding.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12687 · gpt56 · 263.5 AIC · ⌖ 13.8 AIC · ⊞ 25.7K
Comment /review to run again
| var peer = opt.CodeGenerationTarget == Xamarin.Android.Binder.CodeGenerationTarget.XAJavaInterop1 ? "XAPeerMembers" : "JniPeerMembers"; | ||
|
|
||
| Value = $"new {peer} (\"{rawJniType}\", typeof ({declaringType}){(isInterface ? ", isInterface: true" : string.Empty)})"; | ||
| Value = $"new JniPeerMembers (\"{rawJniType}\", typeof ({declaringType}){(isInterface ? ", isInterface: true" : string.Empty)})"; |
There was a problem hiding this comment.
🤖 Java.Lang.Object/Throwable. An already-compiled subclass can contain a direct member reference to (for example) Android.App.Activity.get_ThresholdType() when it evaluates base.ThresholdType; after this change that concrete getter no longer exists, so executing that code can throw MissingMethodException. Please either preserve compatibility for those concrete members, or explicitly accept/document the break and cover it with a consumer compiled against the previous Mono.Android.dll.
Rule: Preserve shipped API compatibility
| } | ||
| var t = value as Java.Lang.Throwable; | ||
| if (t != null) { | ||
| if (value is Java.Lang.Throwable t) { |
There was a problem hiding this comment.
🤖 💡 Testing — The compatibility implementation has a distinct Java.Lang.Throwable path, but the new legacy/mixed-generation fixtures exercise only Java.Lang.Object. Please add an old-shape Throwable-derived binding case (including a managed subclass) so exception bindings do not lose legacy virtual/nonvirtual dispatch unnoticed.
Rule: Cover compatibility branches
1 participant
This is an investigation and compatibility prototype for #12679. The design and device coverage are in place, but broader CI and the follow-up method-cache performance investigation are still outstanding.
Why
XAJavaInterop1 currently emits
ThresholdTypeandThresholdClassoverrides into essentially every generated bound type. These overrides duplicate information already represented byJniPeerMembers, add thousands of properties and associated attributes toMono.Android.dlland its reference assembly, and expand the public API ledger without adding user-facing functionality.This change removes those generated overrides while preserving virtual/nonvirtual JNI dispatch for bindings compiled before this change.
Fixes #12679
Design
New bindings use metadata dispatch
The XAJavaInterop1 generator now emits plain
JniPeerMembersfields:JniPeerMembers.UsesVirtualDispatch()andGetPeerMembers()use the declaring type and receiver peer metadata to choose virtual or nonvirtual dispatch. Newly generated bindings therefore do not need per-type threshold properties.Old binaries retain legacy dispatch
Previously compiled binding assemblies contain both:
new XAPeerMembers(...)in their generated IL; andThresholdType/ThresholdClassoverrides.XAPeerMembersremains public and acts as the legacy compatibility implementation. When both the declaring method and receiver useXAPeerMembers, it readsThresholdTypethrough a normal C# virtual call and applies the original threshold dispatch rule. New generated types useJniPeerMembers, so the runtime can distinguish old and new binding shapes without reflection, configuration, attributes, or assembly-version heuristics.The protected base properties on
Java.Lang.ObjectandJava.Lang.Throwableremain as binary-compatible virtual slots for old binding overrides.ThresholdClassis no longer consulted by current runtime dispatch.Mixed-generation inheritance
The important edge case is a new binding deriving from an old binding:
XAPeerMembersJniPeerMembersJniPeerMembersXAPeerMembersJniPeerMembersA separate old-shape binding assembly and Java hierarchy exercise these combinations on-device.
Other required changes
Three hand-written
Mono.Androidwrappers still read threshold properties directly and would otherwise dispatch againstjava/lang/Objectafter generated overrides disappear. They now useJniPeerMembersinvocation APIs:Android.Content.CursorLoader.LoadInBackground()Android.Widget.AbsListView.SetAdapter()Android.Widget.AdapterViewAnimator.AdapterThe existing
GC.KeepAlive()protections are retained so borrowed JNI handles remain rooted through native calls.JavaSideActivationnow performs one explicit warm-up instance before measuring GREF lifetime. The removedThresholdClassaccess had previously initialized a class reference as an accidental side effect, causing a one-time GREF acquisition inside the measured window.API and size impact
The API 35–37.2
PublicAPI.Unshipped.txtfiles record the threshold members as removed. The large ~36,000-line diff is the expected*REMOVED*ledger, not new API surface.The four hand-written
Android.OS.AsyncTask<TParams,TProgress,TResult>entries remain because that type still declares its compatibility overrides. The base virtual pairs onJava.Lang.ObjectandJava.Lang.Throwablealso remain for binary compatibility.Initial API 37 measurements (before the final peer-members marker refinement):
Mono.Android.dllimplementationMono.Android.dllreference assemblyThese numbers should be remeasured before removing the draft /
do-not-mergestatus.Tests
generator-Tests: 490/490Mono.Android.csprojbuild with API compatibility checksXamarin.Android.JcwGen-Tests: 41/41ThresholdDispatchwrapper tests: 3/3AbsListView.SetAdapterAdapterViewAnimator.AdapterCursorLoader.LoadInBackgroundXamarin.Android.Build.TestsMono.Android.NET-Testscompletion (a local run reached 254 passes, then the process crashed in unrelated networking testResponseHeadersReadBodyReadCancellationIsPrompt)Review notes
UnsafeAccessor, or trimmer-warning suppression in the final detection path.XAPeerMembers→JniPeerMembers, plus removal of threshold overrides.JniMethodInfocache fields versusJniInstanceMethods' string-keyedConcurrentDictionary; that experiment is intentionally not part of this PR.