[generator] Eliminate generated ThresholdType and ThresholdClass overrides - #12687
Draft
simonrozsival wants to merge 6 commits into
Draft
[generator] Eliminate generated ThresholdType and ThresholdClass overrides#12687simonrozsival wants to merge 6 commits into
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
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.