[TrimmableTypeMap] Extract JavaMarshal registered peer tracking

[simonrozsival]

Goal

Extract the JavaMarshal peer-registration and GC-bridge bookkeeping from the monolithic JavaMarshalValueManager into JavaMarshalRegisteredPeers. This will allow us to share code between "coreclr llvm-ir" and "coreclr/nativeaot trimmable typemap" value managers which interact with the CoreCLR GC bridge.

This is the narrow Peer registration / GC bridge slice from #11617, split out so the common infrastructure can be reviewed independently before the broader Java.Interop value-manager/type-manager work lands.

Change map

Area	Additions are for	Removals are for
Peer registration / GC bridge	Adds JavaMarshalRegisteredPeers, containing registered peer tracking, reference-tracking handles, CoreCLR GC bridge callbacks, finalization, and surfaced-peer enumeration.	Removes that bookkeeping from JavaMarshalValueManager.
Value manager shape	Keeps JavaMarshalValueManager focused on value-manager behavior by delegating AddPeer, PeekPeer, RemovePeer, FinalizePeer, CollectPeers, and GetSurfacedPeers.	Removes peer-table fields, collected-context queues, and GC bridge callback implementation from the value manager.
GC-bridge init plumbing	JavaMarshalRegisteredPeers.InitializeIfNeeded () performs the one-shot clr_initialize_gc_bridge registration; the value manager calls it from its constructor.	Removes the managed→native→managed handle round-trip: clr_initialize_gc_bridge and the native GCBridge no longer take/store a java_marshal_value_manager handle, and the finished callback no longer receives it.
Project wiring	Adds JavaMarshalRegisteredPeers.cs to Mono.Android.csproj.	No intended product behavior change.

The main deletion theme, matching #11617, is: remove peer-registration/GC-bridge scaffolding from the old value manager shape. The main addition theme is: make that shared behavior reusable by multiple value managers later.

Design notes

The registered-peer table and the GC-bridge registration are process-wide and never torn down (the detached bridge-processing thread cannot be stopped, and clr_initialize_gc_bridge aborts if it runs twice). To reflect that:

• JavaMarshalRegisteredPeers is a purely static type. Initialization happens through an idempotent internal static void InitializeIfNeeded () (lock + flag): the first call performs the one-shot init, subsequent calls return immediately, so every value manager can safely call it.
• There is no Dispose/IDisposable — the type lives for the lifetime of the process and is never collected.
• Because the type is static, the native side no longer needs a handle back to a managed instance: clr_initialize_gc_bridge drops its IntPtr parameter, the finished callback becomes delegate* unmanaged<MarkCrossReferencesArgs*, void>, and the corresponding intptr_t plumbing is removed from the CoreCLR/NativeAOT host (gc-bridge.hh/.cc, internal-pinvokes).

Review notes / intentional non-goals

• This is a preparatory refactor for the broader manager split in [TrimmableTypeMap] Implement reflection-free TrimmableTypeMapValueManager and TrimmableTypeMapTypeManager #11617.
• It does not change CreatePeer behavior.
• It does not add CoreClrJavaMarshalValueManager or TrimmableTypeMapValueManager.
• It does not change MonoVM/CoreCLR/NativeAOT value-manager selection.
• It does not change generated typemap behavior or build targets.

Testing

Not run locally (no provisioned SDK/NDK in the authoring environment); CI's CoreCLR and NativeAOT device legs exercise this path and will validate.

[Copilot]

Pull request overview

This PR refactors JavaMarshal’s managed peer tracking by extracting peer registration/GC-bridge bookkeeping out of JavaMarshalValueManager into a dedicated JavaMarshalRegisteredPeers helper, and centralizing shared peer-type / cast-compatibility helpers.

Changes:

• Add JavaMarshalRegisteredPeers to own registered-peer bookkeeping and GC-bridge integration; JavaMarshalValueManager delegates peer tracking to it.
• Add JavaMarshalValueManagerHelper to share peer-type resolution and cast-compatibility checks.
• Wire the new source files into Mono.Android.csproj.

Show a summary per file

File	Description
src/Mono.Android/Mono.Android.csproj	Includes new runtime helper classes in the build.
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs	Delegates peer tracking to JavaMarshalRegisteredPeers and uses shared helper methods.
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs	New extracted peer registration + GC bridge bookkeeping implementation.
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManagerHelper.cs	New shared helpers for peer type resolution and cast compatibility diagnostics.

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 2

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Code Review — faithful refactor, no behavior change

This is a clean code-move refactor: the peer-registration / GC-bridge bookkeeping is extracted from JavaMarshalValueManager into a new sealed JavaMarshalRegisteredPeers : IDisposable, with the value manager delegating AddPeer/PeekPeer/RemovePeer/FinalizePeer/CollectPeers/GetSurfacedPeers.

I diffed the moved code line-by-line against the base version. The only non-mechanical changes are all behavior-preserving, and I verified the two that looked substantive:

• GetJniIdentityHashCode (reference) → JniEnvironment.References.GetIdentityHashCode (reference) (in PeekPeer). Equivalent: both ultimately call JniSystem.IdentityHashCode — the base JniValueManager.GetJniIdentityHashCode just wraps it, and References.GetIdentityHashCode is a one-liner over the same call. The substitution is needed because this type isn't a value manager, and it keeps the lookup key consistent with value.JniIdentityHashCode (set via JNIEnv.IdentityHash → the same References.GetIdentityHashCode).
• Runtime.ObjectReferenceManager → JniEnvironment.Runtime.ObjectReferenceManager (in WarnNotReplacing/FinalizePeer). Equivalent given the single process-wide AndroidRuntime.
• Nullability tightened — WeakReference<IJavaPeerable?> and IJavaPeerable? target correctly type values that can be null (the originals had latent CS8600/CS8625). 👍
• sealed + a direct IDisposable.Dispose () (no Dispose(bool) pattern) matches the repo convention for sealed types. 👍

Mono.Android.csproj uses explicit <Compile Include> items (no globbing), so the added include is correct with no duplicate-item risk.

Findings

0 ❌ · 0 ⚠️ · 2 💡 — both inline, and both about documenting the GC-bridge single-instance / process-lifetime invariant. They aren't bugs in this PR (only one value manager is constructed today), but since the stated goal is to reuse this type across the llvm-ir and trimmable-typemap managers, the native side abort_unless-es on a second clr_initialize_gc_bridge, so the "exactly one instance per process" contract is worth making explicit before the follow-up lands.

CI

Azure DevOps checks were green at review time (build & smoke, MSBuild, emulator, and most APK suites passing); a couple of jobs (the dotnet-android aggregate and APKs 2) were still queued/in-progress. No failures observed.

Nice, well-scoped split — easy to review independently. Submitting as a comment (not an approval) per review policy.

Generated by Android PR Reviewer for issue #11750 · 1.7K AIC · ⌖ 66.5 AIC · ⊞ 40K
_{Comment /review to run again}

[simonrozsival]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Review summary — looks good overall

This is a clean, well-structured mechanical refactor: peer-registration and GC-bridge bookkeeping is extracted out of JavaMarshalValueManager into a new process-wide static JavaMarshalRegisteredPeers, and the now-unused intptr_t java_marshal_value_manager_handle parameter is dropped from clr_initialize_gc_bridge and the bridge-finished callback. The new doc comments explaining the process-lifetime / one-shot-initialization semantics are a nice touch.

Verifications I ran (all behavior-preserving):

• ✅ The native callback signature change (clr_initialize_gc_bridge, BridgeProcessingStarted/BridgeProcessingFinished) is consistent across the managed P/Invokes (RuntimeNativeMethods.cs), internal-pinvokes.hh, internal-pinvokes-shared.cc, gc-bridge.hh/gc-bridge.cc — no dangling java_marshal_value_manager references remain.
• ✅ PeekPeer switching from GetJniIdentityHashCode (reference) to JniEnvironment.References.GetIdentityHashCode (reference) is equivalent — both resolve to JniSystem.IdentityHashCode (reference).
• ✅ FinalizePeer's two RemovePeer (value) calls are in mutually exclusive branches (invalid/local vs. valid/non-local), matching the upstream ManagedValueManager structure — not a double-removal.
• ✅ InitializeIfNeeded uses a plain lock + bool flag (not double-checked locking), so it's correctly idempotent.

Suggestions (non-blocking, posted inline):

• 💡 Resource management — the refactor drops the ThrowIfDisposed () guard from the five delegating peer methods while CreatePeer/ActivatePeer keep it; worth confirming that's intentional (and if so, the disposed/ThrowIfDisposed () machinery is now nearly vestigial).
• 💡 Code organization — the new helpers are public static on an internal class with a single same-assembly caller; internal static would match InitializeIfNeeded and repo convention.

Severity counts: 0 ❌ · 0 ⚠️ · 2 💡

⏳ CI: the public dotnet-android builds (Mac/Windows/Linux) were still in progress at review time — not failing, but not yet green. Please hold the merge until CI goes green; the two suggestions above are not blocking.

Generated by Android PR Reviewer for issue #11750 · 1.7K AIC · ⌖ 48.2 AIC · ⊞ 37.9K
_{Comment /review to run again}

[jonathanpeppers]

Looks good — making the GC-bridge bookkeeping a process-wide static matches its actual lifetime semantics (one-shot clr_initialize_gc_bridge, detached bridge-processing thread that can never be torn down), and removes the awkward managed→native→managed handle round-trip.

Why this is effectively behavior-neutral

• Peer table is already process-global. HandleContext.referenceTrackingHandles was already a static readonly dictionary nested in JavaMarshalValueManager, so promoting RegisteredInstances / CollectedContexts to statics changes ownership, not lifetime or sharing.
• GC-bridge init was already one-shot. Native initialize_callback aborts if called twice; the old constructor path implicitly relied on a single value manager. InitializeIfNeeded (lock + flag) makes that explicit and idempotent, so multiple future value managers (llvm-ir, trimmable-typemap) can safely share it.
• No managed instance was ever needed natively. The old intptr_t java_marshal_value_manager_handle was only round-tripped back into BridgeProcessingFinished to call instance methods that touched static state anyway. Dropping the handle and the native field is a pure simplification.
• Callbacks are semantically identical. BridgeProcessingStarted / BridgeProcessingFinished / ProcessCollectedContexts were moved verbatim; the only delta is instance.ProcessCollectedContexts → static call.
• API surface preserved. AddPeer / PeekPeer / RemovePeer / FinalizePeer / CollectPeers / GetSurfacedPeers overrides still exist and forward 1:1; locking discipline (lock (RegisteredInstances), lock (referenceTrackingHandles)) is unchanged.
• Equivalent helpers. GetJniIdentityHashCode(reference) → JniEnvironment.References.GetIdentityHashCode(reference) and Runtime.ObjectReferenceManager → JniEnvironment.Runtime.ObjectReferenceManager both resolve to the same underlying calls (JniSystem.IdentityHashCode / the value manager's own Runtime).
• Minor nullability tightening. WeakReference<IJavaPeerable> → WeakReference<IJavaPeerable?> and IJavaPeerable target → IJavaPeerable? target in RemovePeer are correctness improvements that match Target's actual signature; no runtime change.

Note (non-blocking)

ThrowIfDisposed() is no longer invoked from the peer methods (it remains in CreatePeer). Since the value manager is process-lifetime and the underlying state is now correctly modeled as process-global, this is fine — but the disposed field / ThrowIfDisposed helper is now nearly dead and could be cleaned up in a follow-up if desired.