From ad2b7431afb1b97f67c59cc38c89655a9a64f696 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 2 Jun 2025 12:17:20 +0200 Subject: [PATCH 01/17] WIP --- .vscode/settings.json | 58 +++ .../Android.Runtime/RuntimeNativeMethods.cs | 4 +- .../ManagedValueManager.cs | 165 ++++++++- src/native/clr/host/gc-bridge.cc | 349 +++++++++++++++++- src/native/clr/host/internal-pinvokes.cc | 10 +- src/native/clr/include/host/gc-bridge.hh | 91 ++++- src/native/clr/include/host/os-bridge.hh | 7 +- .../include/runtime-base/internal-pinvokes.hh | 5 +- 8 files changed, 636 insertions(+), 53 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 485e3cde612..e14e0b5d537 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -4,4 +4,62 @@ ], "cmake.configureOnOpen": false, "dotnetCoreExplorer.searchpatterns": "bin/Test{Debug}/**/net?.0/{Xamarin.Android.Build.Tests,MSBuildDeviceIntegration,Microsoft.Android.Sdk.Analysis.Tests}.dll", + "files.associations": { + "semaphore": "cpp", + "__bit_reference": "cpp", + "__hash_table": "cpp", + "__locale": "cpp", + "__node_handle": "cpp", + "__split_buffer": "cpp", + "__verbose_abort": "cpp", + "array": "cpp", + "bitset": "cpp", + "cctype": "cpp", + "charconv": "cpp", + "clocale": "cpp", + "cmath": "cpp", + "complex": "cpp", + "cstdarg": "cpp", + "cstdint": "cpp", + "cstdio": "cpp", + "cstdlib": "cpp", + "cstring": "cpp", + "ctime": "cpp", + "cwchar": "cpp", + "cwctype": "cpp", + "deque": "cpp", + "execution": "cpp", + "memory": "cpp", + "forward_list": "cpp", + "fstream": "cpp", + "initializer_list": "cpp", + "iomanip": "cpp", + "ios": "cpp", + "iosfwd": "cpp", + "iostream": "cpp", + "istream": "cpp", + "limits": "cpp", + "locale": "cpp", + "mutex": "cpp", + "new": "cpp", + "optional": "cpp", + "print": "cpp", + "queue": "cpp", + "ratio": "cpp", + "source_location": "cpp", + "span": "cpp", + "sstream": "cpp", + "stack": "cpp", + "stdexcept": "cpp", + "streambuf": "cpp", + "string": "cpp", + "string_view": "cpp", + "typeinfo": "cpp", + "unordered_map": "cpp", + "unordered_set": "cpp", + "variant": "cpp", + "vector": "cpp", + "algorithm": "cpp", + "shared_mutex": "cpp" + }, } diff --git a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs index f8cb8e95f17..affb37ef62a 100644 --- a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs +++ b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs @@ -101,7 +101,9 @@ internal unsafe static class RuntimeNativeMethods /// A function pointer that should be passed to JavaMarshal.Initialize() on startup. [DllImport (RuntimeConstants.InternalDllName, CallingConvention = CallingConvention.Cdecl)] internal static extern delegate* unmanaged clr_initialize_gc_bridge ( - delegate* unmanaged bridge_processing_finished_callback + delegate* unmanaged bridge_processing_started_callback, + delegate* unmanaged collect_gchandles, + delegate* unmanaged bridge_processing_finished_callback ); [MethodImplAttribute(MethodImplOptions.InternalCall)] diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 2b4d5f5ffff..54990e50cda 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -25,12 +25,16 @@ class ManagedValueManager : JniRuntime.JniValueManager internal unsafe ManagedValueManager () { - var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingFinished); + var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge ( + &BridgeProcessingStarted, + &CollectGCHandles, + &BridgeProcessingFinished); JavaMarshal.Initialize (mark_cross_references_ftn); } - public override void WaitForGCBridgeProcessing () + public override void WaitForGCBridgeProcessing() { + // AndroidRuntimeInternal.WaitForGCBridgeProcessing(); // TODO } public override void CollectPeers () @@ -153,6 +157,32 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal return null; } + private GCHandle PeekPeerHandle (JniObjectReference reference) + { + if (RegisteredInstances == null) + throw new ObjectDisposedException (nameof (ManagedValueManager)); + + if (!reference.IsValid) + return default; + + int key = GetJniIdentityHashCode (reference); + + lock (RegisteredInstances) { + List? peers; + if (!RegisteredInstances.TryGetValue (key, out peers)) + return default; + + for (int i = peers.Count - 1; i >= 0; i--) { + var p = peers [i]; + if (p.Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) + return p; + } + if (peers.Count == 0) + RegisteredInstances.Remove (key); + } + return default; + } + public override void RemovePeer (IJavaPeerable value) { if (RegisteredInstances == null) @@ -267,35 +297,136 @@ public override List GetSurfacedPeers () } } - static GCHandle CreateReferenceTrackingHandle (IJavaPeerable value) => - JavaMarshal.CreateReferenceTrackingHandle (value, value.JniObjectReferenceControlBlock); - - static unsafe void FreeHandle (GCHandle handle) + // private unsafe struct HandleContext + // { + // public IntPtr ControlBlock; + // public IntPtr Handle; + + // public static HandleContext* Alloc(IntPtr controlBlock) + // { + // var size = (uint)Marshal.SizeOf(); + // var ctx = (HandleContext*)NativeMemory.AllocZeroed(1, size); + // ctx->ControlBlock = controlBlock; + // return ctx; + // } + + // public static void Free(HandleContext* ctx) + // { + // if (ctx->ControlBlock != IntPtr.Zero) { + // NativeMemory.Free((void*)ctx->ControlBlock); + // } + + // NativeMemory.Free((void*)ctx); + // } + // } + + static unsafe GCHandle CreateReferenceTrackingHandle(IJavaPeerable value) + { + // JniObjectReferenceControlBlock* controlBlock = (JniObjectReferenceControlBlock*)value.JniObjectReferenceControlBlock; + // Console.WriteLine($"Creating reference tracking handle for {value.GetType().FullName} with JniObjectReferenceControlBlock: {controlBlock->handle}, type: {controlBlock->handle_type}, weak_handle: {controlBlock->weak_handle}, refs_added: {controlBlock->refs_added}"); + + return JavaMarshal.CreateReferenceTrackingHandle(value, value.JniObjectReferenceControlBlock); + } + + static unsafe void FreeHandle(GCHandle handle) + { + Console.WriteLine($"Freeing handle"); + IntPtr context = JavaMarshal.GetContext(handle); + if (context != IntPtr.Zero) + { + var ctx = (JniObjectReferenceControlBlock*)context; + Console.WriteLine($"Freeing handle with JniObjectReferenceControlBlock: {ctx->handle}, type: {ctx->handle_type}, weak_handle: {ctx->weak_handle}, refs_added: {ctx->refs_added}"); + NativeMemory.Free((void*)context); + } + } + + [UnmanagedCallersOnly] + internal static void BridgeProcessingStarted() { - IntPtr context = JavaMarshal.GetContext (handle); - NativeMemory.Free ((void*) context); + AndroidRuntimeInternal.BridgeProcessing = true; } - + [UnmanagedCallersOnly] - internal static unsafe void BridgeProcessingFinished (MarkCrossReferences* mcr) + internal static unsafe IntPtr CollectGCHandles(MarkCrossReferences* mcr) { + Console.WriteLine($"CollectGCHandles (mcr.ComponentsLen={mcr->ComponentsLen})"); + + List handles = []; + for (int i = 0; i < mcr->ComponentsLen; i++) + { + for (int j = 0; j < mcr->Components[i].Count; j++) + { + Console.WriteLine($"CollectGCHandles i={i} j={j}"); + + var ctx = mcr->Components[i].Context[j]; + if (ctx == IntPtr.Zero) + { + Console.WriteLine($"CollectGCHandles: controlBlock->handle is zero, skipping"); + handles.Add(default); + } + else + { + var controlBlock = (JniObjectReferenceControlBlock*)ctx; + var reference = new JniObjectReference(controlBlock->handle, (JniObjectReferenceType)controlBlock->handle_type); + Console.WriteLine($"CollectGCHandles: controlBlock->handle={controlBlock->handle}, type={controlBlock->handle_type}, weak_handle={controlBlock->weak_handle}, refs_added={controlBlock->refs_added}, reference={reference}"); + GCHandle handle = ((ManagedValueManager)AndroidRuntime.CurrentRuntime.ValueManager).PeekPeerHandle(reference); + Console.WriteLine($"CollectGCHandles: PeekPeerHandle returned {handle.IsAllocated} for reference {reference}"); + handles.Add(handle); + } + + } + } + + Console.WriteLine($"CollectGCHandles: collected {handles.Count} handles"); + + return GCHandle.ToIntPtr(GCHandle.Alloc(handles)); + } + + [UnmanagedCallersOnly] + internal static unsafe void BridgeProcessingFinished(MarkCrossReferences* mcr, IntPtr handles) + { + Console.WriteLine($"BridgeProcessingFinished (mcr.ComponentsLen={mcr->ComponentsLen})"); + List? originalHandles = GCHandle.FromIntPtr(handles).Target as List; + if (originalHandles is null) + { + Console.WriteLine($"BridgeProcessingFinished: invalid handles {handles}, target={GCHandle.FromIntPtr(handles).Target}"); + throw new InvalidOperationException($"Invalid GCHandles collection"); + } + List handlesToFree = []; + for (int i = 0; i < mcr->ComponentsLen; i++) { - for (int j = 0; j < mcr->Components [i].Count; j++) + for (int j = 0; j < mcr->Components[i].Count; j++) { - IntPtr *pContext = (IntPtr*) mcr->Components [i].Context [j]; - handlesToFree.Add (GCHandle.FromIntPtr (*pContext)); - NativeMemory.Free (pContext); + Console.WriteLine($"BridgeProcessingFinished i={i} j={j}"); + + var controlBlock = (JniObjectReferenceControlBlock*)mcr->Components[i].Context[j]; + if (controlBlock->handle == IntPtr.Zero) + { + Console.WriteLine($"BridgeProcessingFinished: controlBlock->handle is zero, skipping"); + + // TODO figure out how to get the GCHandle here + // GCHandle handle = PeekGCHandle(new JniObjectReference(controlBlock->handle, controlBlock->handle_type)); + // if (handle.IsAllocated && handle.Target is IJavaPeerable peer) + // { + // Console.WriteLine($"BridgeProcessingFinished: handle for {peer.GetType().FullName} will be freed"); + // handlesToFree.Add(handle); + // JniObjectReferenceControlBlock.Free(ref controlBlock); + // } + } } } - JavaMarshal.FinishCrossReferenceProcessing (mcr, CollectionsMarshal.AsSpan (handlesToFree)); + Console.WriteLine($"BridgeProcessingFinished: freeing {handlesToFree.Count} handles"); + + JavaMarshal.FinishCrossReferenceProcessing(mcr, CollectionsMarshal.AsSpan(handlesToFree)); + AndroidRuntimeInternal.BridgeProcessing = false; } - const BindingFlags ActivationConstructorBindingFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; + const BindingFlags ActivationConstructorBindingFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; - static readonly Type[] XAConstructorSignature = new Type [] { typeof (IntPtr), typeof (JniHandleOwnership) }; + static readonly Type[] XAConstructorSignature = new Type[] { typeof(IntPtr), typeof(JniHandleOwnership) }; protected override bool TryConstructPeer ( IJavaPeerable self, diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index c03da967d99..a3959dfcf46 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -4,9 +4,18 @@ using namespace xamarin::android; +void GCBridge::wait_for_bridge_processing () noexcept +{ + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing..."); + std::shared_lock lock (processing_mutex); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing done"); +} + void GCBridge::initialize_on_load (JNIEnv *env) noexcept { - abort_if_invalid_pointer_argument (env, "env"); + // abort_if_invalid_pointer_argument ("env", env); + + log_write (LOG_DEFAULT, LogLevel::Info, "Initializing GC bridge"); jclass lref = env->FindClass ("java/lang/Runtime"); jmethodID Runtime_getRuntime = env->GetStaticMethodID (lref, "getRuntime", "()Ljava/lang/Runtime;"); @@ -18,27 +27,343 @@ void GCBridge::initialize_on_load (JNIEnv *env) noexcept Runtime_gc != nullptr && Runtime_instance != nullptr, "Failed to look up Java GC runtime API." ); + + log_write (LOG_DEFAULT, LogLevel::Info, "Initialized GC bridge"); } -[[gnu::always_inline]] void GCBridge::trigger_java_gc () noexcept { - JNIEnv *env = OSBridge::ensure_jnienv (); + log_write (LOG_DEFAULT, LogLevel::Info, "Triggering Java GC"); - // NOTE: Mono has a number of pre- and post- calls before invoking the Java GC. At this point - // it is unknown whether the CoreCLR GC bridge will need anything of that sort, so we just trigger - // the Java GC here. env->CallVoidMethod (Runtime_instance, Runtime_gc); + + if (env->ExceptionCheck ()) [[unlikely]] { + env->ExceptionDescribe (); + env->ExceptionClear (); + log_error (LOG_DEFAULT, "Java GC failed"); + } else { + log_write (LOG_DEFAULT, LogLevel::Info, "Java GC triggered"); + } +} + +bool GCBridge::add_reference (JniObjectReferenceControlBlock *from, jobject to) noexcept +{ + if (add_direct_reference (from->handle, to)) { + from->refs_added++; + return true; + } else { + return false; + } +} + +bool GCBridge::add_direct_reference (jobject from, jobject to) noexcept +{ + jclass java_class = env->GetObjectClass (from); + jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); + + env->DeleteLocalRef (java_class); // TODO do I also need to delete the jmethodID? or that's not necessary? + if (add_method_id) { + env->CallVoidMethod (from, add_method_id, to); + return true; + } + + env->ExceptionClear (); + return false; +} + +int GCBridge::scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + abort_unless (scc->Count < 0, "Attempted to load stashed index from an object which does not contain one."); + + return -scc->Count - 1; +} + +void GCBridge::scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, int index) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + scc->Count = -index - 1; // Store the index as a negative value +} + +bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + return scc->Count < 0; // If Count is negative, it's a bridgeless SCC +} + +jobject GCBridge::get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + + if (scc->Count > 0) { + return scc->ContextMemory [0]->handle; // Return the first context's handle + } else { + abort_unless (temporary_peers != nullptr, "Temporary peers must not be null for bridgeless SCCs"); + + int index = scc_get_stashed_temporary_peer_index (scc); + return env->CallObjectMethod (temporary_peers, ArrayList_get, index); + } +} + +void GCBridge::maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + abort_if_invalid_pointer_argument (handle, "handle"); + + if (scc->Count < 0) { + env->DeleteLocalRef (handle); // Release the local ref for bridgeless SCCs returned from get_scc_representative + } } -void GCBridge::mark_cross_references (MarkCrossReferences* crossRefs) noexcept +void GCBridge::prepare_for_java_collection (MarkCrossReferences* cross_refs) noexcept { - if (bridge_processing_finish_callback == nullptr) [[unlikely]] { - return; + log_write_fmt (LOG_DEFAULT, LogLevel::Info, "GCBridge::prepare_for_java_collection called: {} components", cross_refs->ComponentsLen); + + // Some SCCs might have no IGCUserPeers associated with them, so we must create one + jobject temporary_peers = nullptr; // This is an ArrayList + int temporary_peer_count = 0; // Number of items in temporary_peers + + // Before looking at xrefs, scan the SCCs. During collection, an SCC has to behave like a + // single object. If the number of objects in the SCC is anything other than 1, the SCC + // must be doctored to mimic that one-object nature. + for (int i = 0; i < cross_refs->ComponentsLen; i++) + { + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Processing SCC at index {} with {} objects", i, scc->Count); + + // Count > 1 case: The SCC contains many objects which must be collected as one. + // Solution: Make all objects within the SCC directly or indirectly reference each other + if (scc->Count > 1) { + JniObjectReferenceControlBlock *first = scc->ContextMemory [0]; + JniObjectReferenceControlBlock *prev = first; + + for (int j = 1; j < scc->Count; j++) { + JniObjectReferenceControlBlock *current = scc->ContextMemory [j]; + + // TODO are those handles correct? in the mono GC bridge, there is a more complex logic that gets the handles + // from the context block data... maybe in that case the handles are GCHandles? + add_reference (prev, current->handle); + prev = current; + } + + // ref the first from the final + add_reference (prev, first->handle); + } else if (scc->Count == 0) { + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Creating temporary peer for SCC at index {} with no bridge objects", i); + + // Once per process boot, look up JNI metadata we need to make temporary objects + if (ArrayList_class == nullptr) { + ArrayList_class = reinterpret_cast (OSBridge::lref_to_gref (env, env->FindClass ("java/util/ArrayList"))); + ArrayList_ctor = env->GetMethodID (ArrayList_class, "", "()V"); + ArrayList_add = env->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); + ArrayList_get = env->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); + + abort_unless ( + ArrayList_class != nullptr && ArrayList_ctor != nullptr && ArrayList_get != nullptr, + "Failed to load classes required for JNI" + ); + } + + // Once per prepare_for_java_collection call, create a list to hold the temporary + // objects we create. This will protect them from collection while we build the list. + if (!temporary_peers) { + temporary_peers = env->NewObject (ArrayList_class, ArrayList_ctor); + } + + // Create this SCC's temporary object + jobject peer = env->NewObject (GCUserPeer_class, GCUserPeer_ctor); + env->CallBooleanMethod (temporary_peers, ArrayList_add, peer); + env->DeleteLocalRef (peer); + + scc_set_stashed_temporary_peer_index (scc, temporary_peer_count); + temporary_peer_count++; + } + } + + // Add the cross scc refs + for (int i = 0; i < cross_refs->CrossReferencesLen; i++) + { + ComponentCrossReference *xref = &cross_refs->CrossReferences [i]; + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Processing cross-reference at index {}: {} -> {}", i, xref->SourceGroupIndex, xref->DestinationGroupIndex); + + StronglyConnectedComponent *from_scc = &cross_refs->Components [xref->SourceGroupIndex]; + StronglyConnectedComponent *to_scc = &cross_refs->Components [xref->DestinationGroupIndex]; + + if (is_bridgeless_scc (from_scc)) { + jobject from_handle = get_scc_representative (from_scc, temporary_peers); + jobject to_handle = get_scc_representative (to_scc, temporary_peers); + + add_direct_reference (from_handle, to_handle); + + maybe_release_scc_representative (from_scc, from_handle); + maybe_release_scc_representative (to_scc, to_handle); + } else { + JniObjectReferenceControlBlock *from_cb = from_scc->ContextMemory [0]; + jobject to_handle = get_scc_representative (to_scc, temporary_peers); + + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Adding cross-reference from SCC {} to SCC {}: {} -> {}", + // xref->SourceGroupIndex, xref->DestinationGroupIndex, from_handle, to_handle); + add_reference (from_cb, to_handle); + + maybe_release_scc_representative (to_scc, to_handle); + } + } + + // With xrefs processed, the temporary peer list can be released + env->DeleteLocalRef (temporary_peers); + + // Switch global to weak references + for (int i = 0; i < cross_refs->ComponentsLen; i++) + { + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + + if (scc->Count < 0) { + scc->Count = 0; // Reset any temporary peer index stored in Count + } + + for (int j = 0; j < scc->Count; j++) { + JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; + + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Creating weak global ref for: handle {}, type {}, weak handle {}, refs {}", + (intptr_t)context->handle, + context->handle_type, + (intptr_t)context->weak_handle, + context->refs_added); + + jobject handle = context->handle; + jobject weak = env->NewWeakGlobalRef (handle); + + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Creating weak global ref for handle {:p} in context {:p}: weak {:p}", + // handle, context, weak); + + env->DeleteGlobalRef (handle); // Delete the strong reference after creating the weak one + + context->handle = weak; + context->handle_type = JNIWeakGlobalRefType; + } + } +} + +void GCBridge::clear_references (jobject handle) noexcept +{ + // Clear references from the object + jclass java_class = env->GetObjectClass (handle); + jmethodID clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V"); + + if (clear_method_id) { + env->CallVoidMethod (handle, clear_method_id); + } else { + log_error (LOG_DEFAULT, "Failed to find monodroidClearReferences method"); + } + + env->DeleteLocalRef (java_class); // Clean up the local reference to the class +} + +void GCBridge::cleanup_after_java_collection (MarkCrossReferences* cross_refs) noexcept +{ + // try to switch back to global refs to analyze what stayed alive + for (int i = 0; i < cross_refs->ComponentsLen; i++) + { + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + + for (int j = 0; j < scc->Count; j++) { + JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; + + jobject weak = context->handle; + jobject global = env->NewGlobalRef (weak); + + if (global) { + // Object survived Java GC, so we need to update the handle + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Object survived Java GC: handle {:p}, weak {:p}, global {:p}", + // context->handle, weak, global); + + context->handle = global; + context->handle_type = JNIGlobalRefType; + + if (context->refs_added > 0) { + // Clear references + clear_references (context->handle); + + // Reset refs_added + context->refs_added = 0; + } + } else { + // Object was collected by Java GC + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Object was collected by Java GC: weak {:p}, handle {:p}", + // weak, context->handle); + + context->handle = nullptr; + context->handle_type = JNIInvalidRefType; + } + + env->DeleteWeakGlobalRef (weak); + } + } +} + +void GCBridge::bridge_processing () noexcept +{ + env = OSBridge::ensure_jnienv (); + + while (true) + { + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: waiting for semaphore..."); + bridge_processing_semaphore.acquire (); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: processing started"); + bridge_processing_started_callback (); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: taking unique lock..."); + std::unique_lock lock (processing_mutex); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: got unique lock"); + MarkCrossReferences cross_refs = GCBridge::shared_cross_refs; + + intptr_t gchandles = collect_gchandles_callback (&cross_refs); + + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: prepare_for_java_collection"); + prepare_for_java_collection (&cross_refs); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: trigger_java_gc"); + trigger_java_gc (); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: cleanup_after_java_collection"); + cleanup_after_java_collection (&cross_refs); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: bridge_processing_finished_callback"); + bridge_processing_finished_callback (&cross_refs, gchandles); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: done processing"); + } +} + +void GCBridge::mark_cross_references (MarkCrossReferences* cross_refs) noexcept +{ + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: mark cross references"); + + for (int i = 0; i < cross_refs->ComponentsLen; i++) + { + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Processing SCC at index {} with {} objects", i, scc->Count); + + for (int j = 0; j < scc->Count; j++) { + JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; + log_write_fmt (LOG_DEFAULT, LogLevel::Info, + "Context at index {}: handle {}, type {}, weak handle {}, refs {}", + j, (intptr_t)context->handle, context->handle_type, + (intptr_t)context->weak_handle, context->refs_added); + } } - trigger_java_gc (); + GCBridge::shared_cross_refs.ComponentsLen = cross_refs->ComponentsLen; + GCBridge::shared_cross_refs.Components = cross_refs->Components; + GCBridge::shared_cross_refs.CrossReferencesLen = cross_refs->CrossReferencesLen; + GCBridge::shared_cross_refs.CrossReferences = cross_refs->CrossReferences; - // Call back into managed code - bridge_processing_finish_callback (crossRefs); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: set semaphore"); + bridge_processing_semaphore.release (); + log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: exitting mark_cross_references"); } diff --git a/src/native/clr/host/internal-pinvokes.cc b/src/native/clr/host/internal-pinvokes.cc index bf5a0ba4918..5440f9171a0 100644 --- a/src/native/clr/host/internal-pinvokes.cc +++ b/src/native/clr/host/internal-pinvokes.cc @@ -37,10 +37,12 @@ bool clr_typemap_java_to_managed (const char *java_type_name, char const** assem return TypeMapper::typemap_java_to_managed (java_type_name, assembly_name, managed_type_token_id); } -MarkCrossReferencesFtn clr_initialize_gc_bridge (MarkCrossReferencesFtn callback) noexcept +BridgeProcessingFtn clr_initialize_gc_bridge ( + BridgeProcessingStartedFtn bridge_processing_started_callback, + CollectGCHandlesFtn collect_gchandles_callback, + BridgeProcessingFinishedFtn bridge_processing_finished_callback) noexcept { - GCBridge::set_finish_callback (callback); - return GCBridge::mark_cross_references; + return GCBridge::initialize_callback (bridge_processing_started_callback, collect_gchandles_callback, bridge_processing_finished_callback); } void monodroid_log (LogLevel level, LogCategories category, const char *message) noexcept @@ -165,7 +167,7 @@ void _monodroid_lref_log_delete (int lrefc, jobject handle, char type, const cha void _monodroid_gc_wait_for_bridge_processing () { - // mono_gc_wait_for_bridge_processing (); - replace with the new GC bridge call, when we have it + GCBridge::wait_for_bridge_processing (); } void _monodroid_detect_cpu_and_architecture (uint16_t *built_for_cpu, uint16_t *running_on_cpu, unsigned char *is64bit) diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index e472f733256..c84a7b8e41a 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -1,48 +1,107 @@ #pragma once #include +#include +#include +#include #include +struct JniObjectReferenceControlBlock +{ + jobject handle; + int handle_type; + jobject weak_handle; + int refs_added; +}; + struct StronglyConnectedComponent { - size_t Count; - void** ContextMemory; + ssize_t Count; + JniObjectReferenceControlBlock** ContextMemory; }; struct ComponentCrossReference { - size_t SourceGroupIndex; - size_t DestinationGroupIndex; + ssize_t SourceGroupIndex; + ssize_t DestinationGroupIndex; }; struct MarkCrossReferences { - size_t ComponentsLen; + ssize_t ComponentsLen; StronglyConnectedComponent* Components; - size_t CrossReferencesLen; + ssize_t CrossReferencesLen; ComponentCrossReference* CrossReferences; }; -using MarkCrossReferencesFtn = void (*)(MarkCrossReferences*); +using BridgeProcessingStartedFtn = void (*)(); +using CollectGCHandlesFtn = intptr_t (*)(MarkCrossReferences*); +using BridgeProcessingFinishedFtn = void (*)(MarkCrossReferences*, intptr_t); +using BridgeProcessingFtn = void (*)(MarkCrossReferences*); namespace xamarin::android { class GCBridge { public: + static void wait_for_bridge_processing () noexcept; static void initialize_on_load (JNIEnv *env) noexcept; - static void trigger_java_gc () noexcept; - static void mark_cross_references (MarkCrossReferences* crossRefs) noexcept; - - static void set_finish_callback (MarkCrossReferencesFtn callback) noexcept + static BridgeProcessingFtn initialize_callback ( + BridgeProcessingStartedFtn bridge_processing_started_callback, + CollectGCHandlesFtn collect_gchandles_callback, + BridgeProcessingFinishedFtn bridge_processing_finished_callback) noexcept { - abort_if_invalid_pointer_argument (callback, "callback"); - bridge_processing_finish_callback = callback; + abort_if_invalid_pointer_argument (bridge_processing_finished_callback, "bridge_processing_finished_callback"); + abort_unless (GCBridge::bridge_processing_started_callback == nullptr, "GC bridge processing started callback is already set"); + abort_unless (GCBridge::bridge_processing_finished_callback == nullptr, "GC bridge processing finished callback is already set"); + + GCBridge::bridge_processing_started_callback = bridge_processing_started_callback; + GCBridge::collect_gchandles_callback = collect_gchandles_callback; + GCBridge::bridge_processing_finished_callback = bridge_processing_finished_callback; + + bridge_processing_thread = new std::thread(GCBridge::bridge_processing); + bridge_processing_thread->detach (); + + return mark_cross_references; } private: - static inline jobject Runtime_instance = nullptr; - static inline jmethodID Runtime_gc = nullptr; - static inline MarkCrossReferencesFtn bridge_processing_finish_callback = nullptr; + + static inline BridgeProcessingStartedFtn bridge_processing_started_callback = nullptr; + static inline CollectGCHandlesFtn collect_gchandles_callback = nullptr; + static inline BridgeProcessingFinishedFtn bridge_processing_finished_callback = nullptr; + + static inline MarkCrossReferences shared_cross_refs; + static inline std::binary_semaphore bridge_processing_semaphore{0}; + + static inline JNIEnv* env = nullptr; + static inline std::shared_mutex processing_mutex; + static inline std::thread* bridge_processing_thread = nullptr; + + static void trigger_java_gc () noexcept; + static void bridge_processing () noexcept; + static void mark_cross_references (MarkCrossReferences* cross_refs) noexcept; + + static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; + static bool add_reference (JniObjectReferenceControlBlock *from, jobject to) noexcept; + static bool add_direct_reference (jobject from, jobject to) noexcept; // TODO naming + static void clear_references (jobject handle) noexcept; + static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; + static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, int index) noexcept; + static jobject get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; + static void maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept; + static void prepare_for_java_collection (MarkCrossReferences* cross_refs) noexcept; + static void cleanup_after_java_collection (MarkCrossReferences* cross_refs) noexcept; + + static inline jobject Runtime_instance = nullptr; + static inline jmethodID Runtime_gc = nullptr; + + static inline jclass GCUserPeer_class = nullptr; + static inline jmethodID GCUserPeer_ctor = nullptr; + + static inline jclass ArrayList_class = nullptr; + static inline jmethodID ArrayList_ctor = nullptr; + static inline jmethodID ArrayList_get = nullptr; + static inline jmethodID ArrayList_add = nullptr; }; } diff --git a/src/native/clr/include/host/os-bridge.hh b/src/native/clr/include/host/os-bridge.hh index be3291f4866..b418a0ec208 100644 --- a/src/native/clr/include/host/os-bridge.hh +++ b/src/native/clr/include/host/os-bridge.hh @@ -38,8 +38,11 @@ namespace xamarin::android { JNIEnv *env = nullptr; jvm->GetEnv ((void**)&env, JNI_VERSION_1_6); if (env == nullptr) { - // TODO: attach to the runtime thread here - jvm->GetEnv ((void**)&env, JNI_VERSION_1_6); + JavaVMAttachArgs args; + args.version = JNI_VERSION_1_6; + args.name = nullptr; + args.group = nullptr; + jvm->AttachCurrentThread(&env, &args); abort_unless (env != nullptr, "Unable to get a valid pointer to JNIEnv"); } diff --git a/src/native/clr/include/runtime-base/internal-pinvokes.hh b/src/native/clr/include/runtime-base/internal-pinvokes.hh index 5a01d121633..b0721e3a789 100644 --- a/src/native/clr/include/runtime-base/internal-pinvokes.hh +++ b/src/native/clr/include/runtime-base/internal-pinvokes.hh @@ -15,7 +15,10 @@ extern "C" { void _monodroid_gref_log_delete (jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable) noexcept; const char* clr_typemap_managed_to_java (const char *typeName, const uint8_t *mvid) noexcept; bool clr_typemap_java_to_managed (const char *java_type_name, char const** assembly_name, uint32_t *managed_type_token_id) noexcept; - MarkCrossReferencesFtn clr_initialize_gc_bridge (MarkCrossReferencesFtn callback) noexcept; + BridgeProcessingFtn clr_initialize_gc_bridge ( + BridgeProcessingStartedFtn bridge_processing_started_callback, + CollectGCHandlesFtn collect_gchandles_callback, + BridgeProcessingFinishedFtn mark_cross_references_callback) noexcept; void monodroid_log (xamarin::android::LogLevel level, LogCategories category, const char *message) noexcept; char* monodroid_TypeManager_get_java_class_name (jclass klass) noexcept; void monodroid_free (void *ptr) noexcept; From 3b976f692b0b3d61119be21ace2dc644fa947334 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 2 Jun 2025 22:38:15 +0200 Subject: [PATCH 02/17] Implement GC Bridge --- .vscode/settings.json | 58 ---- ...Microsoft.NETCore.App.Ref.10.0.0-dev.nupkg | 4 +- ...App.Runtime.android-arm64.10.0.0-dev.nupkg | 4 +- ...e.App.Runtime.android-x64.10.0.0-dev.nupkg | 3 - .../JavaInteropRuntime.cs | 5 +- .../Java.Interop/JreRuntime.cs | 2 +- .../Android.Runtime/AndroidRuntime.cs | 4 +- .../Android.Runtime/JNIEnvInit.cs | 8 +- .../Android.Runtime/RuntimeNativeMethods.cs | 5 +- src/Mono.Android/Java.Interop/Runtime.cs | 2 +- src/Mono.Android/Java.Lang/Object.cs | 6 +- .../ManagedValueManager.cs | 312 +++++++++++------- src/native/clr/host/gc-bridge.cc | 195 ++++++----- .../clr/host/generate-pinvoke-tables.cc | 1 + src/native/clr/host/internal-pinvokes.cc | 3 +- src/native/clr/host/pinvoke-tables.include | 8 +- src/native/clr/include/host/gc-bridge.hh | 41 +-- .../include/runtime-base/internal-pinvokes.hh | 1 - .../Java.Interop-Tests.NET.csproj | 2 +- 19 files changed, 327 insertions(+), 337 deletions(-) delete mode 100644 packages/Microsoft.NETCore.App.Runtime.android-x64.10.0.0-dev.nupkg diff --git a/.vscode/settings.json b/.vscode/settings.json index e14e0b5d537..485e3cde612 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -4,62 +4,4 @@ ], "cmake.configureOnOpen": false, "dotnetCoreExplorer.searchpatterns": "bin/Test{Debug}/**/net?.0/{Xamarin.Android.Build.Tests,MSBuildDeviceIntegration,Microsoft.Android.Sdk.Analysis.Tests}.dll", - "files.associations": { - "semaphore": "cpp", - "__bit_reference": "cpp", - "__hash_table": "cpp", - "__locale": "cpp", - "__node_handle": "cpp", - "__split_buffer": "cpp", - "__verbose_abort": "cpp", - "array": "cpp", - "bitset": "cpp", - "cctype": "cpp", - "charconv": "cpp", - "clocale": "cpp", - "cmath": "cpp", - "complex": "cpp", - "cstdarg": "cpp", - "cstdint": "cpp", - "cstdio": "cpp", - "cstdlib": "cpp", - "cstring": "cpp", - "ctime": "cpp", - "cwchar": "cpp", - "cwctype": "cpp", - "deque": "cpp", - "execution": "cpp", - "memory": "cpp", - "forward_list": "cpp", - "fstream": "cpp", - "initializer_list": "cpp", - "iomanip": "cpp", - "ios": "cpp", - "iosfwd": "cpp", - "iostream": "cpp", - "istream": "cpp", - "limits": "cpp", - "locale": "cpp", - "mutex": "cpp", - "new": "cpp", - "optional": "cpp", - "print": "cpp", - "queue": "cpp", - "ratio": "cpp", - "source_location": "cpp", - "span": "cpp", - "sstream": "cpp", - "stack": "cpp", - "stdexcept": "cpp", - "streambuf": "cpp", - "string": "cpp", - "string_view": "cpp", - "typeinfo": "cpp", - "unordered_map": "cpp", - "unordered_set": "cpp", - "variant": "cpp", - "vector": "cpp", - "algorithm": "cpp", - "shared_mutex": "cpp" - }, } diff --git a/packages/Microsoft.NETCore.App.Ref.10.0.0-dev.nupkg b/packages/Microsoft.NETCore.App.Ref.10.0.0-dev.nupkg index 962bb77cfd4..9196ebca16e 100644 --- a/packages/Microsoft.NETCore.App.Ref.10.0.0-dev.nupkg +++ b/packages/Microsoft.NETCore.App.Ref.10.0.0-dev.nupkg @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:5240fb5a42a71dc7b9e01f8d75c2251cf81ee6f4ffc571ce50ed0d2d9cb23d1f -size 5313568 +oid sha256:44171d975d5dd2cbaa8cf02a615a6610219c21aa36e08d4a28b2c9aea202b6fa +size 5327734 diff --git a/packages/Microsoft.NETCore.App.Runtime.android-arm64.10.0.0-dev.nupkg b/packages/Microsoft.NETCore.App.Runtime.android-arm64.10.0.0-dev.nupkg index 32d7885b4e4..58fa66afcd6 100644 --- a/packages/Microsoft.NETCore.App.Runtime.android-arm64.10.0.0-dev.nupkg +++ b/packages/Microsoft.NETCore.App.Runtime.android-arm64.10.0.0-dev.nupkg @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:8eb905f6f152401b6375d2e3c51ca753b2545717fd4cb89674f7ff188c3e86c7 -size 215752961 +oid sha256:8ebd400a5695b605981bb21c953fc8874ea7b4aa296edf5c0e8ba00f046b0489 +size 214997107 diff --git a/packages/Microsoft.NETCore.App.Runtime.android-x64.10.0.0-dev.nupkg b/packages/Microsoft.NETCore.App.Runtime.android-x64.10.0.0-dev.nupkg deleted file mode 100644 index 3a361daf778..00000000000 --- a/packages/Microsoft.NETCore.App.Runtime.android-x64.10.0.0-dev.nupkg +++ /dev/null @@ -1,3 +0,0 @@ -version https://git-lfs.github.com/spec/v1 -oid sha256:78a57312c28961bb0e36aea2ae337dc57141652b58723f0229014e13cb3826b0 -size 217468673 diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs index 340078864e8..98315cad67b 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs @@ -38,12 +38,11 @@ static void init (IntPtr jnienv, IntPtr klass, IntPtr classLoader) var settings = new DiagnosticSettings (); settings.AddDebugDotnetLog (); - var typeManager = new ManagedTypeManager (); var options = new NativeAotRuntimeOptions { EnvironmentPointer = jnienv, ClassLoader = new JniObjectReference (classLoader), - TypeManager = typeManager, - ValueManager = new ManagedValueManager (), + TypeManager = new ManagedTypeManager (), + ValueManager = ManagedValueManager.Instance, // TODO this will likely blow up in AOT at runtime currently UseMarshalMemberBuilder = false, JniGlobalReferenceLogWriter = settings.GrefLog, JniLocalReferenceLogWriter = settings.LrefLog, diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs index c660cbc325b..c5818f6d764 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs @@ -61,7 +61,7 @@ static NativeAotRuntimeOptions CreateJreVM (NativeAotRuntimeOptions builder) builder.TypeManager ??= new ManagedTypeManager (); #endif // NET - builder.ValueManager ??= new ManagedValueManager (); + builder.ValueManager ??= ManagedValueManager.Instance; builder.ObjectReferenceManager ??= new ManagedObjectReferenceManager (builder.JniGlobalReferenceLogWriter, builder.JniLocalReferenceLogWriter); if (builder.InvocationPointer != IntPtr.Zero || builder.EnvironmentPointer != IntPtr.Zero) diff --git a/src/Mono.Android/Android.Runtime/AndroidRuntime.cs b/src/Mono.Android/Android.Runtime/AndroidRuntime.cs index 3f6d8f86cf2..f6a1ee4e3c3 100644 --- a/src/Mono.Android/Android.Runtime/AndroidRuntime.cs +++ b/src/Mono.Android/Android.Runtime/AndroidRuntime.cs @@ -58,7 +58,7 @@ public override string GetCurrentManagedThreadStackTrace (int skipFrames, bool f { if (!reference.IsValid) return null; - var peeked = JNIEnvInit.ValueManager?.PeekPeer (reference); + var peeked = JniEnvironment.Runtime.ValueManager.PeekPeer (reference); var peekedExc = peeked as Exception; if (peekedExc == null) { var throwable = Java.Lang.Object.GetObject (reference.Handle, JniHandleOwnership.DoNotTransfer); @@ -66,7 +66,7 @@ public override string GetCurrentManagedThreadStackTrace (int skipFrames, bool f return throwable; } JniObjectReference.Dispose (ref reference, options); - var unwrapped = JNIEnvInit.ValueManager?.PeekValue (peeked!.PeerReference) as Exception; + var unwrapped = JniEnvironment.Runtime.ValueManager.PeekValue (peeked!.PeerReference) as Exception; if (unwrapped != null) { return unwrapped; } diff --git a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs index ad97a401a32..84dc08aa9c6 100644 --- a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs +++ b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs @@ -40,7 +40,6 @@ internal struct JnienvInitializeArgs { } #pragma warning restore 0649 - internal static JniRuntime.JniValueManager? ValueManager; internal static bool IsRunningOnDesktop; internal static bool jniRemappingInUse; internal static bool MarshalMethodsEnabled; @@ -88,7 +87,6 @@ static Type TypeGetType (string typeName) => internal static void InitializeJniRuntime (JniRuntime runtime) { androidRuntime = runtime; - ValueManager = runtime.ValueManager; SetSynchronizationContext (); } @@ -115,11 +113,12 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args) JniRuntime.JniValueManager valueManager; if (RuntimeFeature.ManagedTypeMap) { typeManager = new ManagedTypeManager (); - valueManager = new ManagedValueManager (); } else { typeManager = new AndroidTypeManager (args->jniAddNativeMethodRegistrationAttributePresent != 0); - valueManager = RuntimeType == DotNetRuntimeType.MonoVM ? new AndroidValueManager () : new ManagedValueManager (); } + // TODO is there any reason why the ManagedTypeMap would need ManagedValueManager? + // ManagedValueManager is specifically tied to the CoreCLR JavaMarshal APIs and it does not work with MonoVM. + valueManager = RuntimeType == DotNetRuntimeType.MonoVM ? new AndroidValueManager () : ManagedValueManager.Instance; androidRuntime = new AndroidRuntime ( args->env, args->javaVm, @@ -128,7 +127,6 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args) valueManager, args->jniAddNativeMethodRegistrationAttributePresent != 0 ); - ValueManager = androidRuntime.ValueManager; IsRunningOnDesktop = args->isRunningOnDesktop == 1; diff --git a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs index affb37ef62a..a691ab8498a 100644 --- a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs +++ b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs @@ -100,10 +100,9 @@ internal unsafe static class RuntimeNativeMethods /// A function pointer to a C# callback that will be invoked when bridge processing has completed. /// A function pointer that should be passed to JavaMarshal.Initialize() on startup. [DllImport (RuntimeConstants.InternalDllName, CallingConvention = CallingConvention.Cdecl)] - internal static extern delegate* unmanaged clr_initialize_gc_bridge ( + internal static extern delegate* unmanaged clr_initialize_gc_bridge ( delegate* unmanaged bridge_processing_started_callback, - delegate* unmanaged collect_gchandles, - delegate* unmanaged bridge_processing_finished_callback + delegate* unmanaged bridge_processing_finished_callback ); [MethodImplAttribute(MethodImplOptions.InternalCall)] diff --git a/src/Mono.Android/Java.Interop/Runtime.cs b/src/Mono.Android/Java.Interop/Runtime.cs index 8d69aedffa4..314815a9edb 100644 --- a/src/Mono.Android/Java.Interop/Runtime.cs +++ b/src/Mono.Android/Java.Interop/Runtime.cs @@ -11,7 +11,7 @@ public static class Runtime { [Obsolete ("Please use Java.Interop.JniEnvironment.Runtime.ValueManager.GetSurfacedPeers()")] public static List GetSurfacedObjects () { - var peers = JNIEnvInit.ValueManager!.GetSurfacedPeers (); + var peers = JniEnvironment.Runtime.ValueManager.GetSurfacedPeers (); var r = new List (peers.Count); foreach (var p in peers) { if (p.SurfacedPeer.TryGetTarget (out var target)) diff --git a/src/Mono.Android/Java.Lang/Object.cs b/src/Mono.Android/Java.Lang/Object.cs index 58a7e54d88b..44c9519fb4d 100644 --- a/src/Mono.Android/Java.Lang/Object.cs +++ b/src/Mono.Android/Java.Lang/Object.cs @@ -111,7 +111,7 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer) { var reference = new JniObjectReference (value); var options = FromJniHandleOwnership (transfer); - JNIEnvInit.ValueManager?.ConstructPeer ( + JniEnvironment.Runtime.ValueManager.ConstructPeer ( this, ref reference, value == IntPtr.Zero ? JniObjectReferenceOptions.None : options); @@ -128,7 +128,7 @@ static JniObjectReferenceOptions FromJniHandleOwnership (JniHandleOwnership tran internal static IJavaPeerable? PeekObject (IntPtr handle, Type? requiredType = null) { - var peeked = JNIEnvInit.ValueManager?.PeekPeer (new JniObjectReference (handle)); + var peeked = JniEnvironment.Runtime.ValueManager.PeekPeer (new JniObjectReference (handle)); if (peeked == null) return null; if (requiredType != null && !requiredType.IsAssignableFrom (peeked.GetType ())) @@ -180,7 +180,7 @@ internal static T? _GetObject< if (handle == IntPtr.Zero) return null; - var r = JNIEnvInit.ValueManager!.GetPeer (new JniObjectReference (handle), type); + var r = JniEnvironment.Runtime.ValueManager.GetPeer (new JniObjectReference (handle), type); JNIEnv.DeleteRef (handle, transfer); return r; } diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 54990e50cda..922200ba531 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -23,18 +23,21 @@ class ManagedValueManager : JniRuntime.JniValueManager Dictionary>? RegisteredInstances = new (); - internal unsafe ManagedValueManager () + private static Lazy s_instance = new (() => new ManagedValueManager ()); + public static ManagedValueManager Instance => s_instance.Value; + + private unsafe ManagedValueManager () { + // There can only be one instance of ManagedValueManager because we can call JavaMarshal.Initialize only once. var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge ( &BridgeProcessingStarted, - &CollectGCHandles, &BridgeProcessingFinished); JavaMarshal.Initialize (mark_cross_references_ftn); } - public override void WaitForGCBridgeProcessing() + public override void WaitForGCBridgeProcessing () { - // AndroidRuntimeInternal.WaitForGCBridgeProcessing(); // TODO + AndroidRuntimeInternal.WaitForBridgeProcessing (); } public override void CollectPeers () @@ -55,7 +58,7 @@ public override void CollectPeers () List? exceptions = null; foreach (var peer in peers) { try { - if (peer.Target is IDisposable disposable) + if (TryGetTarget (peer, out IDisposable? disposable)) disposable.Dispose (); } catch (Exception e) { @@ -65,6 +68,8 @@ public override void CollectPeers () } if (exceptions != null) throw new AggregateException ("Exceptions while collecting peers.", exceptions); + + GC.Collect (); } public override void AddPeer (IJavaPeerable value) @@ -72,6 +77,8 @@ public override void AddPeer (IJavaPeerable value) if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); + WaitForGCBridgeProcessing (); + var r = value.PeerReference; if (!r.IsValid) throw new ObjectDisposedException (value.GetType ().FullName); @@ -84,6 +91,7 @@ public override void AddPeer (IJavaPeerable value) lock (RegisteredInstances) { List? peers; if (!RegisteredInstances.TryGetValue (key, out peers)) { + // Console.WriteLine ($"Adding new peer list for key {key} with PeerReference={value.PeerReference} in {GetType ().AssemblyQualifiedName}"); peers = new List () { CreateReferenceTrackingHandle (value) }; @@ -93,24 +101,82 @@ public override void AddPeer (IJavaPeerable value) for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - if (p.Target is not IJavaPeerable peer) + if (!TryGetTarget (p, out IJavaPeerable? peer)) continue; if (!JniEnvironment.Types.IsSameObject (peer.PeerReference, value.PeerReference)) continue; if (Replaceable (p)) { + FreeReferenceTrackingHandle (p); peers [i] = CreateReferenceTrackingHandle (value); } else { WarnNotReplacing (key, value, peer); } return; } + peers.Add (CreateReferenceTrackingHandle (value)); } } + public override void DisposePeer (IJavaPeerable value) + { + if (value == null) + throw new ArgumentNullException (nameof (value)); + + if (RegisteredInstances == null) + throw new ObjectDisposedException (nameof (ManagedValueManager)); + + WaitForGCBridgeProcessing (); + + int key = value.JniIdentityHashCode; + + lock (RegisteredInstances) { + List? peers; + if (!RegisteredInstances.TryGetValue (key, out peers)) + return; + + for (int i = peers.Count - 1; i >= 0; i--) { + var p = peers [i]; + + if (TryGetTarget (p, out IJavaPeerable? peer) + && ReferenceEquals (value, peer)) { + FreeReferenceTrackingHandle (p); + peers.RemoveAt (i); + GC.KeepAlive (peer); + } + } + if (peers.Count == 0) + RegisteredInstances.Remove (key); + } + + base.DisposePeer (value); + } + + public override void DisposePeerUnlessReferenced (IJavaPeerable value) + { + if (RegisteredInstances == null) + throw new ObjectDisposedException (GetType ().Name); + + if (value == null) + throw new ArgumentNullException (nameof (value)); + + var h = value.PeerReference; + if (!h.IsValid) + return; + + var o = PeekPeer (h); + if (o != null && object.ReferenceEquals (o, value)) + return; + + // This is the only difference from base.DisposePeerUnlessReferenced: + // - we're calling the virtual `DisposePeer` instead of the private + // one in the base class + DisposePeer (value); + } + static bool Replaceable (GCHandle handle) { - if (handle.Target is not IJavaPeerable peer) + if (!TryGetTarget (handle, out IJavaPeerable? peer)) return true; return peer.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable); } @@ -139,6 +205,8 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal if (!reference.IsValid) return null; + WaitForGCBridgeProcessing (); + int key = GetJniIdentityHashCode (reference); lock (RegisteredInstances) { @@ -148,7 +216,7 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - if (p.Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) + if (TryGetTarget (p, out IJavaPeerable? peer) && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) return peer; } if (peers.Count == 0) @@ -157,60 +225,77 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal return null; } - private GCHandle PeekPeerHandle (JniObjectReference reference) + public override void RemovePeer (IJavaPeerable value) { if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); - if (!reference.IsValid) - return default; + if (value == null) + throw new ArgumentNullException (nameof (value)); - int key = GetJniIdentityHashCode (reference); + WaitForGCBridgeProcessing (); + RemoveRegisteredInstance (value, freeHandle: true); + } + + private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) + { + if (RegisteredInstances == null) + throw new ObjectDisposedException (nameof (ManagedValueManager)); + + int key = target.JniIdentityHashCode; lock (RegisteredInstances) { List? peers; if (!RegisteredInstances.TryGetValue (key, out peers)) - return default; + return; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (p.Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) - return p; + var p = peers [i]; + if (TryGetTarget (p, out IJavaPeerable? peer) && ReferenceEquals (target, peer)) { + peers.RemoveAt (i); + if (freeHandle) + FreeReferenceTrackingHandle (p); + } } if (peers.Count == 0) RegisteredInstances.Remove (key); } - return default; } - public override void RemovePeer (IJavaPeerable value) + private GCHandle PeekPeerHandle (JniObjectReference reference) { if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); - if (value == null) - throw new ArgumentNullException (nameof (value)); + if (!reference.IsValid) + return default; + + WaitForGCBridgeProcessing (); + + int key = GetJniIdentityHashCode (reference); - int key = value.JniIdentityHashCode; lock (RegisteredInstances) { List? peers; if (!RegisteredInstances.TryGetValue (key, out peers)) - return; + return default; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (object.ReferenceEquals (value, p.Target)) { - peers.RemoveAt (i); - FreeHandle (p); + var p = peers[i]; + if (TryGetTarget (p, out IJavaPeerable? peer) + && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) { + return p; } } if (peers.Count == 0) RegisteredInstances.Remove (key); } + return default; } public override void FinalizePeer (IJavaPeerable value) { + WaitForGCBridgeProcessing (); + var h = value.PeerReference; var o = Runtime.ObjectReferenceManager; // MUST NOT use SafeHandle.ReferenceType: local refs are tied to a JniEnvironment @@ -224,6 +309,7 @@ public override void FinalizePeer (IJavaPeerable value) value.JniIdentityHashCode.ToString ("x", CultureInfo.InvariantCulture), RuntimeHelpers.GetHashCode (value).ToString ("x", CultureInfo.InvariantCulture), value.GetType ().ToString ()); + } RemovePeer (value); value.SetPeerReference (new JniObjectReference ()); @@ -246,6 +332,8 @@ public override void FinalizePeer (IJavaPeerable value) public override void ActivatePeer (IJavaPeerable? self, JniObjectReference reference, ConstructorInfo cinfo, object?[]? argumentValues) { + WaitForGCBridgeProcessing (); + try { ActivateViaReflection (reference, cinfo, argumentValues); } catch (Exception e) { @@ -284,11 +372,13 @@ public override List GetSurfacedPeers () if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); + WaitForGCBridgeProcessing (); + lock (RegisteredInstances) { var peers = new List (RegisteredInstances.Count); foreach (var e in RegisteredInstances) { foreach (var p in e.Value) { - if (p.Target is not IJavaPeerable peer) + if (!TryGetTarget (p, out IJavaPeerable? peer)) continue; peers.Add (new JniSurfacedPeerInfo (e.Key, new WeakReference (peer))); } @@ -297,136 +387,102 @@ public override List GetSurfacedPeers () } } - // private unsafe struct HandleContext - // { - // public IntPtr ControlBlock; - // public IntPtr Handle; - - // public static HandleContext* Alloc(IntPtr controlBlock) - // { - // var size = (uint)Marshal.SizeOf(); - // var ctx = (HandleContext*)NativeMemory.AllocZeroed(1, size); - // ctx->ControlBlock = controlBlock; - // return ctx; - // } - - // public static void Free(HandleContext* ctx) - // { - // if (ctx->ControlBlock != IntPtr.Zero) { - // NativeMemory.Free((void*)ctx->ControlBlock); - // } - - // NativeMemory.Free((void*)ctx); - // } - // } - - static unsafe GCHandle CreateReferenceTrackingHandle(IJavaPeerable value) + [StructLayout (LayoutKind.Sequential)] + unsafe struct HandleContext { - // JniObjectReferenceControlBlock* controlBlock = (JniObjectReferenceControlBlock*)value.JniObjectReferenceControlBlock; - // Console.WriteLine($"Creating reference tracking handle for {value.GetType().FullName} with JniObjectReferenceControlBlock: {controlBlock->handle}, type: {controlBlock->handle_type}, weak_handle: {controlBlock->weak_handle}, refs_added: {controlBlock->refs_added}"); + public IntPtr GCHandle; + public bool IsCollected; + public IntPtr ControlBlock; - return JavaMarshal.CreateReferenceTrackingHandle(value, value.JniObjectReferenceControlBlock); - } + static readonly nuint Size = (nuint) Marshal.SizeOf (); - static unsafe void FreeHandle(GCHandle handle) - { - Console.WriteLine($"Freeing handle"); - IntPtr context = JavaMarshal.GetContext(handle); - if (context != IntPtr.Zero) + public static HandleContext* AllocZeroed () { - var ctx = (JniObjectReferenceControlBlock*)context; - Console.WriteLine($"Freeing handle with JniObjectReferenceControlBlock: {ctx->handle}, type: {ctx->handle_type}, weak_handle: {ctx->weak_handle}, refs_added: {ctx->refs_added}"); - NativeMemory.Free((void*)context); + return (HandleContext*)NativeMemory.AllocZeroed (1, Size); + } + + public static void Free (ref HandleContext* ctx) + { + if (ctx == null) + return; + + NativeMemory.Free (ctx); + ctx = null; } } - [UnmanagedCallersOnly] - internal static void BridgeProcessingStarted() + static unsafe void FreeReferenceTrackingHandle (GCHandle handle) { - AndroidRuntimeInternal.BridgeProcessing = true; + var context = (HandleContext*) JavaMarshal.GetContext (handle); + handle.Free (); + HandleContext.Free (ref context); } - [UnmanagedCallersOnly] - internal static unsafe IntPtr CollectGCHandles(MarkCrossReferences* mcr) + static unsafe GCHandle CreateReferenceTrackingHandle (IJavaPeerable value) { - Console.WriteLine($"CollectGCHandles (mcr.ComponentsLen={mcr->ComponentsLen})"); + var ctx = HandleContext.AllocZeroed (); + var handle = JavaMarshal.CreateReferenceTrackingHandle (value, ctx); - List handles = []; - for (int i = 0; i < mcr->ComponentsLen; i++) - { - for (int j = 0; j < mcr->Components[i].Count; j++) - { - Console.WriteLine($"CollectGCHandles i={i} j={j}"); - - var ctx = mcr->Components[i].Context[j]; - if (ctx == IntPtr.Zero) - { - Console.WriteLine($"CollectGCHandles: controlBlock->handle is zero, skipping"); - handles.Add(default); - } - else - { - var controlBlock = (JniObjectReferenceControlBlock*)ctx; - var reference = new JniObjectReference(controlBlock->handle, (JniObjectReferenceType)controlBlock->handle_type); - Console.WriteLine($"CollectGCHandles: controlBlock->handle={controlBlock->handle}, type={controlBlock->handle_type}, weak_handle={controlBlock->weak_handle}, refs_added={controlBlock->refs_added}, reference={reference}"); - GCHandle handle = ((ManagedValueManager)AndroidRuntime.CurrentRuntime.ValueManager).PeekPeerHandle(reference); - Console.WriteLine($"CollectGCHandles: PeekPeerHandle returned {handle.IsAllocated} for reference {reference}"); - handles.Add(handle); - } + ctx->GCHandle = GCHandle.ToIntPtr (handle); + ctx->IsCollected = false; + ctx->ControlBlock = value.JniObjectReferenceControlBlock; - } - } + return handle; + } - Console.WriteLine($"CollectGCHandles: collected {handles.Count} handles"); + // TODO: The main reason this method is necessary is that there is a SIGSEGV when we access + // handle.Target of a reference tracking handle when its context is null + static unsafe bool TryGetTarget (GCHandle handle, [NotNullWhen (true)] out T? target) + where T : class + { + target = null; - return GCHandle.ToIntPtr(GCHandle.Alloc(handles)); + if (handle.IsAllocated && JavaMarshal.GetContext (handle) != null) { + target = handle.Target as T; + } + + return target is not null; } [UnmanagedCallersOnly] - internal static unsafe void BridgeProcessingFinished(MarkCrossReferences* mcr, IntPtr handles) + internal static void BridgeProcessingStarted () { - Console.WriteLine($"BridgeProcessingFinished (mcr.ComponentsLen={mcr->ComponentsLen})"); - List? originalHandles = GCHandle.FromIntPtr(handles).Target as List; - if (originalHandles is null) - { - Console.WriteLine($"BridgeProcessingFinished: invalid handles {handles}, target={GCHandle.FromIntPtr(handles).Target}"); - throw new InvalidOperationException($"Invalid GCHandles collection"); - } + AndroidRuntimeInternal.BridgeProcessing = true; + } + [UnmanagedCallersOnly] + internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* mcr) + { List handlesToFree = []; - for (int i = 0; i < mcr->ComponentsLen; i++) - { - for (int j = 0; j < mcr->Components[i].Count; j++) - { - Console.WriteLine($"BridgeProcessingFinished i={i} j={j}"); - - var controlBlock = (JniObjectReferenceControlBlock*)mcr->Components[i].Context[j]; - if (controlBlock->handle == IntPtr.Zero) - { - Console.WriteLine($"BridgeProcessingFinished: controlBlock->handle is zero, skipping"); - - // TODO figure out how to get the GCHandle here - // GCHandle handle = PeekGCHandle(new JniObjectReference(controlBlock->handle, controlBlock->handle_type)); - // if (handle.IsAllocated && handle.Target is IJavaPeerable peer) - // { - // Console.WriteLine($"BridgeProcessingFinished: handle for {peer.GetType().FullName} will be freed"); - // handlesToFree.Add(handle); - // JniObjectReferenceControlBlock.Free(ref controlBlock); - // } + for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { + for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) { + var context = (HandleContext*) mcr->Components [i].Contexts [j]; + if (context->IsCollected) { + var handle = GCHandle.FromIntPtr (context->GCHandle); + + // Only free handles that haven't been freed yet + if (handle.IsAllocated && JavaMarshal.GetContext (handle) != null) { + handlesToFree.Add (handle); + } + + // Cleanup: Remove the handle from RegisteredInstances + if (TryGetTarget (handle, out IJavaPeerable? target)) { + Instance.RemoveRegisteredInstance (target, freeHandle: false); + } + + HandleContext.Free (ref context); } } } - Console.WriteLine($"BridgeProcessingFinished: freeing {handlesToFree.Count} handles"); - - JavaMarshal.FinishCrossReferenceProcessing(mcr, CollectionsMarshal.AsSpan(handlesToFree)); + JavaMarshal.FinishCrossReferenceProcessing (mcr, CollectionsMarshal.AsSpan (handlesToFree)); AndroidRuntimeInternal.BridgeProcessing = false; } const BindingFlags ActivationConstructorBindingFlags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance; - static readonly Type[] XAConstructorSignature = new Type[] { typeof(IntPtr), typeof(JniHandleOwnership) }; + static readonly Type[] XAConstructorSignature = new Type[] { typeof (IntPtr), typeof (JniHandleOwnership) }; protected override bool TryConstructPeer ( IJavaPeerable self, @@ -435,6 +491,8 @@ protected override bool TryConstructPeer ( [DynamicallyAccessedMembers (Constructors)] Type type) { + WaitForGCBridgeProcessing (); + var c = type.GetConstructor (ActivationConstructorBindingFlags, null, XAConstructorSignature, null); if (c != null) { var args = new object[] { @@ -450,6 +508,8 @@ protected override bool TryConstructPeer ( protected override bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (true)]out object? result) { + WaitForGCBridgeProcessing (); + var proxy = value as JavaProxyThrowable; if (proxy != null) { result = proxy.InnerException; diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index a3959dfcf46..62b476d3ac4 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -6,16 +6,14 @@ using namespace xamarin::android; void GCBridge::wait_for_bridge_processing () noexcept { - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing..."); + // log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing - waiting to acquire processing mutex"); std::shared_lock lock (processing_mutex); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing done"); + // log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing - acquired processing mutex"); } void GCBridge::initialize_on_load (JNIEnv *env) noexcept { - // abort_if_invalid_pointer_argument ("env", env); - - log_write (LOG_DEFAULT, LogLevel::Info, "Initializing GC bridge"); + abort_if_invalid_pointer_argument (env, "env"); jclass lref = env->FindClass ("java/lang/Runtime"); jmethodID Runtime_getRuntime = env->GetStaticMethodID (lref, "getRuntime", "()Ljava/lang/Runtime;"); @@ -27,29 +25,25 @@ void GCBridge::initialize_on_load (JNIEnv *env) noexcept Runtime_gc != nullptr && Runtime_instance != nullptr, "Failed to look up Java GC runtime API." ); - - log_write (LOG_DEFAULT, LogLevel::Info, "Initialized GC bridge"); } void GCBridge::trigger_java_gc () noexcept { - log_write (LOG_DEFAULT, LogLevel::Info, "Triggering Java GC"); - env->CallVoidMethod (Runtime_instance, Runtime_gc); - if (env->ExceptionCheck ()) [[unlikely]] { env->ExceptionDescribe (); env->ExceptionClear (); log_error (LOG_DEFAULT, "Java GC failed"); - } else { - log_write (LOG_DEFAULT, LogLevel::Info, "Java GC triggered"); } } -bool GCBridge::add_reference (JniObjectReferenceControlBlock *from, jobject to) noexcept +bool GCBridge::add_reference (HandleContext *from, jobject to) noexcept { - if (add_direct_reference (from->handle, to)) { - from->refs_added++; + abort_if_invalid_pointer_argument (from, "from"); + abort_if_invalid_pointer_argument (to, "to"); + + if (add_direct_reference (from->control_block->handle, to)) { + from->control_block->refs_added++; return true; } else { return false; @@ -58,6 +52,9 @@ bool GCBridge::add_reference (JniObjectReferenceControlBlock *from, jobject to) bool GCBridge::add_direct_reference (jobject from, jobject to) noexcept { + abort_if_invalid_pointer_argument (from, "from"); + abort_if_invalid_pointer_argument (to, "to"); + jclass java_class = env->GetObjectClass (from); jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); @@ -91,12 +88,19 @@ bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept return scc->Count < 0; // If Count is negative, it's a bridgeless SCC } +static bool is_valid_gref (JniObjectReferenceControlBlock *control_block) noexcept +{ + return control_block != nullptr + && control_block->handle != nullptr + && control_block->handle_type == JNIGlobalRefType; +} + jobject GCBridge::get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); if (scc->Count > 0) { - return scc->ContextMemory [0]->handle; // Return the first context's handle + return scc->Contexts [0]->control_block->handle; // Return the first valid global reference } else { abort_unless (temporary_peers != nullptr, "Temporary peers must not be null for bridgeless SCCs"); @@ -108,17 +112,14 @@ jobject GCBridge::get_scc_representative (StronglyConnectedComponent *scc, jobje void GCBridge::maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); - abort_if_invalid_pointer_argument (handle, "handle"); if (scc->Count < 0) { env->DeleteLocalRef (handle); // Release the local ref for bridgeless SCCs returned from get_scc_representative } } -void GCBridge::prepare_for_java_collection (MarkCrossReferences* cross_refs) noexcept +void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { - log_write_fmt (LOG_DEFAULT, LogLevel::Info, "GCBridge::prepare_for_java_collection called: {} components", cross_refs->ComponentsLen); - // Some SCCs might have no IGCUserPeers associated with them, so we must create one jobject temporary_peers = nullptr; // This is an ArrayList int temporary_peer_count = 0; // Number of items in temporary_peers @@ -130,26 +131,20 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferences* cross_refs) noe { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Processing SCC at index {} with {} objects", i, scc->Count); - // Count > 1 case: The SCC contains many objects which must be collected as one. // Solution: Make all objects within the SCC directly or indirectly reference each other if (scc->Count > 1) { - JniObjectReferenceControlBlock *first = scc->ContextMemory [0]; - JniObjectReferenceControlBlock *prev = first; + HandleContext *first = scc->Contexts [0]; + HandleContext *prev = first; for (int j = 1; j < scc->Count; j++) { - JniObjectReferenceControlBlock *current = scc->ContextMemory [j]; + HandleContext *current = scc->Contexts [j]; - // TODO are those handles correct? in the mono GC bridge, there is a more complex logic that gets the handles - // from the context block data... maybe in that case the handles are GCHandles? - add_reference (prev, current->handle); + add_reference (prev, current->control_block->handle); prev = current; } - // ref the first from the final - add_reference (prev, first->handle); + add_reference (prev, first->control_block->handle); } else if (scc->Count == 0) { log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Creating temporary peer for SCC at index {} with no bridge objects", i); @@ -202,13 +197,10 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferences* cross_refs) noe maybe_release_scc_representative (from_scc, from_handle); maybe_release_scc_representative (to_scc, to_handle); } else { - JniObjectReferenceControlBlock *from_cb = from_scc->ContextMemory [0]; + HandleContext *from = from_scc->Contexts [0]; jobject to_handle = get_scc_representative (to_scc, temporary_peers); - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Adding cross-reference from SCC {} to SCC {}: {} -> {}", - // xref->SourceGroupIndex, xref->DestinationGroupIndex, from_handle, to_handle); - add_reference (from_cb, to_handle); + add_reference (from, to_handle); maybe_release_scc_representative (to_scc, to_handle); } @@ -227,26 +219,27 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferences* cross_refs) noe } for (int j = 0; j < scc->Count; j++) { - JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; - - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Creating weak global ref for: handle {}, type {}, weak handle {}, refs {}", - (intptr_t)context->handle, - context->handle_type, - (intptr_t)context->weak_handle, - context->refs_added); + HandleContext *context = scc->Contexts [j]; - jobject handle = context->handle; + jobject handle = context->control_block->handle; + + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Switching global reference to weak for context {:x}, gchandle: {}, handle {:x}, refs added {}", + // (intptr_t)context, (intptr_t)context->gc_handle, (intptr_t)handle, context->control_block->refs_added); + jobject weak = env->NewWeakGlobalRef (handle); // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Creating weak global ref for handle {:p} in context {:p}: weak {:p}", - // handle, context, weak); + // "Creating weak global ref for: gc_handle {}, handle {:x}, weak handle {:x}, refs {}", + // (intptr_t)context->gc_handle, + // (intptr_t)handle, + // (intptr_t)weak, + // context->control_block->refs_added); - env->DeleteGlobalRef (handle); // Delete the strong reference after creating the weak one + env->DeleteGlobalRef (handle); // Delete the strong reference now that we have a weak one - context->handle = weak; - context->handle_type = JNIWeakGlobalRefType; + context->control_block->handle = weak; + context->control_block->handle_type = JNIWeakGlobalRefType; } } } @@ -266,7 +259,7 @@ void GCBridge::clear_references (jobject handle) noexcept env->DeleteLocalRef (java_class); // Clean up the local reference to the class } -void GCBridge::cleanup_after_java_collection (MarkCrossReferences* cross_refs) noexcept +void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { // try to switch back to global refs to analyze what stayed alive for (int i = 0; i < cross_refs->ComponentsLen; i++) @@ -274,96 +267,94 @@ void GCBridge::cleanup_after_java_collection (MarkCrossReferences* cross_refs) n StronglyConnectedComponent *scc = &cross_refs->Components [i]; for (int j = 0; j < scc->Count; j++) { - JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; + HandleContext *context = scc->Contexts [j]; + + jobject weak = context->control_block->handle; + + // TODO remove this log + if (context->control_block->handle_type != JNIWeakGlobalRefType) { + log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Skipping non-weak global ref: {:x} ({}), gchandle: {} 0x{:x}", (intptr_t)weak, context->control_block->handle_type, (intptr_t)context->gc_handle, (intptr_t)context->gc_handle); + } - jobject weak = context->handle; + abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, + "Expected weak global reference type for handle"); jobject global = env->NewGlobalRef (weak); if (global) { - // Object survived Java GC, so we need to update the handle - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Object survived Java GC: handle {:p}, weak {:p}, global {:p}", - // context->handle, weak, global); + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Object survived Java GC: weak {:x} -> gref {:x}", (intptr_t)weak, (intptr_t)global); + env->DeleteWeakGlobalRef (weak); - context->handle = global; - context->handle_type = JNIGlobalRefType; + context->control_block->handle = global; + context->control_block->handle_type = JNIGlobalRefType; - if (context->refs_added > 0) { + if (context->control_block->refs_added > 0) { // Clear references - clear_references (context->handle); + clear_references (context->control_block->handle); // Reset refs_added - context->refs_added = 0; + context->control_block->refs_added = 0; } } else { // Object was collected by Java GC - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Object was collected by Java GC: weak {:p}, handle {:p}", - // weak, context->handle); + context->is_collected = 1; - context->handle = nullptr; - context->handle_type = JNIInvalidRefType; + // log_write_fmt (LOG_DEFAULT, LogLevel::Info, + // "Object was collected by Java GC: weak {:x}, gchandle: {}, control block {:x}, handle type {}", + // (intptr_t)weak, (intptr_t)context->gc_handle, (intptr_t)context->control_block, context->control_block->handle_type); } - - env->DeleteWeakGlobalRef (weak); } } } void GCBridge::bridge_processing () noexcept { + abort_unless (bridge_processing_started_callback != nullptr, "GC bridge processing started callback is not set"); + abort_unless (bridge_processing_finished_callback != nullptr, "GC bridge processing finished callback is not set"); + env = OSBridge::ensure_jnienv (); while (true) { - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: waiting for semaphore..."); bridge_processing_semaphore.acquire (); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: processing started"); - bridge_processing_started_callback (); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: taking unique lock..."); std::unique_lock lock (processing_mutex); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: got unique lock"); - MarkCrossReferences cross_refs = GCBridge::shared_cross_refs; - - intptr_t gchandles = collect_gchandles_callback (&cross_refs); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: prepare_for_java_collection"); - prepare_for_java_collection (&cross_refs); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: trigger_java_gc"); + bridge_processing_started_callback (); + prepare_for_java_collection (&GCBridge::shared_cross_refs); trigger_java_gc (); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: cleanup_after_java_collection"); - cleanup_after_java_collection (&cross_refs); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: bridge_processing_finished_callback"); - bridge_processing_finished_callback (&cross_refs, gchandles); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: done processing"); + cleanup_after_java_collection (&GCBridge::shared_cross_refs); + bridge_processing_finished_callback (&GCBridge::shared_cross_refs); } } -void GCBridge::mark_cross_references (MarkCrossReferences* cross_refs) noexcept +static void assert_valid_handles (MarkCrossReferencesArgs* cross_refs) noexcept { - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: mark cross references"); - for (int i = 0; i < cross_refs->ComponentsLen; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Processing SCC at index {} with {} objects", i, scc->Count); - for (int j = 0; j < scc->Count; j++) { - JniObjectReferenceControlBlock *context = scc->ContextMemory [j]; - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Context at index {}: handle {}, type {}, weak handle {}, refs {}", - j, (intptr_t)context->handle, context->handle_type, - (intptr_t)context->weak_handle, context->refs_added); + HandleContext *context = scc->Contexts [j]; + if (!is_valid_gref (context->control_block)) { + log_error_fmt (LOG_DEFAULT, "Invalid global reference in SCC context {:x}, gchandle: {}, control block {}, handle {}, handle type {}", + (intptr_t)context, context->gc_handle, (intptr_t)context->control_block, (intptr_t)context->control_block->handle, context->control_block->handle_type); + } + abort_unless (is_valid_gref (context->control_block), "Invalid global reference in SCC"); } } +} + +void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept +{ + // TODO get rid of this + assert_valid_handles (cross_refs); - GCBridge::shared_cross_refs.ComponentsLen = cross_refs->ComponentsLen; - GCBridge::shared_cross_refs.Components = cross_refs->Components; - GCBridge::shared_cross_refs.CrossReferencesLen = cross_refs->CrossReferencesLen; - GCBridge::shared_cross_refs.CrossReferences = cross_refs->CrossReferences; + { + std::unique_lock lock (processing_mutex); + + GCBridge::shared_cross_refs.ComponentsLen = cross_refs->ComponentsLen; + GCBridge::shared_cross_refs.Components = cross_refs->Components; + GCBridge::shared_cross_refs.CrossReferencesLen = cross_refs->CrossReferencesLen; + GCBridge::shared_cross_refs.CrossReferences = cross_refs->CrossReferences; + } - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: set semaphore"); bridge_processing_semaphore.release (); - log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: exitting mark_cross_references"); } diff --git a/src/native/clr/host/generate-pinvoke-tables.cc b/src/native/clr/host/generate-pinvoke-tables.cc index cbf8bb01d19..df347eceb53 100644 --- a/src/native/clr/host/generate-pinvoke-tables.cc +++ b/src/native/clr/host/generate-pinvoke-tables.cc @@ -257,6 +257,7 @@ const std::vector dotnet_pinvoke_names = { "SystemNative_GetIPv6MulticastOption", "SystemNative_GetLingerOption", "SystemNative_GetLoadLibraryError", + "SystemNative_GetLowResolutionTimestamp", "SystemNative_GetMaximumAddressSize", "SystemNative_GetNameInfo", "SystemNative_GetNativeIPInterfaceStatistics", diff --git a/src/native/clr/host/internal-pinvokes.cc b/src/native/clr/host/internal-pinvokes.cc index 5440f9171a0..239a3a4c22f 100644 --- a/src/native/clr/host/internal-pinvokes.cc +++ b/src/native/clr/host/internal-pinvokes.cc @@ -39,10 +39,9 @@ bool clr_typemap_java_to_managed (const char *java_type_name, char const** assem BridgeProcessingFtn clr_initialize_gc_bridge ( BridgeProcessingStartedFtn bridge_processing_started_callback, - CollectGCHandlesFtn collect_gchandles_callback, BridgeProcessingFinishedFtn bridge_processing_finished_callback) noexcept { - return GCBridge::initialize_callback (bridge_processing_started_callback, collect_gchandles_callback, bridge_processing_finished_callback); + return GCBridge::initialize_callback (bridge_processing_started_callback, bridge_processing_finished_callback); } void monodroid_log (LogLevel level, LogCategories category, const char *message) noexcept diff --git a/src/native/clr/host/pinvoke-tables.include b/src/native/clr/host/pinvoke-tables.include index 4872eb0d97e..ecdb58263b0 100644 --- a/src/native/clr/host/pinvoke-tables.include +++ b/src/native/clr/host/pinvoke-tables.include @@ -42,7 +42,7 @@ namespace { }}; //64-bit DotNet p/invoke table - std::array dotnet_pinvokes {{ + std::array dotnet_pinvokes {{ {0x99f2ee02463000, "CompressionNative_Crc32", nullptr}, {0xb38afc8bfe830b, "SystemNative_Bind", nullptr}, {0x190fe65d8736dcb, "SystemNative_TryGetIPPacketInformation", nullptr}, @@ -132,6 +132,7 @@ namespace { {0x27944922cd8283ca, "CryptoNative_EvpSha384", nullptr}, {0x2795a01c2c64aea1, "CryptoNative_HmacReset", nullptr}, {0x27f3d9266af2b315, "SystemNative_GetIPv4Address", nullptr}, + {0x287bb84178e256e0, "SystemNative_GetLowResolutionTimestamp", nullptr}, {0x2925953889c48cab, "SystemNative_CreateNetworkChangeListenerSocket", nullptr}, {0x2a49948ae20571cb, "SystemNative_SchedGetAffinity", nullptr}, {0x2b45d7cdf6e8e0c7, "AndroidCryptoNative_X509StoreDeleteEntry", nullptr}, @@ -563,7 +564,7 @@ constexpr hash_t system_globalization_native_library_hash = 0x28b5c8fca080abd5; }}; //32-bit DotNet p/invoke table - std::array dotnet_pinvokes {{ + std::array dotnet_pinvokes {{ {0xaf6b1c, "AndroidCryptoNative_RsaPrivateDecrypt", nullptr}, {0x1733089, "SystemNative_SetTerminalInvalidationHandler", nullptr}, {0x1dd1f00, "AndroidCryptoNative_Aes192Cfb8", nullptr}, @@ -934,6 +935,7 @@ constexpr hash_t system_globalization_native_library_hash = 0x28b5c8fca080abd5; {0xc5a83c28, "AndroidCryptoNative_SSLStreamCreateWithKeyStorePrivateKeyEntry", nullptr}, {0xc6d5929c, "SystemNative_LowLevelMonitor_TimedWait", nullptr}, {0xc6f2fb9e, "AndroidCryptoNative_X509ChainSetCustomTrustStore", nullptr}, + {0xc6fc0368, "SystemNative_GetLowResolutionTimestamp", nullptr}, {0xc717b16e, "CryptoNative_EvpMdCtxDestroy", nullptr}, {0xc746b70c, "AndroidCryptoNative_DsaSign", nullptr}, {0xc83527e0, "CryptoNative_HmacReset", nullptr}, @@ -1054,5 +1056,5 @@ constexpr hash_t system_globalization_native_library_hash = 0xa66f1e5a; #endif constexpr size_t internal_pinvokes_count = 27; -constexpr size_t dotnet_pinvokes_count = 477; +constexpr size_t dotnet_pinvokes_count = 478; } // end of anonymous namespace diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index c84a7b8e41a..6c8bfd7864c 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -15,30 +15,36 @@ struct JniObjectReferenceControlBlock int refs_added; }; +struct HandleContext +{ + intptr_t gc_handle; + int32_t is_collected; + JniObjectReferenceControlBlock* control_block; +}; + struct StronglyConnectedComponent { - ssize_t Count; - JniObjectReferenceControlBlock** ContextMemory; + size_t Count; + HandleContext** Contexts; }; struct ComponentCrossReference { - ssize_t SourceGroupIndex; - ssize_t DestinationGroupIndex; + size_t SourceGroupIndex; + size_t DestinationGroupIndex; }; -struct MarkCrossReferences +struct MarkCrossReferencesArgs { - ssize_t ComponentsLen; + size_t ComponentsLen; StronglyConnectedComponent* Components; - ssize_t CrossReferencesLen; + size_t CrossReferencesLen; ComponentCrossReference* CrossReferences; }; using BridgeProcessingStartedFtn = void (*)(); -using CollectGCHandlesFtn = intptr_t (*)(MarkCrossReferences*); -using BridgeProcessingFinishedFtn = void (*)(MarkCrossReferences*, intptr_t); -using BridgeProcessingFtn = void (*)(MarkCrossReferences*); +using BridgeProcessingFinishedFtn = void (*)(MarkCrossReferencesArgs*); +using BridgeProcessingFtn = void (*)(MarkCrossReferencesArgs*); namespace xamarin::android { class GCBridge @@ -48,15 +54,14 @@ namespace xamarin::android { static void initialize_on_load (JNIEnv *env) noexcept; static BridgeProcessingFtn initialize_callback ( BridgeProcessingStartedFtn bridge_processing_started_callback, - CollectGCHandlesFtn collect_gchandles_callback, BridgeProcessingFinishedFtn bridge_processing_finished_callback) noexcept { + abort_if_invalid_pointer_argument (bridge_processing_started_callback, "bridge_processing_started_callback"); abort_if_invalid_pointer_argument (bridge_processing_finished_callback, "bridge_processing_finished_callback"); abort_unless (GCBridge::bridge_processing_started_callback == nullptr, "GC bridge processing started callback is already set"); abort_unless (GCBridge::bridge_processing_finished_callback == nullptr, "GC bridge processing finished callback is already set"); GCBridge::bridge_processing_started_callback = bridge_processing_started_callback; - GCBridge::collect_gchandles_callback = collect_gchandles_callback; GCBridge::bridge_processing_finished_callback = bridge_processing_finished_callback; bridge_processing_thread = new std::thread(GCBridge::bridge_processing); @@ -66,12 +71,10 @@ namespace xamarin::android { } private: - static inline BridgeProcessingStartedFtn bridge_processing_started_callback = nullptr; - static inline CollectGCHandlesFtn collect_gchandles_callback = nullptr; static inline BridgeProcessingFinishedFtn bridge_processing_finished_callback = nullptr; - static inline MarkCrossReferences shared_cross_refs; + static inline MarkCrossReferencesArgs shared_cross_refs; static inline std::binary_semaphore bridge_processing_semaphore{0}; static inline JNIEnv* env = nullptr; @@ -80,18 +83,18 @@ namespace xamarin::android { static void trigger_java_gc () noexcept; static void bridge_processing () noexcept; - static void mark_cross_references (MarkCrossReferences* cross_refs) noexcept; + static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; - static bool add_reference (JniObjectReferenceControlBlock *from, jobject to) noexcept; + static bool add_reference (HandleContext *from, jobject to) noexcept; static bool add_direct_reference (jobject from, jobject to) noexcept; // TODO naming static void clear_references (jobject handle) noexcept; static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, int index) noexcept; static jobject get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; static void maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept; - static void prepare_for_java_collection (MarkCrossReferences* cross_refs) noexcept; - static void cleanup_after_java_collection (MarkCrossReferences* cross_refs) noexcept; + static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; + static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; static inline jobject Runtime_instance = nullptr; static inline jmethodID Runtime_gc = nullptr; diff --git a/src/native/clr/include/runtime-base/internal-pinvokes.hh b/src/native/clr/include/runtime-base/internal-pinvokes.hh index b0721e3a789..0bc3f2d841f 100644 --- a/src/native/clr/include/runtime-base/internal-pinvokes.hh +++ b/src/native/clr/include/runtime-base/internal-pinvokes.hh @@ -17,7 +17,6 @@ extern "C" { bool clr_typemap_java_to_managed (const char *java_type_name, char const** assembly_name, uint32_t *managed_type_token_id) noexcept; BridgeProcessingFtn clr_initialize_gc_bridge ( BridgeProcessingStartedFtn bridge_processing_started_callback, - CollectGCHandlesFtn collect_gchandles_callback, BridgeProcessingFinishedFtn mark_cross_references_callback) noexcept; void monodroid_log (xamarin::android::LogLevel level, LogCategories category, const char *message) noexcept; char* monodroid_TypeManager_get_java_class_name (jclass klass) noexcept; diff --git a/tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj b/tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj index 484fe9c2525..d50e2ba1c1f 100644 --- a/tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj +++ b/tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj @@ -14,7 +14,7 @@ true ..\..\..\product.snk $(DefineConstants);NO_MARSHAL_MEMBER_BUILDER_SUPPORT - $(DefineConstants);NO_GC_BRIDGE_SUPPORT + $(DefineConstants);NO_GC_BRIDGE_SUPPORT $(JavaInteropSourceDirectory)\tests\Java.Interop-Tests\ From 247d851a5363a4c8d2afc09da4ac5af77c4b7665 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 9 Jun 2025 11:37:09 +0200 Subject: [PATCH 03/17] Improve DisposePeer --- .../ManagedValueManager.cs | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 922200ba531..714f9150096 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -29,9 +29,7 @@ class ManagedValueManager : JniRuntime.JniValueManager private unsafe ManagedValueManager () { // There can only be one instance of ManagedValueManager because we can call JavaMarshal.Initialize only once. - var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge ( - &BridgeProcessingStarted, - &BridgeProcessingFinished); + var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingStarted, &BridgeProcessingFinished); JavaMarshal.Initialize (mark_cross_references_ftn); } @@ -56,10 +54,10 @@ public override void CollectPeers () RegisteredInstances.Clear (); } List? exceptions = null; - foreach (var peer in peers) { + foreach (var handle in peers) { try { - if (TryGetTarget (peer, out IDisposable? disposable)) - disposable.Dispose (); + if (handle.IsAllocated) + (handle.Target as IDisposable)?.Dispose (); } catch (Exception e) { exceptions = exceptions ?? new List (); @@ -101,7 +99,9 @@ public override void AddPeer (IJavaPeerable value) for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - if (!TryGetTarget (p, out IJavaPeerable? peer)) + if (!p.IsAllocated) + continue; + if (p.Target is not IJavaPeerable peer) continue; if (!JniEnvironment.Types.IsSameObject (peer.PeerReference, value.PeerReference)) continue; @@ -128,7 +128,9 @@ public override void DisposePeer (IJavaPeerable value) WaitForGCBridgeProcessing (); + // start by collecting peers that need to be removed later int key = value.JniIdentityHashCode; + List indexesToRemove = []; lock (RegisteredInstances) { List? peers; @@ -137,19 +139,29 @@ public override void DisposePeer (IJavaPeerable value) for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - - if (TryGetTarget (p, out IJavaPeerable? peer) - && ReferenceEquals (value, peer)) { - FreeReferenceTrackingHandle (p); - peers.RemoveAt (i); - GC.KeepAlive (peer); + if (p.IsAllocated && ReferenceEquals (value, p.Target)) + { + indexesToRemove.Add (i); } } - if (peers.Count == 0) - RegisteredInstances.Remove (key); } - base.DisposePeer (value); + // dispose the peer + base.DisposePeer(value); + + // and then clean up the registered instances + lock (RegisteredInstances) { + List? peers; + if (!RegisteredInstances.TryGetValue(key, out peers)) + return; + + foreach (int i in indexesToRemove) { + // Remove the peer from the list + var p = peers[i]; + FreeReferenceTrackingHandle(p); + peers.RemoveAt(i); + } + } } public override void DisposePeerUnlessReferenced (IJavaPeerable value) @@ -175,11 +187,7 @@ public override void DisposePeerUnlessReferenced (IJavaPeerable value) } static bool Replaceable (GCHandle handle) - { - if (!TryGetTarget (handle, out IJavaPeerable? peer)) - return true; - return peer.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable); - } + => handle.IsAllocated && (handle.Target as IJavaPeerable)?.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) ?? false; void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue) { @@ -216,8 +224,12 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - if (TryGetTarget (p, out IJavaPeerable? peer) && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) + if (p.IsAllocated + && p.Target is IJavaPeerable peer + && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) + { return peer; + } } if (peers.Count == 0) RegisteredInstances.Remove (key); @@ -250,8 +262,8 @@ private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) return; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (TryGetTarget (p, out IJavaPeerable? peer) && ReferenceEquals (target, peer)) { + var p = peers [i]; + if (p.IsAllocated && ReferenceEquals (target, p.Target)) { peers.RemoveAt (i); if (freeHandle) FreeReferenceTrackingHandle (p); @@ -281,8 +293,10 @@ private GCHandle PeekPeerHandle (JniObjectReference reference) for (int i = peers.Count - 1; i >= 0; i--) { var p = peers[i]; - if (TryGetTarget (p, out IJavaPeerable? peer) - && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) { + if (p.IsAllocated + && p.Target is IJavaPeerable peer + && JniEnvironment.Types.IsSameObject(reference, peer.PeerReference)) + { return p; } } @@ -378,9 +392,9 @@ public override List GetSurfacedPeers () var peers = new List (RegisteredInstances.Count); foreach (var e in RegisteredInstances) { foreach (var p in e.Value) { - if (!TryGetTarget (p, out IJavaPeerable? peer)) - continue; - peers.Add (new JniSurfacedPeerInfo (e.Key, new WeakReference (peer))); + if (p.IsAllocated && p.Target is IJavaPeerable peer) { + peers.Add (new JniSurfacedPeerInfo (e.Key, new WeakReference (peer))); + } } } return peers; @@ -430,20 +444,6 @@ static unsafe GCHandle CreateReferenceTrackingHandle (IJavaPeerable value) return handle; } - // TODO: The main reason this method is necessary is that there is a SIGSEGV when we access - // handle.Target of a reference tracking handle when its context is null - static unsafe bool TryGetTarget (GCHandle handle, [NotNullWhen (true)] out T? target) - where T : class - { - target = null; - - if (handle.IsAllocated && JavaMarshal.GetContext (handle) != null) { - target = handle.Target as T; - } - - return target is not null; - } - [UnmanagedCallersOnly] internal static void BridgeProcessingStarted () { @@ -467,7 +467,7 @@ internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* m } // Cleanup: Remove the handle from RegisteredInstances - if (TryGetTarget (handle, out IJavaPeerable? target)) { + if (handle.IsAllocated && handle.Target is IJavaPeerable target) { Instance.RemoveRegisteredInstance (target, freeHandle: false); } From 1825684b6755bb9b5c8aba05a80facccde3400c6 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 9 Jun 2025 14:41:14 +0200 Subject: [PATCH 04/17] Remove unnecessary assert --- src/native/clr/host/gc-bridge.cc | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 62b476d3ac4..a7f32c07c50 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -326,27 +326,8 @@ void GCBridge::bridge_processing () noexcept } } -static void assert_valid_handles (MarkCrossReferencesArgs* cross_refs) noexcept -{ - for (int i = 0; i < cross_refs->ComponentsLen; i++) - { - StronglyConnectedComponent *scc = &cross_refs->Components [i]; - for (int j = 0; j < scc->Count; j++) { - HandleContext *context = scc->Contexts [j]; - if (!is_valid_gref (context->control_block)) { - log_error_fmt (LOG_DEFAULT, "Invalid global reference in SCC context {:x}, gchandle: {}, control block {}, handle {}, handle type {}", - (intptr_t)context, context->gc_handle, (intptr_t)context->control_block, (intptr_t)context->control_block->handle, context->control_block->handle_type); - } - abort_unless (is_valid_gref (context->control_block), "Invalid global reference in SCC"); - } - } -} - void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept { - // TODO get rid of this - assert_valid_handles (cross_refs); - { std::unique_lock lock (processing_mutex); From 9fa456266b3fd361e093c55557c1332205ae86bc Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 9 Jun 2025 15:46:08 +0200 Subject: [PATCH 05/17] Fix warnings --- .../ManagedValueManager.cs | 11 +- src/native/clr/host/gc-bridge.cc | 109 ++++++++---------- src/native/clr/include/host/gc-bridge.hh | 26 ++--- 3 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 714f9150096..c451c7c6e30 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -187,7 +187,16 @@ public override void DisposePeerUnlessReferenced (IJavaPeerable value) } static bool Replaceable (GCHandle handle) - => handle.IsAllocated && (handle.Target as IJavaPeerable)?.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) ?? false; + { + if (!handle.IsAllocated) + return false; + + var target = handle.Target as IJavaPeerable; + if (target is null) + return false; + + return target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable); + } void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue) { diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index a7f32c07c50..9ada883b43c 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -1,30 +1,41 @@ #include #include #include +#include using namespace xamarin::android; void GCBridge::wait_for_bridge_processing () noexcept { - // log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing - waiting to acquire processing mutex"); std::shared_lock lock (processing_mutex); - // log_write (LOG_DEFAULT, LogLevel::Info, "GCBridge: wait_for_bridge_processing - acquired processing mutex"); } -void GCBridge::initialize_on_load (JNIEnv *env) noexcept +void GCBridge::initialize_on_load (JNIEnv *jniEnv) noexcept { - abort_if_invalid_pointer_argument (env, "env"); + abort_if_invalid_pointer_argument (jniEnv, "jniEnv"); - jclass lref = env->FindClass ("java/lang/Runtime"); - jmethodID Runtime_getRuntime = env->GetStaticMethodID (lref, "getRuntime", "()Ljava/lang/Runtime;"); - Runtime_gc = env->GetMethodID (lref, "gc", "()V"); - Runtime_instance = OSBridge::lref_to_gref (env, env->CallStaticObjectMethod (lref, Runtime_getRuntime)); - env->DeleteLocalRef (lref); + jclass lref = jniEnv->FindClass ("java/lang/Runtime"); + jmethodID Runtime_getRuntime = jniEnv->GetStaticMethodID (lref, "getRuntime", "()Ljava/lang/Runtime;"); + Runtime_gc = jniEnv->GetMethodID (lref, "gc", "()V"); + Runtime_instance = OSBridge::lref_to_gref (jniEnv, jniEnv->CallStaticObjectMethod (lref, Runtime_getRuntime)); + jniEnv->DeleteLocalRef (lref); abort_unless ( Runtime_gc != nullptr && Runtime_instance != nullptr, "Failed to look up Java GC runtime API." ); + + lref = jniEnv->FindClass ("java/util/ArrayList"); + ArrayList_class = reinterpret_cast (OSBridge::lref_to_gref (jniEnv, lref)); + ArrayList_ctor = jniEnv->GetMethodID (ArrayList_class, "", "()V"); + ArrayList_add = jniEnv->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); + ArrayList_get = jniEnv->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); + jniEnv->DeleteLocalRef (lref); + + abort_unless ( + ArrayList_class != nullptr && ArrayList_ctor != nullptr && ArrayList_add != nullptr && ArrayList_get != nullptr, + "Failed to look up ArrayList and its methods." + ); } void GCBridge::trigger_java_gc () noexcept @@ -37,49 +48,46 @@ void GCBridge::trigger_java_gc () noexcept } } -bool GCBridge::add_reference (HandleContext *from, jobject to) noexcept +void GCBridge::add_reference (HandleContext *from, jobject to) noexcept { abort_if_invalid_pointer_argument (from, "from"); abort_if_invalid_pointer_argument (to, "to"); - if (add_direct_reference (from->control_block->handle, to)) { - from->control_block->refs_added++; - return true; - } else { - return false; - } + add_direct_reference (from->control_block->handle, to); + from->control_block->refs_added++; } -bool GCBridge::add_direct_reference (jobject from, jobject to) noexcept +void GCBridge::add_direct_reference (jobject from, jobject to) noexcept { abort_if_invalid_pointer_argument (from, "from"); abort_if_invalid_pointer_argument (to, "to"); jclass java_class = env->GetObjectClass (from); jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); - - env->DeleteLocalRef (java_class); // TODO do I also need to delete the jmethodID? or that's not necessary? + env->DeleteLocalRef (java_class); + if (add_method_id) { env->CallVoidMethod (from, add_method_id, to); - return true; + } else { + env->ExceptionClear (); } - - env->ExceptionClear (); - return false; } int GCBridge::scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); abort_unless (scc->Count < 0, "Attempted to load stashed index from an object which does not contain one."); + abort_unless (scc->Count >= static_cast (std::numeric_limits::min ()), "Count cannot fit in an int."); - return -scc->Count - 1; + return static_cast (-scc->Count - 1); } -void GCBridge::scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, int index) noexcept +void GCBridge::scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); - scc->Count = -index - 1; // Store the index as a negative value + abort_unless (index >= 0, "Index must be non-negative"); + + scc->Count = -index - 1; } bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept @@ -88,13 +96,6 @@ bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept return scc->Count < 0; // If Count is negative, it's a bridgeless SCC } -static bool is_valid_gref (JniObjectReferenceControlBlock *control_block) noexcept -{ - return control_block != nullptr - && control_block->handle != nullptr - && control_block->handle_type == JNIGlobalRefType; -} - jobject GCBridge::get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); @@ -127,17 +128,20 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) // Before looking at xrefs, scan the SCCs. During collection, an SCC has to behave like a // single object. If the number of objects in the SCC is anything other than 1, the SCC // must be doctored to mimic that one-object nature. - for (int i = 0; i < cross_refs->ComponentsLen; i++) + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; + abort_unless (scc->Count >= 0, + "Attempted to prepare for GC bridge processing where the number of strongly connected components is negative (likely due to overflow of ssize_t)."); + // Count > 1 case: The SCC contains many objects which must be collected as one. // Solution: Make all objects within the SCC directly or indirectly reference each other if (scc->Count > 1) { HandleContext *first = scc->Contexts [0]; HandleContext *prev = first; - for (int j = 1; j < scc->Count; j++) { + for (ssize_t j = 1; j < scc->Count; j++) { HandleContext *current = scc->Contexts [j]; add_reference (prev, current->control_block->handle); @@ -149,19 +153,6 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Creating temporary peer for SCC at index {} with no bridge objects", i); - // Once per process boot, look up JNI metadata we need to make temporary objects - if (ArrayList_class == nullptr) { - ArrayList_class = reinterpret_cast (OSBridge::lref_to_gref (env, env->FindClass ("java/util/ArrayList"))); - ArrayList_ctor = env->GetMethodID (ArrayList_class, "", "()V"); - ArrayList_add = env->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); - ArrayList_get = env->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); - - abort_unless ( - ArrayList_class != nullptr && ArrayList_ctor != nullptr && ArrayList_get != nullptr, - "Failed to load classes required for JNI" - ); - } - // Once per prepare_for_java_collection call, create a list to hold the temporary // objects we create. This will protect them from collection while we build the list. if (!temporary_peers) { @@ -179,7 +170,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) } // Add the cross scc refs - for (int i = 0; i < cross_refs->CrossReferencesLen; i++) + for (size_t i = 0; i < cross_refs->CrossReferenceCount; i++) { ComponentCrossReference *xref = &cross_refs->CrossReferences [i]; log_write_fmt (LOG_DEFAULT, LogLevel::Info, @@ -210,7 +201,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) env->DeleteLocalRef (temporary_peers); // Switch global to weak references - for (int i = 0; i < cross_refs->ComponentsLen; i++) + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; @@ -218,7 +209,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) scc->Count = 0; // Reset any temporary peer index stored in Count } - for (int j = 0; j < scc->Count; j++) { + for (ssize_t j = 0; j < scc->Count; j++) { HandleContext *context = scc->Contexts [j]; jobject handle = context->control_block->handle; @@ -262,11 +253,11 @@ void GCBridge::clear_references (jobject handle) noexcept void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { // try to switch back to global refs to analyze what stayed alive - for (int i = 0; i < cross_refs->ComponentsLen; i++) + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - for (int j = 0; j < scc->Count; j++) { + for (ssize_t j = 0; j < scc->Count; j++) { HandleContext *context = scc->Contexts [j]; jobject weak = context->control_block->handle; @@ -328,14 +319,12 @@ void GCBridge::bridge_processing () noexcept void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept { - { - std::unique_lock lock (processing_mutex); + std::unique_lock lock (processing_mutex); - GCBridge::shared_cross_refs.ComponentsLen = cross_refs->ComponentsLen; - GCBridge::shared_cross_refs.Components = cross_refs->Components; - GCBridge::shared_cross_refs.CrossReferencesLen = cross_refs->CrossReferencesLen; - GCBridge::shared_cross_refs.CrossReferences = cross_refs->CrossReferences; - } + GCBridge::shared_cross_refs.ComponentCount = cross_refs->ComponentCount; + GCBridge::shared_cross_refs.Components = cross_refs->Components; + GCBridge::shared_cross_refs.CrossReferenceCount = cross_refs->CrossReferenceCount; + GCBridge::shared_cross_refs.CrossReferences = cross_refs->CrossReferences; bridge_processing_semaphore.release (); } diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 6c8bfd7864c..1f1b9e89732 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -24,7 +24,7 @@ struct HandleContext struct StronglyConnectedComponent { - size_t Count; + ssize_t Count; // We need to use ssize_t instead of size_t to allow negative values for bridgeless SCCs HandleContext** Contexts; }; @@ -36,9 +36,9 @@ struct ComponentCrossReference struct MarkCrossReferencesArgs { - size_t ComponentsLen; + size_t ComponentCount; StronglyConnectedComponent* Components; - size_t CrossReferencesLen; + size_t CrossReferenceCount; ComponentCrossReference* CrossReferences; }; @@ -53,16 +53,16 @@ namespace xamarin::android { static void wait_for_bridge_processing () noexcept; static void initialize_on_load (JNIEnv *env) noexcept; static BridgeProcessingFtn initialize_callback ( - BridgeProcessingStartedFtn bridge_processing_started_callback, - BridgeProcessingFinishedFtn bridge_processing_finished_callback) noexcept + BridgeProcessingStartedFtn bridge_processing_started, + BridgeProcessingFinishedFtn bridge_processing_finished) noexcept { - abort_if_invalid_pointer_argument (bridge_processing_started_callback, "bridge_processing_started_callback"); - abort_if_invalid_pointer_argument (bridge_processing_finished_callback, "bridge_processing_finished_callback"); + abort_if_invalid_pointer_argument (bridge_processing_started, "bridge_processing_started"); + abort_if_invalid_pointer_argument (bridge_processing_finished, "bridge_processing_finished"); abort_unless (GCBridge::bridge_processing_started_callback == nullptr, "GC bridge processing started callback is already set"); abort_unless (GCBridge::bridge_processing_finished_callback == nullptr, "GC bridge processing finished callback is already set"); - GCBridge::bridge_processing_started_callback = bridge_processing_started_callback; - GCBridge::bridge_processing_finished_callback = bridge_processing_finished_callback; + GCBridge::bridge_processing_started_callback = bridge_processing_started; + GCBridge::bridge_processing_finished_callback = bridge_processing_finished; bridge_processing_thread = new std::thread(GCBridge::bridge_processing); bridge_processing_thread->detach (); @@ -86,11 +86,11 @@ namespace xamarin::android { static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; - static bool add_reference (HandleContext *from, jobject to) noexcept; - static bool add_direct_reference (jobject from, jobject to) noexcept; // TODO naming + static void add_reference (HandleContext *from, jobject to) noexcept; + static void add_direct_reference (jobject from, jobject to) noexcept; // TODO naming static void clear_references (jobject handle) noexcept; static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; - static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, int index) noexcept; + static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; static jobject get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; static void maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept; static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; @@ -101,7 +101,7 @@ namespace xamarin::android { static inline jclass GCUserPeer_class = nullptr; static inline jmethodID GCUserPeer_ctor = nullptr; - + static inline jclass ArrayList_class = nullptr; static inline jmethodID ArrayList_ctor = nullptr; static inline jmethodID ArrayList_get = nullptr; From 1bfcbfba36d5839337e0266dfecba40d3b65bdf1 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 9 Jun 2025 17:08:26 +0200 Subject: [PATCH 06/17] Cleanup --- src/native/clr/host/gc-bridge.cc | 128 +++++++++++++---------- src/native/clr/include/host/gc-bridge.hh | 20 +++- 2 files changed, 90 insertions(+), 58 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 9ada883b43c..b7d7fbfee2a 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -24,18 +24,6 @@ void GCBridge::initialize_on_load (JNIEnv *jniEnv) noexcept Runtime_gc != nullptr && Runtime_instance != nullptr, "Failed to look up Java GC runtime API." ); - - lref = jniEnv->FindClass ("java/util/ArrayList"); - ArrayList_class = reinterpret_cast (OSBridge::lref_to_gref (jniEnv, lref)); - ArrayList_ctor = jniEnv->GetMethodID (ArrayList_class, "", "()V"); - ArrayList_add = jniEnv->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); - ArrayList_get = jniEnv->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); - jniEnv->DeleteLocalRef (lref); - - abort_unless ( - ArrayList_class != nullptr && ArrayList_ctor != nullptr && ArrayList_add != nullptr && ArrayList_get != nullptr, - "Failed to look up ArrayList and its methods." - ); } void GCBridge::trigger_java_gc () noexcept @@ -48,28 +36,49 @@ void GCBridge::trigger_java_gc () noexcept } } -void GCBridge::add_reference (HandleContext *from, jobject to) noexcept +void GCBridge::add_inner_reference (HandleContext *from, HandleContext *to) noexcept { abort_if_invalid_pointer_argument (from, "from"); abort_if_invalid_pointer_argument (to, "to"); - add_direct_reference (from->control_block->handle, to); - from->control_block->refs_added++; + if (add_reference (from->control_block->handle, to->control_block->handle)) { + from->control_block->refs_added++; + } } -void GCBridge::add_direct_reference (jobject from, jobject to) noexcept +void GCBridge::add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept { - abort_if_invalid_pointer_argument (from, "from"); - abort_if_invalid_pointer_argument (to, "to"); + jobject from_object; + if (from.is_bridgeless_scc) { + from_object = from.target; + } else { + from_object = from.handle_context->control_block->handle; + } + jobject to_object; + if (to.is_bridgeless_scc) { + to_object = to.target; + } else { + to_object = to.handle_context->control_block->handle; + } + + if (add_reference (from_object, to_object) && !from.is_bridgeless_scc) { + from.handle_context->control_block->refs_added++; + } +} + +bool GCBridge::add_reference (jobject from, jobject to) noexcept +{ jclass java_class = env->GetObjectClass (from); jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); env->DeleteLocalRef (java_class); - + if (add_method_id) { env->CallVoidMethod (from, add_method_id, to); + return true; } else { env->ExceptionClear (); + return false; } } @@ -96,26 +105,33 @@ bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept return scc->Count < 0; // If Count is negative, it's a bridgeless SCC } -jobject GCBridge::get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept +GCBridge::CrossReferenceComponent GCBridge::get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); - if (scc->Count > 0) { - return scc->Contexts [0]->control_block->handle; // Return the first valid global reference - } else { + if (is_bridgeless_scc (scc)) { abort_unless (temporary_peers != nullptr, "Temporary peers must not be null for bridgeless SCCs"); int index = scc_get_stashed_temporary_peer_index (scc); - return env->CallObjectMethod (temporary_peers, ArrayList_get, index); + jobject target = env->CallObjectMethod (temporary_peers, ArrayList_get, index); + + return GCBridge::CrossReferenceComponent { + .is_bridgeless_scc = true, + .target = target, + }; + } else { + return GCBridge::CrossReferenceComponent { + .is_bridgeless_scc = false, + .handle_context = scc->Contexts [0], + }; } } -void GCBridge::maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept +void GCBridge::release_target (GCBridge::CrossReferenceComponent target) noexcept { - abort_if_invalid_pointer_argument (scc, "scc"); - - if (scc->Count < 0) { - env->DeleteLocalRef (handle); // Release the local ref for bridgeless SCCs returned from get_scc_representative + if (target.is_bridgeless_scc) { + // Release the local ref for bridgeless SCCs returned from get_scc_representative + env->DeleteLocalRef (target.target); } } @@ -143,16 +159,14 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) for (ssize_t j = 1; j < scc->Count; j++) { HandleContext *current = scc->Contexts [j]; - - add_reference (prev, current->control_block->handle); + add_inner_reference (prev, current); prev = current; } - add_reference (prev, first->control_block->handle); + add_inner_reference (prev, first); } else if (scc->Count == 0) { - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Creating temporary peer for SCC at index {} with no bridge objects", i); - + ensure_array_list (); + // Once per prepare_for_java_collection call, create a list to hold the temporary // objects we create. This will protect them from collection while we build the list. if (!temporary_peers) { @@ -173,28 +187,14 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) for (size_t i = 0; i < cross_refs->CrossReferenceCount; i++) { ComponentCrossReference *xref = &cross_refs->CrossReferences [i]; - log_write_fmt (LOG_DEFAULT, LogLevel::Info, - "Processing cross-reference at index {}: {} -> {}", i, xref->SourceGroupIndex, xref->DestinationGroupIndex); - - StronglyConnectedComponent *from_scc = &cross_refs->Components [xref->SourceGroupIndex]; - StronglyConnectedComponent *to_scc = &cross_refs->Components [xref->DestinationGroupIndex]; + + GCBridge::CrossReferenceComponent from = get_target (&cross_refs->Components [xref->SourceGroupIndex], temporary_peers); + GCBridge::CrossReferenceComponent to = get_target (&cross_refs->Components [xref->DestinationGroupIndex], temporary_peers); - if (is_bridgeless_scc (from_scc)) { - jobject from_handle = get_scc_representative (from_scc, temporary_peers); - jobject to_handle = get_scc_representative (to_scc, temporary_peers); + add_cross_reference (from, to); - add_direct_reference (from_handle, to_handle); - - maybe_release_scc_representative (from_scc, from_handle); - maybe_release_scc_representative (to_scc, to_handle); - } else { - HandleContext *from = from_scc->Contexts [0]; - jobject to_handle = get_scc_representative (to_scc, temporary_peers); - - add_reference (from, to_handle); - - maybe_release_scc_representative (to_scc, to_handle); - } + release_target (from); + release_target (to); } // With xrefs processed, the temporary peer list can be released @@ -328,3 +328,21 @@ void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexc bridge_processing_semaphore.release (); } + +[[gnu::always_inline]] +void GCBridge::ensure_array_list () noexcept +{ + if (ArrayList_class == nullptr) [[unlikely]] { + ArrayList_class = env->FindClass ("java/util/ArrayList"); + abort_unless (ArrayList_class != nullptr, "Failed to find java/util/ArrayList class"); + + ArrayList_ctor = env->GetMethodID (ArrayList_class, "", "()V"); + abort_unless (ArrayList_ctor != nullptr, "Failed to find ArrayList constructor"); + + ArrayList_get = env->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); + abort_unless (ArrayList_get != nullptr, "Failed to find ArrayList get method"); + + ArrayList_add = env->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); + abort_unless (ArrayList_add != nullptr, "Failed to find ArrayList add method"); + } +} diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 1f1b9e89732..43f390047d2 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -70,6 +70,15 @@ namespace xamarin::android { return mark_cross_references; } + struct CrossReferenceComponent + { + bool is_bridgeless_scc; + union { + jobject target; + HandleContext* handle_context; + }; + }; + private: static inline BridgeProcessingStartedFtn bridge_processing_started_callback = nullptr; static inline BridgeProcessingFinishedFtn bridge_processing_finished_callback = nullptr; @@ -86,8 +95,9 @@ namespace xamarin::android { static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; - static void add_reference (HandleContext *from, jobject to) noexcept; - static void add_direct_reference (jobject from, jobject to) noexcept; // TODO naming + static void add_inner_reference (HandleContext *from, HandleContext *to) noexcept; + static void add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept; + static bool add_reference (jobject from, jobject to) noexcept; static void clear_references (jobject handle) noexcept; static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; @@ -101,10 +111,14 @@ namespace xamarin::android { static inline jclass GCUserPeer_class = nullptr; static inline jmethodID GCUserPeer_ctor = nullptr; - + + static void ensure_array_list () noexcept; static inline jclass ArrayList_class = nullptr; static inline jmethodID ArrayList_ctor = nullptr; static inline jmethodID ArrayList_get = nullptr; static inline jmethodID ArrayList_add = nullptr; + + static GCBridge::CrossReferenceComponent get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; + static void release_target (GCBridge::CrossReferenceComponent target) noexcept; }; } From a717c800a33ae40b625a50399d13ff6344489373 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Mon, 9 Jun 2025 18:07:37 +0200 Subject: [PATCH 07/17] Align the implementation more with the mono side --- src/native/clr/host/gc-bridge.cc | 216 ++++++++++++++++------- src/native/clr/include/host/gc-bridge.hh | 18 +- 2 files changed, 166 insertions(+), 68 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index b7d7fbfee2a..d930a29023a 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -127,11 +127,11 @@ GCBridge::CrossReferenceComponent GCBridge::get_target (StronglyConnectedCompone } } -void GCBridge::release_target (GCBridge::CrossReferenceComponent target) noexcept +void GCBridge::release_target (GCBridge::CrossReferenceComponent component) noexcept { - if (target.is_bridgeless_scc) { - // Release the local ref for bridgeless SCCs returned from get_scc_representative - env->DeleteLocalRef (target.target); + if (component.is_bridgeless_scc) { + // Release the local ref for bridgeless SCCs + env->DeleteLocalRef (component.target); } } @@ -165,6 +165,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) add_inner_reference (prev, first); } else if (scc->Count == 0) { + // Once per process boot, look up JNI metadata we need to make temporary objects ensure_array_list (); // Once per prepare_for_java_collection call, create a list to hold the temporary @@ -201,38 +202,76 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) env->DeleteLocalRef (temporary_peers); // Switch global to weak references - for (size_t i = 0; i < cross_refs->ComponentCount; i++) - { + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - if (scc->Count < 0) { - scc->Count = 0; // Reset any temporary peer index stored in Count - } + // Reset any temporary peer index stored in Count + if (scc->Count < 0) + scc->Count = 0; for (ssize_t j = 0; j < scc->Count; j++) { - HandleContext *context = scc->Contexts [j]; + take_weak_global_ref (scc->Contexts [j]); + } + } +} - jobject handle = context->control_block->handle; - - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Switching global reference to weak for context {:x}, gchandle: {}, handle {:x}, refs added {}", - // (intptr_t)context, (intptr_t)context->gc_handle, (intptr_t)handle, context->control_block->refs_added); - - jobject weak = env->NewWeakGlobalRef (handle); +void GCBridge::take_global_ref (HandleContext *context) noexcept +{ + abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, "Expected weak global reference type for handle"); + jobject weak = context->control_block->handle; + + jobject handle = env->NewGlobalRef (weak); + // if (gref_log) { + // fprintf (gref_log, "*try_take_global obj=%p -> wref=%p handle=%p\n", obj, weak, handle); + // fflush (gref_log); + // } + + if (handle != nullptr) { + context->is_collected = 0; + // _monodroid_gref_log_new (weak, get_object_ref_type (env, weak), + // handle, get_object_ref_type (env, handle), + // "finalizer", gettid (), + // " at [[gc:take_global_ref_jni]]", 0); + } else { + context->is_collected = 1; + // MonoClass *klass = mono_object_get_class (obj); + // char *message = Util::monodroid_strdup_printf ( + // "handle %p/W; key_handle %p; MCW type: `%s.%s`: was collected by a Java GC", + // weak, + // nullptr, // ?? + // mono_class_get_namespace (klass), + // mono_class_get_name (klass)); + // _monodroid_gref_log (message); + // free (message); + } - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Creating weak global ref for: gc_handle {}, handle {:x}, weak handle {:x}, refs {}", - // (intptr_t)context->gc_handle, - // (intptr_t)handle, - // (intptr_t)weak, - // context->control_block->refs_added); + context->control_block->handle = handle; + context->control_block->handle_type = JNIGlobalRefType; - env->DeleteGlobalRef (handle); // Delete the strong reference now that we have a weak one + // _monodroid_weak_gref_delete (weak, get_object_ref_type (env, weak), + // "finalizer", gettid (), " at [[gc:take_global_ref_jni]]", 0); + env->DeleteWeakGlobalRef (weak); +} - context->control_block->handle = weak; - context->control_block->handle_type = JNIWeakGlobalRefType; - } - } +void GCBridge::take_weak_global_ref (HandleContext *context) noexcept +{ + jobject handle = context->control_block->handle; + // if (gref_log) { + // fprintf (gref_log, "*take_weak obj=%p; handle=%p\n", obj, handle); + // fflush (gref_log); + // } + + jobject weak = env->NewWeakGlobalRef (handle); + // _monodroid_weak_gref_new (handle, get_object_ref_type (env, handle), + // weak, get_object_ref_type (env, weak), + // "finalizer", gettid (), " at [[gc:take_weak_global_ref_jni]]", 0); + + context->control_block->handle = weak; + context->control_block->handle_type = JNIWeakGlobalRefType; + + // _monodroid_gref_log_delete (handle, get_object_ref_type (env, handle), + // "finalizer", gettid (), " at [[gc:take_weak_global_ref_jni]]", 0); + env->DeleteGlobalRef (handle); } void GCBridge::clear_references (jobject handle) noexcept @@ -240,61 +279,82 @@ void GCBridge::clear_references (jobject handle) noexcept // Clear references from the object jclass java_class = env->GetObjectClass (handle); jmethodID clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V"); + env->DeleteLocalRef (java_class); // Clean up the local reference to the class if (clear_method_id) { env->CallVoidMethod (handle, clear_method_id); } else { log_error (LOG_DEFAULT, "Failed to find monodroidClearReferences method"); + env->ExceptionClear (); +#if DEBUG + // if (Logger::gc_spew_enabled ()) { + // klass = mono_object_get_class (obj); + // log_error (LOG_GC, + // "Missing monodroidClearReferences method for object of class {}.{}", + // optional_string (mono_class_get_namespace (klass)), + // optional_string (mono_class_get_name (klass)) + // ); + // } +#endif } - - env->DeleteLocalRef (java_class); // Clean up the local reference to the class } void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { - // try to switch back to global refs to analyze what stayed alive - for (size_t i = 0; i < cross_refs->ComponentCount; i++) - { +#if DEBUG +int total = 0; +int alive = 0; +#endif + +// try to switch back to global refs to analyze what stayed alive +for (size_t i = 0; i < cross_refs->ComponentCount; i++) +{ + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + for (ssize_t j = 0; j < scc->Count; j++) { +#if DEBUG + total++; +#endif + take_global_ref (scc->Contexts [j]); + } + } + + // clear the cross references on any remaining items + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; + bool is_alive = false; for (ssize_t j = 0; j < scc->Count; j++) { HandleContext *context = scc->Contexts [j]; + jobject handle = context->control_block->handle; - jobject weak = context->control_block->handle; - - // TODO remove this log - if (context->control_block->handle_type != JNIWeakGlobalRefType) { - log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Skipping non-weak global ref: {:x} ({}), gchandle: {} 0x{:x}", (intptr_t)weak, context->control_block->handle_type, (intptr_t)context->gc_handle, (intptr_t)context->gc_handle); - } - - abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, - "Expected weak global reference type for handle"); - jobject global = env->NewGlobalRef (weak); - - if (global) { - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, "Object survived Java GC: weak {:x} -> gref {:x}", (intptr_t)weak, (intptr_t)global); - env->DeleteWeakGlobalRef (weak); - - context->control_block->handle = global; - context->control_block->handle_type = JNIGlobalRefType; + if (handle) { +#if DEBUG + alive++; +#endif + if (j > 0) { + abort_unless ( + is_alive, + [&i] { return detail::_format_message ("Bridge SCC at index %d must be alive", i); } + ); + } + is_alive = true; if (context->control_block->refs_added > 0) { - // Clear references - clear_references (context->control_block->handle); - - // Reset refs_added + clear_references (handle); context->control_block->refs_added = 0; } } else { - // Object was collected by Java GC - context->is_collected = 1; - - // log_write_fmt (LOG_DEFAULT, LogLevel::Info, - // "Object was collected by Java GC: weak {:x}, gchandle: {}, control block {:x}, handle type {}", - // (intptr_t)weak, (intptr_t)context->gc_handle, (intptr_t)context->control_block, context->control_block->handle_type); + abort_unless ( + !is_alive, + [&i] { return detail::_format_message ("Bridge SCC at index %d must NOT be alive", i); } + ); } } } + +#if DEBUG + log_info (LOG_GC, "GC cleanup summary: {} objects tested - resurrecting {}.", total, alive); +#endif } void GCBridge::bridge_processing () noexcept @@ -309,6 +369,42 @@ void GCBridge::bridge_processing () noexcept bridge_processing_semaphore.acquire (); std::unique_lock lock (processing_mutex); + // if (Logger::gc_spew_enabled ()) { + // int i, j; + // log_info (LOG_GC, "cross references callback invoked with {} sccs and {} xrefs.", num_sccs, num_xrefs); + + // for (i = 0; i < num_sccs; ++i) { + // log_info (LOG_GC, "group {} with {} objects", i, sccs [i]->num_objs); + // for (j = 0; j < sccs [i]->num_objs; ++j) { + // MonoObject *obj = sccs [i]->objs [j]; + + // JniObjectReferenceControlBlock *control_block = get_gc_control_block_for_object (obj); + // jobject handle = 0; + // void *key_handle = nullptr; + // if (control_block != nullptr) { + // mono_field_get_value (obj, reinterpret_cast(control_block->handle), &handle); + // if (control_block->weak_handle != nullptr) { + // mono_field_get_value (obj, reinterpret_cast(control_block->weak_handle), &key_handle); + // } + // } + // MonoClass *klass = mono_object_get_class (obj); + // log_info (LOG_GC, + // "\tobj {:p} [{}::{}] handle {:p} key_handle {:p}", + // reinterpret_cast(obj), + // optional_string (mono_class_get_namespace (klass)), + // optional_string (mono_class_get_name (klass)), + // reinterpret_cast(handle), + // key_handle + // ); + // } + // } + + // if (Util::should_log (LOG_GC)) { + // for (i = 0; i < num_xrefs; ++i) + // log_info_nocheck_fmt (LOG_GC, "xref [{}] {} -> {}", i, xrefs [i].src_scc_index, xrefs [i].dst_scc_index); + // } + // } + bridge_processing_started_callback (); prepare_for_java_collection (&GCBridge::shared_cross_refs); trigger_java_gc (); diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 43f390047d2..81310150352 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -90,22 +90,27 @@ namespace xamarin::android { static inline std::shared_mutex processing_mutex; static inline std::thread* bridge_processing_thread = nullptr; - static void trigger_java_gc () noexcept; static void bridge_processing () noexcept; static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; - + static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; static void add_inner_reference (HandleContext *from, HandleContext *to) noexcept; static void add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept; static bool add_reference (jobject from, jobject to) noexcept; static void clear_references (jobject handle) noexcept; + static GCBridge::CrossReferenceComponent get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; + static void release_target (GCBridge::CrossReferenceComponent target) noexcept; + static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; - static jobject get_scc_representative (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; - static void maybe_release_scc_representative (StronglyConnectedComponent *scc, jobject handle) noexcept; + static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; - + + static void take_weak_global_ref (HandleContext *context) noexcept; + static void take_global_ref (HandleContext *context) noexcept; + + static void trigger_java_gc () noexcept; static inline jobject Runtime_instance = nullptr; static inline jmethodID Runtime_gc = nullptr; @@ -117,8 +122,5 @@ namespace xamarin::android { static inline jmethodID ArrayList_ctor = nullptr; static inline jmethodID ArrayList_get = nullptr; static inline jmethodID ArrayList_add = nullptr; - - static GCBridge::CrossReferenceComponent get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; - static void release_target (GCBridge::CrossReferenceComponent target) noexcept; }; } From 22d93ca414e38add81de12e58927d88e02345e63 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 10 Jun 2025 07:22:37 +0200 Subject: [PATCH 08/17] Remove IsCollected --- .../ManagedValueManager.cs | 4 +- src/native/clr/host/gc-bridge.cc | 72 ++++++++----------- src/native/clr/include/host/gc-bridge.hh | 12 ++-- 3 files changed, 38 insertions(+), 50 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index c451c7c6e30..205e2f11132 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -414,7 +414,6 @@ public override List GetSurfacedPeers () unsafe struct HandleContext { public IntPtr GCHandle; - public bool IsCollected; public IntPtr ControlBlock; static readonly nuint Size = (nuint) Marshal.SizeOf (); @@ -447,7 +446,6 @@ static unsafe GCHandle CreateReferenceTrackingHandle (IJavaPeerable value) var handle = JavaMarshal.CreateReferenceTrackingHandle (value, ctx); ctx->GCHandle = GCHandle.ToIntPtr (handle); - ctx->IsCollected = false; ctx->ControlBlock = value.JniObjectReferenceControlBlock; return handle; @@ -467,7 +465,7 @@ internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* m for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) { var context = (HandleContext*) mcr->Components [i].Contexts [j]; - if (context->IsCollected) { + if (context->ControlBlock == IntPtr.Zero) { var handle = GCHandle.FromIntPtr (context->GCHandle); // Only free handles that haven't been freed yet diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index d930a29023a..f271636ab67 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -38,31 +38,20 @@ void GCBridge::trigger_java_gc () noexcept void GCBridge::add_inner_reference (HandleContext *from, HandleContext *to) noexcept { - abort_if_invalid_pointer_argument (from, "from"); - abort_if_invalid_pointer_argument (to, "to"); + jobject from_object = from->control_block->handle; + jobject to_object = to->control_block->handle; - if (add_reference (from->control_block->handle, to->control_block->handle)) { + if (add_reference (from_object, to_object)) { from->control_block->refs_added++; } } void GCBridge::add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept { - jobject from_object; - if (from.is_bridgeless_scc) { - from_object = from.target; - } else { - from_object = from.handle_context->control_block->handle; - } - - jobject to_object; - if (to.is_bridgeless_scc) { - to_object = to.target; - } else { - to_object = to.handle_context->control_block->handle; - } + jobject from_object = from.is_bridgeless_component ? from.temporary_peer : from.handle_context->control_block->handle; + jobject to_object = to.is_bridgeless_component ? to.temporary_peer : to.handle_context->control_block->handle; - if (add_reference (from_object, to_object) && !from.is_bridgeless_scc) { + if (add_reference (from_object, to_object) && !from.is_bridgeless_component) { from.handle_context->control_block->refs_added++; } } @@ -82,16 +71,15 @@ bool GCBridge::add_reference (jobject from, jobject to) noexcept } } -int GCBridge::scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept +ssize_t GCBridge::get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); abort_unless (scc->Count < 0, "Attempted to load stashed index from an object which does not contain one."); - abort_unless (scc->Count >= static_cast (std::numeric_limits::min ()), "Count cannot fit in an int."); - return static_cast (-scc->Count - 1); + return -scc->Count - 1; } -void GCBridge::scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept +void GCBridge::set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); abort_unless (index >= 0, "Index must be non-negative"); @@ -99,39 +87,40 @@ void GCBridge::scc_set_stashed_temporary_peer_index (StronglyConnectedComponent scc->Count = -index - 1; } -bool GCBridge::is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept +bool GCBridge::is_bridgeless_component (StronglyConnectedComponent *scc) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); - return scc->Count < 0; // If Count is negative, it's a bridgeless SCC + + // if we stashed a temporary peer index in Count (negative number), then this is a bridgeless SCC + return scc->Count < 0; } GCBridge::CrossReferenceComponent GCBridge::get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); - if (is_bridgeless_scc (scc)) { + if (is_bridgeless_component (scc)) { abort_unless (temporary_peers != nullptr, "Temporary peers must not be null for bridgeless SCCs"); - int index = scc_get_stashed_temporary_peer_index (scc); - jobject target = env->CallObjectMethod (temporary_peers, ArrayList_get, index); + ssize_t index = get_stashed_temporary_peer_index (scc); + abort_unless (index <= static_cast (std::numeric_limits::max ()), "Count cannot fit temporary peer index in an int."); + + int index_int = static_cast(index); + jobject target = env->CallObjectMethod (temporary_peers, ArrayList_get, index_int); - return GCBridge::CrossReferenceComponent { - .is_bridgeless_scc = true, - .target = target, - }; + return { .is_bridgeless_component = true, .temporary_peer = target }; } else { - return GCBridge::CrossReferenceComponent { - .is_bridgeless_scc = false, - .handle_context = scc->Contexts [0], - }; + abort_unless (scc->Count > 0, "SCC must have at least one context for non-bridgeless SCCs"); + + return { .is_bridgeless_component = false, .handle_context = scc->Contexts [0] }; } } void GCBridge::release_target (GCBridge::CrossReferenceComponent component) noexcept { - if (component.is_bridgeless_scc) { + if (component.is_bridgeless_component) { // Release the local ref for bridgeless SCCs - env->DeleteLocalRef (component.target); + env->DeleteLocalRef (component.temporary_peer); } } @@ -179,7 +168,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) env->CallBooleanMethod (temporary_peers, ArrayList_add, peer); env->DeleteLocalRef (peer); - scc_set_stashed_temporary_peer_index (scc, temporary_peer_count); + set_stashed_temporary_peer_index (scc, temporary_peer_count); temporary_peer_count++; } } @@ -227,13 +216,16 @@ void GCBridge::take_global_ref (HandleContext *context) noexcept // } if (handle != nullptr) { - context->is_collected = 0; + context->control_block->handle = handle; + context->control_block->handle_type = JNIGlobalRefType; + // _monodroid_gref_log_new (weak, get_object_ref_type (env, weak), // handle, get_object_ref_type (env, handle), // "finalizer", gettid (), // " at [[gc:take_global_ref_jni]]", 0); } else { - context->is_collected = 1; + context->control_block = nullptr; + // MonoClass *klass = mono_object_get_class (obj); // char *message = Util::monodroid_strdup_printf ( // "handle %p/W; key_handle %p; MCW type: `%s.%s`: was collected by a Java GC", @@ -245,8 +237,6 @@ void GCBridge::take_global_ref (HandleContext *context) noexcept // free (message); } - context->control_block->handle = handle; - context->control_block->handle_type = JNIGlobalRefType; // _monodroid_weak_gref_delete (weak, get_object_ref_type (env, weak), // "finalizer", gettid (), " at [[gc:take_global_ref_jni]]", 0); diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 81310150352..33db073d26b 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -18,7 +18,6 @@ struct JniObjectReferenceControlBlock struct HandleContext { intptr_t gc_handle; - int32_t is_collected; JniObjectReferenceControlBlock* control_block; }; @@ -72,9 +71,9 @@ namespace xamarin::android { struct CrossReferenceComponent { - bool is_bridgeless_scc; + bool is_bridgeless_component; union { - jobject target; + jobject temporary_peer; HandleContext* handle_context; }; }; @@ -93,16 +92,17 @@ namespace xamarin::android { static void bridge_processing () noexcept; static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; - static bool is_bridgeless_scc (StronglyConnectedComponent *scc) noexcept; + static bool is_bridgeless_component (StronglyConnectedComponent *scc) noexcept; static void add_inner_reference (HandleContext *from, HandleContext *to) noexcept; static void add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept; static bool add_reference (jobject from, jobject to) noexcept; static void clear_references (jobject handle) noexcept; + static GCBridge::CrossReferenceComponent get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; static void release_target (GCBridge::CrossReferenceComponent target) noexcept; - static int scc_get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; - static void scc_set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; + static ssize_t get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; + static void set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; From 9d1d44d73751daf5c71a96084324b941f446cfdf Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 10 Jun 2025 11:16:48 +0200 Subject: [PATCH 09/17] Add logging --- .../Android.Runtime/RuntimeNativeMethods.cs | 9 +- .../ManagedValueManager.cs | 5 +- src/native/clr/host/gc-bridge.cc | 96 +++++++++---------- src/native/clr/host/os-bridge.cc | 15 +++ src/native/clr/include/host/os-bridge.hh | 2 + src/native/clr/include/runtime-base/util.hh | 2 + src/native/clr/runtime-base/util.cc | 21 ++++ 7 files changed, 86 insertions(+), 64 deletions(-) diff --git a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs index a691ab8498a..ae7042b4efa 100644 --- a/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs +++ b/src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs @@ -93,17 +93,10 @@ internal unsafe static class RuntimeNativeMethods [DllImport (RuntimeConstants.InternalDllName, CallingConvention = CallingConvention.Cdecl)] internal static extern bool clr_typemap_java_to_managed (string java_type_name, out IntPtr managed_assembly_name, out uint managed_type_token_id); - /// - /// TODO: implement this in the native side. - /// Initializes the "GC Bridge" implementation for the CoreCLR runtime. - /// - /// A function pointer to a C# callback that will be invoked when bridge processing has completed. - /// A function pointer that should be passed to JavaMarshal.Initialize() on startup. [DllImport (RuntimeConstants.InternalDllName, CallingConvention = CallingConvention.Cdecl)] internal static extern delegate* unmanaged clr_initialize_gc_bridge ( delegate* unmanaged bridge_processing_started_callback, - delegate* unmanaged bridge_processing_finished_callback - ); + delegate* unmanaged bridge_processing_finished_callback); [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern void monodroid_unhandled_exception (Exception javaException); diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 205e2f11132..3471af50caf 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -89,10 +89,7 @@ public override void AddPeer (IJavaPeerable value) lock (RegisteredInstances) { List? peers; if (!RegisteredInstances.TryGetValue (key, out peers)) { - // Console.WriteLine ($"Adding new peer list for key {key} with PeerReference={value.PeerReference} in {GetType ().AssemblyQualifiedName}"); - peers = new List () { - CreateReferenceTrackingHandle (value) - }; + peers = [CreateReferenceTrackingHandle (value)]; RegisteredInstances.Add (key, peers); return; } diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index f271636ab67..6dd7360247b 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -207,60 +208,61 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) void GCBridge::take_global_ref (HandleContext *context) noexcept { abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, "Expected weak global reference type for handle"); - jobject weak = context->control_block->handle; + + jobject weak = context->control_block->handle; jobject handle = env->NewGlobalRef (weak); - // if (gref_log) { - // fprintf (gref_log, "*try_take_global obj=%p -> wref=%p handle=%p\n", obj, weak, handle); - // fflush (gref_log); - // } + + if (Logger::gref_log ()) [[unlikely]] { + char *message = Util::monodroid_strdup_printf ("*try_take_global gchandle=%p -> wref=%p handle=%p\n", context->gc_handle, weak, handle); + OSBridge::_monodroid_gref_log (message); + free (message); + } if (handle != nullptr) { context->control_block->handle = handle; context->control_block->handle_type = JNIGlobalRefType; - // _monodroid_gref_log_new (weak, get_object_ref_type (env, weak), - // handle, get_object_ref_type (env, handle), - // "finalizer", gettid (), - // " at [[gc:take_global_ref_jni]]", 0); + OSBridge::_monodroid_gref_log_new (weak, OSBridge::get_object_ref_type (env, weak), + handle, OSBridge::get_object_ref_type (env, handle), + "finalizer", gettid (), + " at [[clr-gc:take_global_ref]]", 0); + + OSBridge::_monodroid_weak_gref_delete (weak, OSBridge::get_object_ref_type (env, weak), + "finalizer", gettid (), " at [[clr-gc:take_global_ref]]", 0); + env->DeleteWeakGlobalRef (weak); } else { context->control_block = nullptr; - // MonoClass *klass = mono_object_get_class (obj); - // char *message = Util::monodroid_strdup_printf ( - // "handle %p/W; key_handle %p; MCW type: `%s.%s`: was collected by a Java GC", - // weak, - // nullptr, // ?? - // mono_class_get_namespace (klass), - // mono_class_get_name (klass)); - // _monodroid_gref_log (message); - // free (message); - } - + if (Logger::gc_spew_enabled ()) [[unlikely]] { + char *message = Util::monodroid_strdup_printf ("handle %p/W; was collected by a Java GC", weak); + OSBridge::_monodroid_gref_log (message); + free (message); + } - // _monodroid_weak_gref_delete (weak, get_object_ref_type (env, weak), - // "finalizer", gettid (), " at [[gc:take_global_ref_jni]]", 0); - env->DeleteWeakGlobalRef (weak); + // The weak reference will be deleted by JniObjectReferenceManager.DeleteWeakGlobalReference later + } } void GCBridge::take_weak_global_ref (HandleContext *context) noexcept { jobject handle = context->control_block->handle; - // if (gref_log) { - // fprintf (gref_log, "*take_weak obj=%p; handle=%p\n", obj, handle); - // fflush (gref_log); - // } + if (Logger::gref_log ()) [[unlikely]] { + char *message = Util::monodroid_strdup_printf ("*take_weak gchandle=%p; handle=%p\n", context->gc_handle, handle); + OSBridge::_monodroid_gref_log (message); + free (message); + } jobject weak = env->NewWeakGlobalRef (handle); - // _monodroid_weak_gref_new (handle, get_object_ref_type (env, handle), - // weak, get_object_ref_type (env, weak), - // "finalizer", gettid (), " at [[gc:take_weak_global_ref_jni]]", 0); + OSBridge::_monodroid_weak_gref_new (handle, OSBridge::get_object_ref_type (env, handle), + weak, OSBridge::get_object_ref_type (env, weak), + "finalizer", gettid (), " at [[clr-gc:take_weak_global_ref]]", 0); context->control_block->handle = weak; context->control_block->handle_type = JNIWeakGlobalRefType; - // _monodroid_gref_log_delete (handle, get_object_ref_type (env, handle), - // "finalizer", gettid (), " at [[gc:take_weak_global_ref_jni]]", 0); + OSBridge::_monodroid_gref_log_delete (handle, OSBridge::get_object_ref_type (env, handle), + "finalizer", gettid (), " at [[clr-gc:take_weak_global_ref]]", 0); env->DeleteGlobalRef (handle); } @@ -278,12 +280,9 @@ void GCBridge::clear_references (jobject handle) noexcept env->ExceptionClear (); #if DEBUG // if (Logger::gc_spew_enabled ()) { - // klass = mono_object_get_class (obj); - // log_error (LOG_GC, - // "Missing monodroidClearReferences method for object of class {}.{}", - // optional_string (mono_class_get_namespace (klass)), - // optional_string (mono_class_get_name (klass)) - // ); + // char *class_name = Host::get_java_class_name_for_TypeManager (java_class); + // log_error (LOG_GC, "Missing monodroidClearReferences method for object of class {}", class_name); + // free (class_name); // } #endif } @@ -314,30 +313,23 @@ for (size_t i = 0; i < cross_refs->ComponentCount; i++) bool is_alive = false; for (ssize_t j = 0; j < scc->Count; j++) { - HandleContext *context = scc->Contexts [j]; - jobject handle = context->control_block->handle; + JniObjectReferenceControlBlock *control_block = scc->Contexts [j]->control_block; - if (handle) { + if (control_block != nullptr) { #if DEBUG alive++; #endif if (j > 0) { - abort_unless ( - is_alive, - [&i] { return detail::_format_message ("Bridge SCC at index %d must be alive", i); } - ); + abort_unless (is_alive, [&i] { return detail::_format_message ("Bridge SCC at index %d must be alive", i); }); } is_alive = true; - if (context->control_block->refs_added > 0) { - clear_references (handle); - context->control_block->refs_added = 0; + if (control_block->refs_added > 0) { + clear_references (control_block->handle); + control_block->refs_added = 0; } } else { - abort_unless ( - !is_alive, - [&i] { return detail::_format_message ("Bridge SCC at index %d must NOT be alive", i); } - ); + abort_unless (!is_alive, [&i] { return detail::_format_message ("Bridge SCC at index %d must NOT be alive", i); }); } } } diff --git a/src/native/clr/host/os-bridge.cc b/src/native/clr/host/os-bridge.cc index 14974b7543f..b6b9009913e 100644 --- a/src/native/clr/host/os-bridge.cc +++ b/src/native/clr/host/os-bridge.cc @@ -49,6 +49,21 @@ auto OSBridge::lref_to_gref (JNIEnv *env, jobject lref) noexcept -> jobject return g; } +auto OSBridge::get_object_ref_type (JNIEnv *env, void *handle) noexcept -> char +{ + jobjectRefType value; + if (handle == nullptr) + return 'I'; + value = env->GetObjectRefType (reinterpret_cast (handle)); + switch (value) { + case JNIInvalidRefType: return 'I'; + case JNILocalRefType: return 'L'; + case JNIGlobalRefType: return 'G'; + case JNIWeakGlobalRefType: return 'W'; + default: return '*'; + } +} + auto OSBridge::_monodroid_gref_inc () noexcept -> int { return __sync_add_and_fetch (&gc_gref_count, 1); diff --git a/src/native/clr/include/host/os-bridge.hh b/src/native/clr/include/host/os-bridge.hh index b418a0ec208..edf9056990c 100644 --- a/src/native/clr/include/host/os-bridge.hh +++ b/src/native/clr/include/host/os-bridge.hh @@ -13,6 +13,7 @@ namespace xamarin::android { static void initialize_on_onload (JavaVM *vm, JNIEnv *env) noexcept; static void initialize_on_runtime_init (JNIEnv *env, jclass runtimeClass) noexcept; static auto lref_to_gref (JNIEnv *env, jobject lref) noexcept -> jobject; + static auto get_object_ref_type (JNIEnv *env, void *handle) noexcept -> char; static auto get_gc_gref_count () noexcept -> int { @@ -29,6 +30,7 @@ namespace xamarin::android { static void _monodroid_gref_log_delete (jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable) noexcept; static void _monodroid_weak_gref_new (jobject curHandle, char curType, jobject newHandle, char newType, const char *threadName, int threadId, const char *from, int from_writable); static void _monodroid_weak_gref_delete (jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable); + static void _monodroid_weak_gref_collected (jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable) noexcept; static void _monodroid_lref_log_new (int lrefc, jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable); static void _monodroid_lref_log_delete (int lrefc, jobject handle, char type, const char *threadName, int threadId, const char *from, int from_writable); diff --git a/src/native/clr/include/runtime-base/util.hh b/src/native/clr/include/runtime-base/util.hh index 0dc4316f24b..be2c5524834 100644 --- a/src/native/clr/include/runtime-base/util.hh +++ b/src/native/clr/include/runtime-base/util.hh @@ -59,6 +59,8 @@ namespace xamarin::android { static auto monodroid_fopen (std::string_view const& filename, std::string_view const& mode) noexcept -> FILE*; static void set_world_accessable (std::string_view const& path); static auto set_world_accessible (int fd) noexcept -> bool; + static auto monodroid_strdup_printf (const char *format, ...) -> char*; + static auto monodroid_strdup_vprintf (const char *format, va_list vargs) -> char*; // Puts higher half of the `value` byte as a hexadecimal character in `high_half` and // the lower half in `low_half` diff --git a/src/native/clr/runtime-base/util.cc b/src/native/clr/runtime-base/util.cc index 85c2b339ad4..66451c3e5be 100644 --- a/src/native/clr/runtime-base/util.cc +++ b/src/native/clr/runtime-base/util.cc @@ -105,3 +105,24 @@ auto Util::set_world_accessible (int fd) noexcept -> bool return true; } + +char * +Util::monodroid_strdup_printf (const char *format, ...) +{ + va_list args; + + va_start (args, format); + char *ret = monodroid_strdup_vprintf (format, args); + va_end (args); + + return ret; +} + +char* +Util::monodroid_strdup_vprintf (const char *format, va_list vargs) +{ + char *ret = nullptr; + int n = vasprintf (&ret, format, vargs); + + return n == -1 ? nullptr : ret; +} From b8aab1a7b986c3e39688d87c31f671c4c90b78f5 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Tue, 10 Jun 2025 14:04:33 +0200 Subject: [PATCH 10/17] Change property to a method --- .../Android.Runtime.NativeAOT/JavaInteropRuntime.cs | 2 +- .../Java.Interop/JreRuntime.cs | 2 +- src/Mono.Android/Android.Runtime/JNIEnvInit.cs | 2 +- .../Microsoft.Android.Runtime/ManagedValueManager.cs | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs index 98315cad67b..1dd5aabc4ff 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs @@ -42,7 +42,7 @@ static void init (IntPtr jnienv, IntPtr klass, IntPtr classLoader) EnvironmentPointer = jnienv, ClassLoader = new JniObjectReference (classLoader), TypeManager = new ManagedTypeManager (), - ValueManager = ManagedValueManager.Instance, // TODO this will likely blow up in AOT at runtime currently + ValueManager = ManagedValueManager.GetOrCreateInstance (), // TODO this will likely blow up in AOT at runtime currently UseMarshalMemberBuilder = false, JniGlobalReferenceLogWriter = settings.GrefLog, JniLocalReferenceLogWriter = settings.LrefLog, diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs index c5818f6d764..fa244eb67b9 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs @@ -61,7 +61,7 @@ static NativeAotRuntimeOptions CreateJreVM (NativeAotRuntimeOptions builder) builder.TypeManager ??= new ManagedTypeManager (); #endif // NET - builder.ValueManager ??= ManagedValueManager.Instance; + builder.ValueManager ??= ManagedValueManager.GetOrCreateInstance (); builder.ObjectReferenceManager ??= new ManagedObjectReferenceManager (builder.JniGlobalReferenceLogWriter, builder.JniLocalReferenceLogWriter); if (builder.InvocationPointer != IntPtr.Zero || builder.EnvironmentPointer != IntPtr.Zero) diff --git a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs index 84dc08aa9c6..6ec510ff886 100644 --- a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs +++ b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs @@ -118,7 +118,7 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args) } // TODO is there any reason why the ManagedTypeMap would need ManagedValueManager? // ManagedValueManager is specifically tied to the CoreCLR JavaMarshal APIs and it does not work with MonoVM. - valueManager = RuntimeType == DotNetRuntimeType.MonoVM ? new AndroidValueManager () : ManagedValueManager.Instance; + valueManager = RuntimeType == DotNetRuntimeType.MonoVM ? new AndroidValueManager () : ManagedValueManager.GetOrCreateInstance (); androidRuntime = new AndroidRuntime ( args->env, args->javaVm, diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 3471af50caf..e003833e1ba 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -24,7 +24,7 @@ class ManagedValueManager : JniRuntime.JniValueManager Dictionary>? RegisteredInstances = new (); private static Lazy s_instance = new (() => new ManagedValueManager ()); - public static ManagedValueManager Instance => s_instance.Value; + public static ManagedValueManager GetOrCreateInstance () => s_instance.Value; private unsafe ManagedValueManager () { @@ -458,6 +458,7 @@ internal static void BridgeProcessingStarted () internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* mcr) { List handlesToFree = []; + ManagedValueManager instance = GetOrCreateInstance (); for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) { @@ -472,7 +473,7 @@ internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* m // Cleanup: Remove the handle from RegisteredInstances if (handle.IsAllocated && handle.Target is IJavaPeerable target) { - Instance.RemoveRegisteredInstance (target, freeHandle: false); + instance.RemoveRegisteredInstance (target, freeHandle: false); } HandleContext.Free (ref context); From 7137e6d5535d06039fc1610a6a51bb967595a524 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 11 Jun 2025 11:35:27 +0200 Subject: [PATCH 11/17] Cleanup and more logging --- src/native/clr/host/gc-bridge.cc | 121 ++++++++++++++----------------- 1 file changed, 55 insertions(+), 66 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 6dd7360247b..0c0684b2325 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -16,15 +17,18 @@ void GCBridge::initialize_on_load (JNIEnv *jniEnv) noexcept abort_if_invalid_pointer_argument (jniEnv, "jniEnv"); jclass lref = jniEnv->FindClass ("java/lang/Runtime"); + abort_unless (lref != nullptr, "Failed to look up java/lang/Runtime class."); + jmethodID Runtime_getRuntime = jniEnv->GetStaticMethodID (lref, "getRuntime", "()Ljava/lang/Runtime;"); + abort_unless (Runtime_getRuntime != nullptr, "Failed to look up the Runtime.getRuntime() method."); + Runtime_gc = jniEnv->GetMethodID (lref, "gc", "()V"); + abort_unless (Runtime_gc != nullptr, "Failed to look up the Runtime.gc() method."); + Runtime_instance = OSBridge::lref_to_gref (jniEnv, jniEnv->CallStaticObjectMethod (lref, Runtime_getRuntime)); - jniEnv->DeleteLocalRef (lref); + abort_unless (Runtime_instance != nullptr, "Failed to obtain Runtime instance."); - abort_unless ( - Runtime_gc != nullptr && Runtime_instance != nullptr, - "Failed to look up Java GC runtime API." - ); + jniEnv->DeleteLocalRef (lref); } void GCBridge::trigger_java_gc () noexcept @@ -134,8 +138,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) // Before looking at xrefs, scan the SCCs. During collection, an SCC has to behave like a // single object. If the number of objects in the SCC is anything other than 1, the SCC // must be doctored to mimic that one-object nature. - for (size_t i = 0; i < cross_refs->ComponentCount; i++) - { + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; abort_unless (scc->Count >= 0, @@ -175,8 +178,7 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) } // Add the cross scc refs - for (size_t i = 0; i < cross_refs->CrossReferenceCount; i++) - { + for (size_t i = 0; i < cross_refs->CrossReferenceCount; i++) { ComponentCrossReference *xref = &cross_refs->CrossReferences [i]; GCBridge::CrossReferenceComponent from = get_target (&cross_refs->Components [xref->SourceGroupIndex], temporary_peers); @@ -232,6 +234,7 @@ void GCBridge::take_global_ref (HandleContext *context) noexcept "finalizer", gettid (), " at [[clr-gc:take_global_ref]]", 0); env->DeleteWeakGlobalRef (weak); } else { + // The native memory of the control block will be freed in managed code as well as the weak global ref context->control_block = nullptr; if (Logger::gc_spew_enabled ()) [[unlikely]] { @@ -239,8 +242,6 @@ void GCBridge::take_global_ref (HandleContext *context) noexcept OSBridge::_monodroid_gref_log (message); free (message); } - - // The weak reference will be deleted by JniObjectReferenceManager.DeleteWeakGlobalReference later } } @@ -279,11 +280,11 @@ void GCBridge::clear_references (jobject handle) noexcept log_error (LOG_DEFAULT, "Failed to find monodroidClearReferences method"); env->ExceptionClear (); #if DEBUG - // if (Logger::gc_spew_enabled ()) { - // char *class_name = Host::get_java_class_name_for_TypeManager (java_class); - // log_error (LOG_GC, "Missing monodroidClearReferences method for object of class {}", class_name); - // free (class_name); - // } + if (Logger::gc_spew_enabled ()) { + char *class_name = Host::get_java_class_name_for_TypeManager (java_class); + log_error (LOG_GC, "Missing monodroidClearReferences method for object of class {}", class_name); + free (class_name); + } #endif } } @@ -291,20 +292,19 @@ void GCBridge::clear_references (jobject handle) noexcept void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { #if DEBUG -int total = 0; -int alive = 0; + int total = 0; + int alive = 0; #endif -// try to switch back to global refs to analyze what stayed alive -for (size_t i = 0; i < cross_refs->ComponentCount; i++) -{ - StronglyConnectedComponent *scc = &cross_refs->Components [i]; - for (ssize_t j = 0; j < scc->Count; j++) { + // try to switch back to global refs to analyze what stayed alive + for (size_t i = 0; i < cross_refs->ComponentCount; i++) { + StronglyConnectedComponent *scc = &cross_refs->Components [i]; + for (ssize_t j = 0; j < scc->Count; j++) { #if DEBUG - total++; + total++; #endif - take_global_ref (scc->Contexts [j]); - } + take_global_ref (scc->Contexts [j]); + } } // clear the cross references on any remaining items @@ -346,52 +346,41 @@ void GCBridge::bridge_processing () noexcept env = OSBridge::ensure_jnienv (); - while (true) - { + while (true) { bridge_processing_semaphore.acquire (); std::unique_lock lock (processing_mutex); - // if (Logger::gc_spew_enabled ()) { - // int i, j; - // log_info (LOG_GC, "cross references callback invoked with {} sccs and {} xrefs.", num_sccs, num_xrefs); - - // for (i = 0; i < num_sccs; ++i) { - // log_info (LOG_GC, "group {} with {} objects", i, sccs [i]->num_objs); - // for (j = 0; j < sccs [i]->num_objs; ++j) { - // MonoObject *obj = sccs [i]->objs [j]; - - // JniObjectReferenceControlBlock *control_block = get_gc_control_block_for_object (obj); - // jobject handle = 0; - // void *key_handle = nullptr; - // if (control_block != nullptr) { - // mono_field_get_value (obj, reinterpret_cast(control_block->handle), &handle); - // if (control_block->weak_handle != nullptr) { - // mono_field_get_value (obj, reinterpret_cast(control_block->weak_handle), &key_handle); - // } - // } - // MonoClass *klass = mono_object_get_class (obj); - // log_info (LOG_GC, - // "\tobj {:p} [{}::{}] handle {:p} key_handle {:p}", - // reinterpret_cast(obj), - // optional_string (mono_class_get_namespace (klass)), - // optional_string (mono_class_get_name (klass)), - // reinterpret_cast(handle), - // key_handle - // ); - // } - // } - - // if (Util::should_log (LOG_GC)) { - // for (i = 0; i < num_xrefs; ++i) - // log_info_nocheck_fmt (LOG_GC, "xref [{}] {} -> {}", i, xrefs [i].src_scc_index, xrefs [i].dst_scc_index); - // } - // } + MarkCrossReferencesArgs* args = &GCBridge::shared_cross_refs; + + if (Logger::gc_spew_enabled ()) { + log_info (LOG_GC, "cross references callback invoked with {} sccs and {} xrefs.", args->ComponentCount, args->CrossReferenceCount); + + for (size_t i = 0; i < args->ComponentCount; ++i) { + log_info (LOG_GC, "group {} with {} objects", i, args->Components [i].Count); + for (ssize_t j = 0; j < args->Components [i].Count; ++j) { + HandleContext *ctx = args->Components [i].Contexts [j]; + jobject handle = ctx->control_block->handle; + jclass java_class = env->GetObjectClass (handle); + char *class_name = Host::get_java_class_name_for_TypeManager (java_class); + log_info (LOG_GC, "\tgchandle {:p} gref {:p} [{}]", reinterpret_cast (ctx->gc_handle), reinterpret_cast (handle), class_name); + free (class_name); + } + } + + if (Util::should_log (LOG_GC)) { + for (size_t i = 0; i < args->CrossReferenceCount; ++i) { + size_t source_index = args->CrossReferences [i].SourceGroupIndex; + size_t dest_index = args->CrossReferences [i].DestinationGroupIndex; + log_info_nocheck_fmt (LOG_GC, "xref [{}] {} -> {}", i, source_index, dest_index); + } + } + } bridge_processing_started_callback (); - prepare_for_java_collection (&GCBridge::shared_cross_refs); + prepare_for_java_collection (args); trigger_java_gc (); - cleanup_after_java_collection (&GCBridge::shared_cross_refs); - bridge_processing_finished_callback (&GCBridge::shared_cross_refs); + cleanup_after_java_collection (args); + bridge_processing_finished_callback (args); } } From b9a591b763f7fa44a090822d904e498d1cc32f3c Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 11 Jun 2025 11:47:42 +0200 Subject: [PATCH 12/17] Simplify prepare_for_java_collection --- src/native/clr/host/gc-bridge.cc | 56 ++++++++++++++---------- src/native/clr/include/host/gc-bridge.hh | 4 +- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 0c0684b2325..06bfde27841 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -147,33 +147,16 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) // Count > 1 case: The SCC contains many objects which must be collected as one. // Solution: Make all objects within the SCC directly or indirectly reference each other if (scc->Count > 1) { - HandleContext *first = scc->Contexts [0]; - HandleContext *prev = first; - - for (ssize_t j = 1; j < scc->Count; j++) { - HandleContext *current = scc->Contexts [j]; - add_inner_reference (prev, current); - prev = current; - } - - add_inner_reference (prev, first); + add_inner_references (scc); } else if (scc->Count == 0) { - // Once per process boot, look up JNI metadata we need to make temporary objects - ensure_array_list (); - - // Once per prepare_for_java_collection call, create a list to hold the temporary - // objects we create. This will protect them from collection while we build the list. if (!temporary_peers) { + // Once per prepare_for_java_collection call, create a list to hold the temporary + // objects we create. This will protect them from collection while we build the list. + ensure_array_list (); temporary_peers = env->NewObject (ArrayList_class, ArrayList_ctor); } - // Create this SCC's temporary object - jobject peer = env->NewObject (GCUserPeer_class, GCUserPeer_ctor); - env->CallBooleanMethod (temporary_peers, ArrayList_add, peer); - env->DeleteLocalRef (peer); - - set_stashed_temporary_peer_index (scc, temporary_peer_count); - temporary_peer_count++; + add_temporary_peer (scc, temporary_peers, temporary_peer_count); } } @@ -207,11 +190,37 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) } } +void GCBridge::add_inner_references (StronglyConnectedComponent *scc) noexcept +{ + abort_if_invalid_pointer_argument (scc, "scc"); + + HandleContext *first = scc->Contexts [0]; + HandleContext *prev = first; + + for (ssize_t j = 1; j < scc->Count; j++) { + HandleContext *current = scc->Contexts [j]; + add_inner_reference (prev, current); + prev = current; + } + + add_inner_reference (prev, first); +} + +void GCBridge::add_temporary_peer (StronglyConnectedComponent *scc, jobject temporary_peers, int &temporary_peer_count) noexcept +{ + // Create this SCC's temporary object + jobject peer = env->NewObject (GCUserPeer_class, GCUserPeer_ctor); + env->CallBooleanMethod (temporary_peers, ArrayList_add, peer); + env->DeleteLocalRef (peer); + + set_stashed_temporary_peer_index (scc, temporary_peer_count); + temporary_peer_count++; +} + void GCBridge::take_global_ref (HandleContext *context) noexcept { abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, "Expected weak global reference type for handle"); - jobject weak = context->control_block->handle; jobject handle = env->NewGlobalRef (weak); @@ -399,6 +408,7 @@ void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexc [[gnu::always_inline]] void GCBridge::ensure_array_list () noexcept { + // Once per process boot, look up JNI metadata we need to make temporary objects if (ArrayList_class == nullptr) [[unlikely]] { ArrayList_class = env->FindClass ("java/util/ArrayList"); abort_unless (ArrayList_class != nullptr, "Failed to find java/util/ArrayList class"); diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 33db073d26b..06eb3e3f850 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -106,7 +106,9 @@ namespace xamarin::android { static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; - + + static void add_inner_references (StronglyConnectedComponent *scc) noexcept; + static void add_temporary_peer (StronglyConnectedComponent *scc, jobject temporary_peers, int &temporary_peer_count) noexcept; static void take_weak_global_ref (HandleContext *context) noexcept; static void take_global_ref (HandleContext *context) noexcept; From 68b8379ad31d7ae3879644881666a42a3db9647d Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 11 Jun 2025 13:32:06 +0200 Subject: [PATCH 13/17] Drop temporary peers impl via ArrayList --- .../ManagedValueManager.cs | 36 ++-- src/native/clr/host/gc-bridge.cc | 169 +++--------------- src/native/clr/include/host/gc-bridge.hh | 32 +--- 3 files changed, 49 insertions(+), 188 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index e003833e1ba..b84b05b5c7b 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -135,8 +135,8 @@ public override void DisposePeer (IJavaPeerable value) return; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (p.IsAllocated && ReferenceEquals (value, p.Target)) + var handle = peers [i]; + if (handle.IsAllocated && ReferenceEquals (value, handle.Target)) { indexesToRemove.Add (i); } @@ -144,19 +144,19 @@ public override void DisposePeer (IJavaPeerable value) } // dispose the peer - base.DisposePeer(value); + base.DisposePeer (value); // and then clean up the registered instances lock (RegisteredInstances) { List? peers; - if (!RegisteredInstances.TryGetValue(key, out peers)) + if (!RegisteredInstances.TryGetValue (key, out peers)) return; foreach (int i in indexesToRemove) { // Remove the peer from the list - var p = peers[i]; - FreeReferenceTrackingHandle(p); - peers.RemoveAt(i); + var handle = peers[i]; + FreeReferenceTrackingHandle (handle); + peers.RemoveAt (i); } } } @@ -229,9 +229,9 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal return null; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (p.IsAllocated - && p.Target is IJavaPeerable peer + var handle = peers [i]; + if (handle.IsAllocated + && handle.Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) { return peer; @@ -268,11 +268,11 @@ private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) return; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers [i]; - if (p.IsAllocated && ReferenceEquals (target, p.Target)) { + var handle = peers [i]; + if (handle.IsAllocated && ReferenceEquals (target, handle.Target)) { peers.RemoveAt (i); if (freeHandle) - FreeReferenceTrackingHandle (p); + FreeReferenceTrackingHandle (handle); } } if (peers.Count == 0) @@ -298,12 +298,12 @@ private GCHandle PeekPeerHandle (JniObjectReference reference) return default; for (int i = peers.Count - 1; i >= 0; i--) { - var p = peers[i]; - if (p.IsAllocated - && p.Target is IJavaPeerable peer - && JniEnvironment.Types.IsSameObject(reference, peer.PeerReference)) + var handle = peers[i]; + if (handle.IsAllocated + && handle.Target is IJavaPeerable peer + && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) { - return p; + return handle; } } if (peers.Count == 0) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 06bfde27841..722b1fb8abd 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -4,6 +4,7 @@ #include #include #include +#include using namespace xamarin::android; @@ -41,26 +42,6 @@ void GCBridge::trigger_java_gc () noexcept } } -void GCBridge::add_inner_reference (HandleContext *from, HandleContext *to) noexcept -{ - jobject from_object = from->control_block->handle; - jobject to_object = to->control_block->handle; - - if (add_reference (from_object, to_object)) { - from->control_block->refs_added++; - } -} - -void GCBridge::add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept -{ - jobject from_object = from.is_bridgeless_component ? from.temporary_peer : from.handle_context->control_block->handle; - jobject to_object = to.is_bridgeless_component ? to.temporary_peer : to.handle_context->control_block->handle; - - if (add_reference (from_object, to_object) && !from.is_bridgeless_component) { - from.handle_context->control_block->refs_added++; - } -} - bool GCBridge::add_reference (jobject from, jobject to) noexcept { jclass java_class = env->GetObjectClass (from); @@ -76,64 +57,10 @@ bool GCBridge::add_reference (jobject from, jobject to) noexcept } } -ssize_t GCBridge::get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept -{ - abort_if_invalid_pointer_argument (scc, "scc"); - abort_unless (scc->Count < 0, "Attempted to load stashed index from an object which does not contain one."); - - return -scc->Count - 1; -} - -void GCBridge::set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept -{ - abort_if_invalid_pointer_argument (scc, "scc"); - abort_unless (index >= 0, "Index must be non-negative"); - - scc->Count = -index - 1; -} - -bool GCBridge::is_bridgeless_component (StronglyConnectedComponent *scc) noexcept -{ - abort_if_invalid_pointer_argument (scc, "scc"); - - // if we stashed a temporary peer index in Count (negative number), then this is a bridgeless SCC - return scc->Count < 0; -} - -GCBridge::CrossReferenceComponent GCBridge::get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept -{ - abort_if_invalid_pointer_argument (scc, "scc"); - - if (is_bridgeless_component (scc)) { - abort_unless (temporary_peers != nullptr, "Temporary peers must not be null for bridgeless SCCs"); - - ssize_t index = get_stashed_temporary_peer_index (scc); - abort_unless (index <= static_cast (std::numeric_limits::max ()), "Count cannot fit temporary peer index in an int."); - - int index_int = static_cast(index); - jobject target = env->CallObjectMethod (temporary_peers, ArrayList_get, index_int); - - return { .is_bridgeless_component = true, .temporary_peer = target }; - } else { - abort_unless (scc->Count > 0, "SCC must have at least one context for non-bridgeless SCCs"); - - return { .is_bridgeless_component = false, .handle_context = scc->Contexts [0] }; - } -} - -void GCBridge::release_target (GCBridge::CrossReferenceComponent component) noexcept -{ - if (component.is_bridgeless_component) { - // Release the local ref for bridgeless SCCs - env->DeleteLocalRef (component.temporary_peer); - } -} - void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { // Some SCCs might have no IGCUserPeers associated with them, so we must create one - jobject temporary_peers = nullptr; // This is an ArrayList - int temporary_peer_count = 0; // Number of items in temporary_peers + std::unordered_map temporary_peers; // Before looking at xrefs, scan the SCCs. During collection, an SCC has to behave like a // single object. If the number of objects in the SCC is anything other than 1, the SCC @@ -141,80 +68,57 @@ void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - abort_unless (scc->Count >= 0, - "Attempted to prepare for GC bridge processing where the number of strongly connected components is negative (likely due to overflow of ssize_t)."); - // Count > 1 case: The SCC contains many objects which must be collected as one. // Solution: Make all objects within the SCC directly or indirectly reference each other if (scc->Count > 1) { - add_inner_references (scc); + add_references (scc); } else if (scc->Count == 0) { - if (!temporary_peers) { - // Once per prepare_for_java_collection call, create a list to hold the temporary - // objects we create. This will protect them from collection while we build the list. - ensure_array_list (); - temporary_peers = env->NewObject (ArrayList_class, ArrayList_ctor); - } - - add_temporary_peer (scc, temporary_peers, temporary_peer_count); + temporary_peers [scc] = env->NewObject (GCUserPeer_class, GCUserPeer_ctor); } } // Add the cross scc refs for (size_t i = 0; i < cross_refs->CrossReferenceCount; i++) { ComponentCrossReference *xref = &cross_refs->CrossReferences [i]; - - GCBridge::CrossReferenceComponent from = get_target (&cross_refs->Components [xref->SourceGroupIndex], temporary_peers); - GCBridge::CrossReferenceComponent to = get_target (&cross_refs->Components [xref->DestinationGroupIndex], temporary_peers); - add_cross_reference (from, to); + StronglyConnectedComponent *source = &cross_refs->Components [xref->SourceGroupIndex]; + jobject from = source->Count > 0 ? source->Contexts [0]->control_block->handle : temporary_peers [source]; + + StronglyConnectedComponent *destination = &cross_refs->Components [xref->DestinationGroupIndex]; + jobject to = destination->Count > 0 ? destination->Contexts [0]->control_block->handle : temporary_peers [destination]; - release_target (from); - release_target (to); + add_reference (from, to); } - // With xrefs processed, the temporary peer list can be released - env->DeleteLocalRef (temporary_peers); + // With cross references processed, the temporary peer list can be released + for (const auto& [scc, temporary_peer] : temporary_peers) { + env->DeleteLocalRef (temporary_peer); + } // Switch global to weak references for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - - // Reset any temporary peer index stored in Count - if (scc->Count < 0) - scc->Count = 0; - - for (ssize_t j = 0; j < scc->Count; j++) { + for (size_t j = 0; j < scc->Count; j++) { take_weak_global_ref (scc->Contexts [j]); } } } -void GCBridge::add_inner_references (StronglyConnectedComponent *scc) noexcept +void GCBridge::add_references (StronglyConnectedComponent *scc) noexcept { abort_if_invalid_pointer_argument (scc, "scc"); + abort_unless (scc->Count > 1, "SCC must have at least two items to add inner references"); + + JniObjectReferenceControlBlock *prev = scc->Contexts [scc->Count - 1]->control_block; - HandleContext *first = scc->Contexts [0]; - HandleContext *prev = first; + for (size_t j = 1; j < scc->Count; j++) { + JniObjectReferenceControlBlock *current = scc->Contexts [j]->control_block; + if (add_reference (prev->handle, current->handle)) { + prev->refs_added++; + } - for (ssize_t j = 1; j < scc->Count; j++) { - HandleContext *current = scc->Contexts [j]; - add_inner_reference (prev, current); prev = current; } - - add_inner_reference (prev, first); -} - -void GCBridge::add_temporary_peer (StronglyConnectedComponent *scc, jobject temporary_peers, int &temporary_peer_count) noexcept -{ - // Create this SCC's temporary object - jobject peer = env->NewObject (GCUserPeer_class, GCUserPeer_ctor); - env->CallBooleanMethod (temporary_peers, ArrayList_add, peer); - env->DeleteLocalRef (peer); - - set_stashed_temporary_peer_index (scc, temporary_peer_count); - temporary_peer_count++; } void GCBridge::take_global_ref (HandleContext *context) noexcept @@ -308,7 +212,7 @@ void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_ref // try to switch back to global refs to analyze what stayed alive for (size_t i = 0; i < cross_refs->ComponentCount; i++) { StronglyConnectedComponent *scc = &cross_refs->Components [i]; - for (ssize_t j = 0; j < scc->Count; j++) { + for (size_t j = 0; j < scc->Count; j++) { #if DEBUG total++; #endif @@ -321,7 +225,7 @@ void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_ref StronglyConnectedComponent *scc = &cross_refs->Components [i]; bool is_alive = false; - for (ssize_t j = 0; j < scc->Count; j++) { + for (size_t j = 0; j < scc->Count; j++) { JniObjectReferenceControlBlock *control_block = scc->Contexts [j]->control_block; if (control_block != nullptr) { @@ -366,7 +270,7 @@ void GCBridge::bridge_processing () noexcept for (size_t i = 0; i < args->ComponentCount; ++i) { log_info (LOG_GC, "group {} with {} objects", i, args->Components [i].Count); - for (ssize_t j = 0; j < args->Components [i].Count; ++j) { + for (size_t j = 0; j < args->Components [i].Count; ++j) { HandleContext *ctx = args->Components [i].Contexts [j]; jobject handle = ctx->control_block->handle; jclass java_class = env->GetObjectClass (handle); @@ -404,22 +308,3 @@ void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexc bridge_processing_semaphore.release (); } - -[[gnu::always_inline]] -void GCBridge::ensure_array_list () noexcept -{ - // Once per process boot, look up JNI metadata we need to make temporary objects - if (ArrayList_class == nullptr) [[unlikely]] { - ArrayList_class = env->FindClass ("java/util/ArrayList"); - abort_unless (ArrayList_class != nullptr, "Failed to find java/util/ArrayList class"); - - ArrayList_ctor = env->GetMethodID (ArrayList_class, "", "()V"); - abort_unless (ArrayList_ctor != nullptr, "Failed to find ArrayList constructor"); - - ArrayList_get = env->GetMethodID (ArrayList_class, "get", "(I)Ljava/lang/Object;"); - abort_unless (ArrayList_get != nullptr, "Failed to find ArrayList get method"); - - ArrayList_add = env->GetMethodID (ArrayList_class, "add", "(Ljava/lang/Object;)Z"); - abort_unless (ArrayList_add != nullptr, "Failed to find ArrayList add method"); - } -} diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 06eb3e3f850..9d8e204d0dd 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -23,7 +23,7 @@ struct HandleContext struct StronglyConnectedComponent { - ssize_t Count; // We need to use ssize_t instead of size_t to allow negative values for bridgeless SCCs + size_t Count; HandleContext** Contexts; }; @@ -69,15 +69,6 @@ namespace xamarin::android { return mark_cross_references; } - struct CrossReferenceComponent - { - bool is_bridgeless_component; - union { - jobject temporary_peer; - HandleContext* handle_context; - }; - }; - private: static inline BridgeProcessingStartedFtn bridge_processing_started_callback = nullptr; static inline BridgeProcessingFinishedFtn bridge_processing_finished_callback = nullptr; @@ -91,38 +82,23 @@ namespace xamarin::android { static void bridge_processing () noexcept; static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; - - static bool is_bridgeless_component (StronglyConnectedComponent *scc) noexcept; - static void add_inner_reference (HandleContext *from, HandleContext *to) noexcept; - static void add_cross_reference (GCBridge::CrossReferenceComponent from, GCBridge::CrossReferenceComponent to) noexcept; + + static void add_references (StronglyConnectedComponent *scc) noexcept; static bool add_reference (jobject from, jobject to) noexcept; static void clear_references (jobject handle) noexcept; - static GCBridge::CrossReferenceComponent get_target (StronglyConnectedComponent *scc, jobject temporary_peers) noexcept; - static void release_target (GCBridge::CrossReferenceComponent target) noexcept; - - static ssize_t get_stashed_temporary_peer_index (StronglyConnectedComponent *scc) noexcept; - static void set_stashed_temporary_peer_index (StronglyConnectedComponent *scc, ssize_t index) noexcept; - static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; - static void add_inner_references (StronglyConnectedComponent *scc) noexcept; - static void add_temporary_peer (StronglyConnectedComponent *scc, jobject temporary_peers, int &temporary_peer_count) noexcept; static void take_weak_global_ref (HandleContext *context) noexcept; static void take_global_ref (HandleContext *context) noexcept; static void trigger_java_gc () noexcept; + static inline jobject Runtime_instance = nullptr; static inline jmethodID Runtime_gc = nullptr; static inline jclass GCUserPeer_class = nullptr; static inline jmethodID GCUserPeer_ctor = nullptr; - - static void ensure_array_list () noexcept; - static inline jclass ArrayList_class = nullptr; - static inline jmethodID ArrayList_ctor = nullptr; - static inline jmethodID ArrayList_get = nullptr; - static inline jmethodID ArrayList_add = nullptr; }; } From 90ca1ceeebae0ba406650e4c44fd1eeaa267397a Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 11 Jun 2025 14:07:17 +0200 Subject: [PATCH 14/17] Fix --- src/native/clr/host/gc-bridge.cc | 141 ++++++++++++----------- src/native/clr/include/host/gc-bridge.hh | 8 +- 2 files changed, 78 insertions(+), 71 deletions(-) diff --git a/src/native/clr/host/gc-bridge.cc b/src/native/clr/host/gc-bridge.cc index 722b1fb8abd..308811ff1dc 100644 --- a/src/native/clr/host/gc-bridge.cc +++ b/src/native/clr/host/gc-bridge.cc @@ -8,11 +8,6 @@ using namespace xamarin::android; -void GCBridge::wait_for_bridge_processing () noexcept -{ - std::shared_lock lock (processing_mutex); -} - void GCBridge::initialize_on_load (JNIEnv *jniEnv) noexcept { abort_if_invalid_pointer_argument (jniEnv, "jniEnv"); @@ -42,21 +37,6 @@ void GCBridge::trigger_java_gc () noexcept } } -bool GCBridge::add_reference (jobject from, jobject to) noexcept -{ - jclass java_class = env->GetObjectClass (from); - jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); - env->DeleteLocalRef (java_class); - - if (add_method_id) { - env->CallVoidMethod (from, add_method_id, to); - return true; - } else { - env->ExceptionClear (); - return false; - } -} - void GCBridge::prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { // Some SCCs might have no IGCUserPeers associated with them, so we must create one @@ -121,6 +101,43 @@ void GCBridge::add_references (StronglyConnectedComponent *scc) noexcept } } +bool GCBridge::add_reference (jobject from, jobject to) noexcept +{ + jclass java_class = env->GetObjectClass (from); + jmethodID add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); + env->DeleteLocalRef (java_class); + + if (add_method_id) { + env->CallVoidMethod (from, add_method_id, to); + return true; + } else { + env->ExceptionClear (); + return false; + } +} + +void GCBridge::clear_references (jobject handle) noexcept +{ + // Clear references from the object + jclass java_class = env->GetObjectClass (handle); + jmethodID clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V"); + env->DeleteLocalRef (java_class); // Clean up the local reference to the class + + if (clear_method_id) { + env->CallVoidMethod (handle, clear_method_id); + } else { + log_error (LOG_DEFAULT, "Failed to find monodroidClearReferences method"); + env->ExceptionClear (); +#if DEBUG + if (Logger::gc_spew_enabled ()) { + char *class_name = Host::get_java_class_name_for_TypeManager (java_class); + log_error (LOG_GC, "Missing monodroidClearReferences method for object of class {}", class_name); + free (class_name); + } +#endif + } +} + void GCBridge::take_global_ref (HandleContext *context) noexcept { abort_unless (context->control_block->handle_type == JNIWeakGlobalRefType, "Expected weak global reference type for handle"); @@ -129,7 +146,7 @@ void GCBridge::take_global_ref (HandleContext *context) noexcept jobject handle = env->NewGlobalRef (weak); if (Logger::gref_log ()) [[unlikely]] { - char *message = Util::monodroid_strdup_printf ("*try_take_global gchandle=%p -> wref=%p handle=%p\n", context->gc_handle, weak, handle); + char *message = Util::monodroid_strdup_printf ("take_global_ref gchandle=%p -> wref=%p handle=%p\n", context->gc_handle, weak, handle); OSBridge::_monodroid_gref_log (message); free (message); } @@ -162,7 +179,7 @@ void GCBridge::take_weak_global_ref (HandleContext *context) noexcept { jobject handle = context->control_block->handle; if (Logger::gref_log ()) [[unlikely]] { - char *message = Util::monodroid_strdup_printf ("*take_weak gchandle=%p; handle=%p\n", context->gc_handle, handle); + char *message = Util::monodroid_strdup_printf ("take_weak_global_ref gchandle=%p; handle=%p\n", context->gc_handle, handle); OSBridge::_monodroid_gref_log (message); free (message); } @@ -180,28 +197,6 @@ void GCBridge::take_weak_global_ref (HandleContext *context) noexcept env->DeleteGlobalRef (handle); } -void GCBridge::clear_references (jobject handle) noexcept -{ - // Clear references from the object - jclass java_class = env->GetObjectClass (handle); - jmethodID clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V"); - env->DeleteLocalRef (java_class); // Clean up the local reference to the class - - if (clear_method_id) { - env->CallVoidMethod (handle, clear_method_id); - } else { - log_error (LOG_DEFAULT, "Failed to find monodroidClearReferences method"); - env->ExceptionClear (); -#if DEBUG - if (Logger::gc_spew_enabled ()) { - char *class_name = Host::get_java_class_name_for_TypeManager (java_class); - log_error (LOG_GC, "Missing monodroidClearReferences method for object of class {}", class_name); - free (class_name); - } -#endif - } -} - void GCBridge::cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept { #if DEBUG @@ -264,30 +259,7 @@ void GCBridge::bridge_processing () noexcept std::unique_lock lock (processing_mutex); MarkCrossReferencesArgs* args = &GCBridge::shared_cross_refs; - - if (Logger::gc_spew_enabled ()) { - log_info (LOG_GC, "cross references callback invoked with {} sccs and {} xrefs.", args->ComponentCount, args->CrossReferenceCount); - - for (size_t i = 0; i < args->ComponentCount; ++i) { - log_info (LOG_GC, "group {} with {} objects", i, args->Components [i].Count); - for (size_t j = 0; j < args->Components [i].Count; ++j) { - HandleContext *ctx = args->Components [i].Contexts [j]; - jobject handle = ctx->control_block->handle; - jclass java_class = env->GetObjectClass (handle); - char *class_name = Host::get_java_class_name_for_TypeManager (java_class); - log_info (LOG_GC, "\tgchandle {:p} gref {:p} [{}]", reinterpret_cast (ctx->gc_handle), reinterpret_cast (handle), class_name); - free (class_name); - } - } - - if (Util::should_log (LOG_GC)) { - for (size_t i = 0; i < args->CrossReferenceCount; ++i) { - size_t source_index = args->CrossReferences [i].SourceGroupIndex; - size_t dest_index = args->CrossReferences [i].DestinationGroupIndex; - log_info_nocheck_fmt (LOG_GC, "xref [{}] {} -> {}", i, source_index, dest_index); - } - } - } + log_mark_cross_references_args_if_enabled (args); bridge_processing_started_callback (); prepare_for_java_collection (args); @@ -308,3 +280,36 @@ void GCBridge::mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexc bridge_processing_semaphore.release (); } + +void GCBridge::wait_for_bridge_processing () noexcept +{ + std::shared_lock lock (processing_mutex); +} + +[[gnu::always_inline]] +void GCBridge::log_mark_cross_references_args_if_enabled (MarkCrossReferencesArgs* args) noexcept +{ + if (Logger::gc_spew_enabled ()) [[unlikely]] { + log_info (LOG_GC, "cross references callback invoked with {} sccs and {} xrefs.", args->ComponentCount, args->CrossReferenceCount); + + for (size_t i = 0; i < args->ComponentCount; ++i) { + log_info (LOG_GC, "group {} with {} objects", i, args->Components [i].Count); + for (size_t j = 0; j < args->Components [i].Count; ++j) { + HandleContext *ctx = args->Components [i].Contexts [j]; + jobject handle = ctx->control_block->handle; + jclass java_class = env->GetObjectClass (handle); + char *class_name = Host::get_java_class_name_for_TypeManager (java_class); + log_info (LOG_GC, "\tgchandle {:p} gref {:p} [{}]", reinterpret_cast (ctx->gc_handle), reinterpret_cast (handle), class_name); + free (class_name); + } + } + + if (Util::should_log (LOG_GC)) { + for (size_t i = 0; i < args->CrossReferenceCount; ++i) { + size_t source_index = args->CrossReferences [i].SourceGroupIndex; + size_t dest_index = args->CrossReferences [i].DestinationGroupIndex; + log_info_nocheck_fmt (LOG_GC, "xref [{}] {} -> {}", i, source_index, dest_index); + } + } + } +} diff --git a/src/native/clr/include/host/gc-bridge.hh b/src/native/clr/include/host/gc-bridge.hh index 9d8e204d0dd..2960d41cd36 100644 --- a/src/native/clr/include/host/gc-bridge.hh +++ b/src/native/clr/include/host/gc-bridge.hh @@ -81,14 +81,14 @@ namespace xamarin::android { static inline std::thread* bridge_processing_thread = nullptr; static void bridge_processing () noexcept; - static void mark_cross_references (MarkCrossReferencesArgs* cross_refs) noexcept; + static void mark_cross_references (MarkCrossReferencesArgs *cross_refs) noexcept; static void add_references (StronglyConnectedComponent *scc) noexcept; static bool add_reference (jobject from, jobject to) noexcept; static void clear_references (jobject handle) noexcept; - static void prepare_for_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; - static void cleanup_after_java_collection (MarkCrossReferencesArgs* cross_refs) noexcept; + static void prepare_for_java_collection (MarkCrossReferencesArgs *cross_refs) noexcept; + static void cleanup_after_java_collection (MarkCrossReferencesArgs *cross_refs) noexcept; static void take_weak_global_ref (HandleContext *context) noexcept; static void take_global_ref (HandleContext *context) noexcept; @@ -100,5 +100,7 @@ namespace xamarin::android { static inline jclass GCUserPeer_class = nullptr; static inline jmethodID GCUserPeer_ctor = nullptr; + + static void log_mark_cross_references_args_if_enabled (MarkCrossReferencesArgs *cross_refs) noexcept; }; } From 317bc801d1d6448d883abc51450ffafce0402e46 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Wed, 11 Jun 2025 16:38:36 +0200 Subject: [PATCH 15/17] Remove unused method --- .../ManagedValueManager.cs | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index b84b05b5c7b..60523613d20 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -280,38 +280,6 @@ private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) } } - private GCHandle PeekPeerHandle (JniObjectReference reference) - { - if (RegisteredInstances == null) - throw new ObjectDisposedException (nameof (ManagedValueManager)); - - if (!reference.IsValid) - return default; - - WaitForGCBridgeProcessing (); - - int key = GetJniIdentityHashCode (reference); - - lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) - return default; - - for (int i = peers.Count - 1; i >= 0; i--) { - var handle = peers[i]; - if (handle.IsAllocated - && handle.Target is IJavaPeerable peer - && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) - { - return handle; - } - } - if (peers.Count == 0) - RegisteredInstances.Remove (key); - } - return default; - } - public override void FinalizePeer (IJavaPeerable value) { WaitForGCBridgeProcessing (); From d8da1b88bc30314a6f98fb2194ce22771e2e3128 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 13 Jun 2025 09:57:11 +0200 Subject: [PATCH 16/17] Simplify code --- .../ManagedValueManager.cs | 186 ++++-------------- 1 file changed, 34 insertions(+), 152 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 60523613d20..4b8b367f4f2 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -38,36 +38,9 @@ public override void WaitForGCBridgeProcessing () AndroidRuntimeInternal.WaitForBridgeProcessing (); } - public override void CollectPeers () + public override void CollectPeers() { - if (RegisteredInstances == null) - throw new ObjectDisposedException (nameof (ManagedValueManager)); - - var peers = new List (); - - lock (RegisteredInstances) { - foreach (var ps in RegisteredInstances.Values) { - foreach (var p in ps) { - peers.Add (p); - } - } - RegisteredInstances.Clear (); - } - List? exceptions = null; - foreach (var handle in peers) { - try { - if (handle.IsAllocated) - (handle.Target as IDisposable)?.Dispose (); - } - catch (Exception e) { - exceptions = exceptions ?? new List (); - exceptions.Add (e); - } - } - if (exceptions != null) - throw new AggregateException ("Exceptions while collecting peers.", exceptions); - - GC.Collect (); + // Intentionally left empty. } public override void AddPeer (IJavaPeerable value) @@ -75,8 +48,6 @@ public override void AddPeer (IJavaPeerable value) if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); - WaitForGCBridgeProcessing (); - var r = value.PeerReference; if (!r.IsValid) throw new ObjectDisposedException (value.GetType ().FullName); @@ -96,14 +67,18 @@ public override void AddPeer (IJavaPeerable value) for (int i = peers.Count - 1; i >= 0; i--) { var p = peers [i]; - if (!p.IsAllocated) - continue; if (p.Target is not IJavaPeerable peer) continue; if (!JniEnvironment.Types.IsSameObject (peer.PeerReference, value.PeerReference)) continue; if (Replaceable (p)) { - FreeReferenceTrackingHandle (p); + // We need to take special care here so that we don't free a handle which will be collected + // by the garbage collector at the same time. + if (p.Target is IJavaPeerable replacedPeer) { + FreeReferenceTrackingHandle (p); + GC.KeepAlive (replacedPeer); + } + peers [i] = CreateReferenceTrackingHandle (value); } else { WarnNotReplacing (key, value, peer); @@ -115,85 +90,8 @@ public override void AddPeer (IJavaPeerable value) } } - public override void DisposePeer (IJavaPeerable value) - { - if (value == null) - throw new ArgumentNullException (nameof (value)); - - if (RegisteredInstances == null) - throw new ObjectDisposedException (nameof (ManagedValueManager)); - - WaitForGCBridgeProcessing (); - - // start by collecting peers that need to be removed later - int key = value.JniIdentityHashCode; - List indexesToRemove = []; - - lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) - return; - - for (int i = peers.Count - 1; i >= 0; i--) { - var handle = peers [i]; - if (handle.IsAllocated && ReferenceEquals (value, handle.Target)) - { - indexesToRemove.Add (i); - } - } - } - - // dispose the peer - base.DisposePeer (value); - - // and then clean up the registered instances - lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) - return; - - foreach (int i in indexesToRemove) { - // Remove the peer from the list - var handle = peers[i]; - FreeReferenceTrackingHandle (handle); - peers.RemoveAt (i); - } - } - } - - public override void DisposePeerUnlessReferenced (IJavaPeerable value) - { - if (RegisteredInstances == null) - throw new ObjectDisposedException (GetType ().Name); - - if (value == null) - throw new ArgumentNullException (nameof (value)); - - var h = value.PeerReference; - if (!h.IsValid) - return; - - var o = PeekPeer (h); - if (o != null && object.ReferenceEquals (o, value)) - return; - - // This is the only difference from base.DisposePeerUnlessReferenced: - // - we're calling the virtual `DisposePeer` instead of the private - // one in the base class - DisposePeer (value); - } - - static bool Replaceable (GCHandle handle) - { - if (!handle.IsAllocated) - return false; - - var target = handle.Target as IJavaPeerable; - if (target is null) - return false; - - return target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable); - } + static bool Replaceable(GCHandle handle) + => handle.Target is IJavaPeerable peer && peer.JniManagedPeerState.HasFlag(JniManagedPeerStates.Replaceable); void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue) { @@ -219,26 +117,22 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal if (!reference.IsValid) return null; - WaitForGCBridgeProcessing (); - int key = GetJniIdentityHashCode (reference); lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) + if (!RegisteredInstances.TryGetValue (key, out List? peers)) return null; for (int i = peers.Count - 1; i >= 0; i--) { - var handle = peers [i]; - if (handle.IsAllocated - && handle.Target is IJavaPeerable peer + if (peers [i].Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) { return peer; } } + if (peers.Count == 0) - RegisteredInstances.Remove (key); + RegisteredInstances.Remove(key); } return null; } @@ -251,16 +145,19 @@ public override void RemovePeer (IJavaPeerable value) if (value == null) throw new ArgumentNullException (nameof (value)); - WaitForGCBridgeProcessing (); - - RemoveRegisteredInstance (value, freeHandle: true); + RemoveRegisteredInstance (value, out GCHandle? removedHandle); + if (removedHandle.HasValue) { + FreeReferenceTrackingHandle (removedHandle.Value); + } } - private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) + private void RemoveRegisteredInstance (IJavaPeerable target, out GCHandle? removedHandle) { if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); + removedHandle = default; + int key = target.JniIdentityHashCode; lock (RegisteredInstances) { List? peers; @@ -269,10 +166,9 @@ private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) for (int i = peers.Count - 1; i >= 0; i--) { var handle = peers [i]; - if (handle.IsAllocated && ReferenceEquals (target, handle.Target)) { + if (ReferenceEquals (target, handle.Target)) { peers.RemoveAt (i); - if (freeHandle) - FreeReferenceTrackingHandle (handle); + removedHandle = handle; } } if (peers.Count == 0) @@ -282,8 +178,6 @@ private void RemoveRegisteredInstance (IJavaPeerable target, bool freeHandle) public override void FinalizePeer (IJavaPeerable value) { - WaitForGCBridgeProcessing (); - var h = value.PeerReference; var o = Runtime.ObjectReferenceManager; // MUST NOT use SafeHandle.ReferenceType: local refs are tied to a JniEnvironment @@ -320,8 +214,6 @@ public override void FinalizePeer (IJavaPeerable value) public override void ActivatePeer (IJavaPeerable? self, JniObjectReference reference, ConstructorInfo cinfo, object?[]? argumentValues) { - WaitForGCBridgeProcessing (); - try { ActivateViaReflection (reference, cinfo, argumentValues); } catch (Exception e) { @@ -360,13 +252,11 @@ public override List GetSurfacedPeers () if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); - WaitForGCBridgeProcessing (); - lock (RegisteredInstances) { var peers = new List (RegisteredInstances.Count); foreach (var e in RegisteredInstances) { foreach (var p in e.Value) { - if (p.IsAllocated && p.Target is IJavaPeerable peer) { + if (p.Target is IJavaPeerable peer) { peers.Add (new JniSurfacedPeerInfo (e.Key, new WeakReference (peer))); } } @@ -376,19 +266,19 @@ public override List GetSurfacedPeers () } [StructLayout (LayoutKind.Sequential)] - unsafe struct HandleContext + struct HandleContext { public IntPtr GCHandle; public IntPtr ControlBlock; static readonly nuint Size = (nuint) Marshal.SizeOf (); - public static HandleContext* AllocZeroed () + public unsafe static HandleContext* AllocZeroed () { return (HandleContext*)NativeMemory.AllocZeroed (1, Size); } - public static void Free (ref HandleContext* ctx) + public unsafe static void Free (ref HandleContext* ctx) { if (ctx == null) return; @@ -433,17 +323,14 @@ internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* m var context = (HandleContext*) mcr->Components [i].Contexts [j]; if (context->ControlBlock == IntPtr.Zero) { var handle = GCHandle.FromIntPtr (context->GCHandle); + handlesToFree.Add (handle); - // Only free handles that haven't been freed yet - if (handle.IsAllocated && JavaMarshal.GetContext (handle) != null) { - handlesToFree.Add (handle); - } - - // Cleanup: Remove the handle from RegisteredInstances - if (handle.IsAllocated && handle.Target is IJavaPeerable target) { - instance.RemoveRegisteredInstance (target, freeHandle: false); + // Remove the handle from RegisteredInstances, but don't free the handle + if (handle.Target is IJavaPeerable target) { + instance.RemoveRegisteredInstance (target, out _); } + // Free the context but DON'T free the handle HandleContext.Free (ref context); } } @@ -464,8 +351,6 @@ protected override bool TryConstructPeer ( [DynamicallyAccessedMembers (Constructors)] Type type) { - WaitForGCBridgeProcessing (); - var c = type.GetConstructor (ActivationConstructorBindingFlags, null, XAConstructorSignature, null); if (c != null) { var args = new object[] { @@ -481,10 +366,7 @@ protected override bool TryConstructPeer ( protected override bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (true)]out object? result) { - WaitForGCBridgeProcessing (); - - var proxy = value as JavaProxyThrowable; - if (proxy != null) { + if (value is JavaProxyThrowable proxy) { result = proxy.InnerException; return true; } From 3ea928cfc18d7aaa01840562ebdc83e5a1070b15 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 13 Jun 2025 14:41:07 +0200 Subject: [PATCH 17/17] Use WeakReference to avoid race conditions --- .../ManagedValueManager.cs | 154 +++++++++--------- 1 file changed, 73 insertions(+), 81 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index 4b8b367f4f2..626ffdb8a3c 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -21,12 +21,12 @@ class ManagedValueManager : JniRuntime.JniValueManager { const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; - Dictionary>? RegisteredInstances = new (); + Dictionary>? RegisteredInstances = new (); - private static Lazy s_instance = new (() => new ManagedValueManager ()); + static Lazy s_instance = new (() => new ManagedValueManager ()); public static ManagedValueManager GetOrCreateInstance () => s_instance.Value; - private unsafe ManagedValueManager () + unsafe ManagedValueManager () { // There can only be one instance of ManagedValueManager because we can call JavaMarshal.Initialize only once. var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingStarted, &BridgeProcessingFinished); @@ -38,9 +38,9 @@ public override void WaitForGCBridgeProcessing () AndroidRuntimeInternal.WaitForBridgeProcessing (); } - public override void CollectPeers() + public override void CollectPeers () { - // Intentionally left empty. + // GC.Collect (); } public override void AddPeer (IJavaPeerable value) @@ -58,9 +58,9 @@ public override void AddPeer (IJavaPeerable value) } int key = value.JniIdentityHashCode; lock (RegisteredInstances) { - List? peers; + List? peers; if (!RegisteredInstances.TryGetValue (key, out peers)) { - peers = [CreateReferenceTrackingHandle (value)]; + peers = [new WeakPeerReference (value)]; RegisteredInstances.Add (key, peers); return; } @@ -71,28 +71,19 @@ public override void AddPeer (IJavaPeerable value) continue; if (!JniEnvironment.Types.IsSameObject (peer.PeerReference, value.PeerReference)) continue; - if (Replaceable (p)) { - // We need to take special care here so that we don't free a handle which will be collected - // by the garbage collector at the same time. - if (p.Target is IJavaPeerable replacedPeer) { - FreeReferenceTrackingHandle (p); - GC.KeepAlive (replacedPeer); - } - - peers [i] = CreateReferenceTrackingHandle (value); + if (peer.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { + p.Dispose (); + peers [i] = new WeakPeerReference (value); } else { WarnNotReplacing (key, value, peer); } return; } - peers.Add (CreateReferenceTrackingHandle (value)); + peers.Add (new WeakPeerReference (value)); } } - static bool Replaceable(GCHandle handle) - => handle.Target is IJavaPeerable peer && peer.JniManagedPeerState.HasFlag(JniManagedPeerStates.Replaceable); - void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepValue) { Runtime.ObjectReferenceManager.WriteGlobalReferenceLine ( @@ -114,14 +105,16 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal if (RegisteredInstances == null) throw new ObjectDisposedException (nameof (ManagedValueManager)); - if (!reference.IsValid) + if (!reference.IsValid) { return null; + } int key = GetJniIdentityHashCode (reference); lock (RegisteredInstances) { - if (!RegisteredInstances.TryGetValue (key, out List? peers)) + if (!RegisteredInstances.TryGetValue (key, out List? peers)) { return null; + } for (int i = peers.Count - 1; i >= 0; i--) { if (peers [i].Target is IJavaPeerable peer @@ -132,7 +125,7 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal } if (peers.Count == 0) - RegisteredInstances.Remove(key); + RegisteredInstances.Remove (key); } return null; } @@ -145,35 +138,29 @@ public override void RemovePeer (IJavaPeerable value) if (value == null) throw new ArgumentNullException (nameof (value)); - RemoveRegisteredInstance (value, out GCHandle? removedHandle); - if (removedHandle.HasValue) { - FreeReferenceTrackingHandle (removedHandle.Value); + lock (RegisteredInstances) { + RemoveRegisteredInstance (value, out WeakPeerReference? removedPeer); + removedPeer?.Dispose (); } } - private void RemoveRegisteredInstance (IJavaPeerable target, out GCHandle? removedHandle) + private void RemoveRegisteredInstance (IJavaPeerable target, out WeakPeerReference? removedPeer) { - if (RegisteredInstances == null) - throw new ObjectDisposedException (nameof (ManagedValueManager)); - - removedHandle = default; + removedPeer = default; int key = target.JniIdentityHashCode; - lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) - return; + if (!RegisteredInstances.TryGetValue (key, out List? peers)) + return; - for (int i = peers.Count - 1; i >= 0; i--) { - var handle = peers [i]; - if (ReferenceEquals (target, handle.Target)) { - peers.RemoveAt (i); - removedHandle = handle; - } + for (int i = peers.Count - 1; i >= 0; i--) { + var peer = peers [i]; + if (ReferenceEquals (target, peer.Target)) { + peers.RemoveAt (i); + removedPeer = peer; } - if (peers.Count == 0) - RegisteredInstances.Remove (key); } + if (peers.Count == 0) + RegisteredInstances.Remove (key); } public override void FinalizePeer (IJavaPeerable value) @@ -265,45 +252,51 @@ public override List GetSurfacedPeers () } } - [StructLayout (LayoutKind.Sequential)] - struct HandleContext + unsafe struct WeakPeerReference : IDisposable { - public IntPtr GCHandle; - public IntPtr ControlBlock; - - static readonly nuint Size = (nuint) Marshal.SizeOf (); + private WeakReference _weakReference; + private GCHandle _handle; - public unsafe static HandleContext* AllocZeroed () + public WeakPeerReference (IJavaPeerable peer) { - return (HandleContext*)NativeMemory.AllocZeroed (1, Size); + _weakReference = new WeakReference (peer); + + var handleContext = (HandleContext*) NativeMemory.AllocZeroed (1, HandleContext.Size); + if (handleContext == null) { + throw new OutOfMemoryException ("Failed to allocate memory for HandleContext."); + } + + _handle = JavaMarshal.CreateReferenceTrackingHandle (peer, handleContext); + + handleContext->Handle = GCHandle.ToIntPtr (_handle); + handleContext->ControlBlock = peer.JniObjectReferenceControlBlock; } - public unsafe static void Free (ref HandleContext* ctx) + public void FreeContext () { - if (ctx == null) - return; + var context = (HandleContext*) JavaMarshal.GetContext (_handle); + if (context != null) { + NativeMemory.Free (context); + } + } - NativeMemory.Free (ctx); - ctx = null; + public void Dispose () + { + FreeContext (); + _handle.Free (); } - } - static unsafe void FreeReferenceTrackingHandle (GCHandle handle) - { - var context = (HandleContext*) JavaMarshal.GetContext (handle); - handle.Free (); - HandleContext.Free (ref context); + public IJavaPeerable? Target + => _weakReference.TryGetTarget (out var target) ? target : null; } - static unsafe GCHandle CreateReferenceTrackingHandle (IJavaPeerable value) + [StructLayout (LayoutKind.Sequential)] + struct HandleContext { - var ctx = HandleContext.AllocZeroed (); - var handle = JavaMarshal.CreateReferenceTrackingHandle (value, ctx); + public static readonly nuint Size = (nuint) Marshal.SizeOf (); - ctx->GCHandle = GCHandle.ToIntPtr (handle); - ctx->ControlBlock = value.JniObjectReferenceControlBlock; - - return handle; + public IntPtr Handle; + public IntPtr ControlBlock; } [UnmanagedCallersOnly] @@ -318,20 +311,19 @@ internal static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* m List handlesToFree = []; ManagedValueManager instance = GetOrCreateInstance (); - for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { - for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) { - var context = (HandleContext*) mcr->Components [i].Contexts [j]; - if (context->ControlBlock == IntPtr.Zero) { - var handle = GCHandle.FromIntPtr (context->GCHandle); - handlesToFree.Add (handle); + lock (instance.RegisteredInstances) { + for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { + for (int j = 0; (nuint)j < mcr->Components [i].Count; j++) { + var context = (HandleContext*) mcr->Components [i].Contexts [j]; + if (context->ControlBlock == IntPtr.Zero) { + var handle = GCHandle.FromIntPtr (context->Handle); - // Remove the handle from RegisteredInstances, but don't free the handle - if (handle.Target is IJavaPeerable target) { - instance.RemoveRegisteredInstance (target, out _); - } + // Clean up the RegisteredInstances dictionary + free the control block native memory associated with the handle + instance.RemoveRegisteredInstance ((IJavaPeerable)handle.Target, out WeakPeerReference? removedPeer); + removedPeer?.FreeContext (); - // Free the context but DON'T free the handle - HandleContext.Free (ref context); + handlesToFree.Add (handle); + } } } }