diff --git a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs index cf6f550b5fd..bfa7952d5b6 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs @@ -6,6 +6,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.InteropServices.Java; +using System.Threading; using Android.Runtime; using Java.Interop; @@ -37,6 +38,75 @@ static class JavaMarshalRegisteredPeers static readonly object initializeLock = new (); static bool initialized; + /// + /// Signaled whenever GC bridge processing is *not* in progress. + /// + /// + /// + /// This is the CoreCLR/NativeAOT equivalent of Mono's + /// mono_gc_wait_for_bridge_processing(), which blocks on the SGen GC lock held + /// for the duration of a bridge round. A bridge round flips every registered peer's + /// global reference strong->weak, asks ART to collect + /// (java.lang.Runtime.gc()), and then flips the survivors back. The wait exists + /// to match Mono's semantics: threads entering managed code from Java do not observe + /// peers mid-flip. + /// + /// + /// It is *not* a performance optimization. It was measured on a MAUI/SkiaSharp + /// benchmark (Pixel 5, arm64, 60 Hz, two runs per arm) with the barrier enabled and + /// disabled, and it made no useful difference: ART "Explicit concurrent copying GC" + /// wall time was 24.7/27.1 ms enabled vs 24.6/21.6 ms disabled, worst frame gap + /// 53.6/53.6 ms vs 54.3/57.4 ms. Both arms were far from the Mono baseline on the same + /// SDK (10.8-11.5 ms GC, 33.5 ms worst gap) even though the JNI global reference count + /// was identical (~840) on all arms. Whatever makes an ART bridge collection ~2x more + /// expensive under CoreCLR, it is not mutator activity during the round. + /// + /// + /// is constructed with spinCount: 0 on + /// purpose: the goal is to park the waiting thread rather than burn CPU that ART's + /// collector could use. + /// + /// + static readonly ManualResetEventSlim bridgeProcessingFinished = new (initialState: true, spinCount: 0); + + /// + /// of the bridge processing thread + /// while a round is in progress, otherwise 0. + /// + static int bridgeProcessingThreadId; + + /// + /// Blocks until any in-progress GC bridge round completes. + /// + /// + /// + /// Like Mono's implementation, this is inherently racy: a thread that observes "no + /// bridge processing" can still be preempted by a round that begins immediately + /// afterwards. The wait narrows the window, it does not close it. + /// + /// + /// In practice exactly one thread parks per round in a typical UI app (measured: + /// 15-36 ms per round), so this trades one unbounded stall for one bounded one + /// without reducing total GC cost. + /// + /// + internal static void WaitForBridgeProcessing () + { + if (!RuntimeFeature.WaitForGCBridgeProcessing) + return; + + var finished = bridgeProcessingFinished; + if (finished.IsSet) + return; + + // The bridge thread runs BridgeProcessingStarted/Finished itself, so it must + // never wait on its own completion signal. + if (Environment.CurrentManagedThreadId == Volatile.Read (ref bridgeProcessingThreadId)) + return; + + finished.Wait (); + } + /// /// Performs the one-shot, process-global GC-bridge initialization the first time it is /// called; subsequent calls return immediately. See @@ -404,6 +474,11 @@ static unsafe void BridgeProcessingStarted (MarkCrossReferencesArgs* mcr) throw new ArgumentNullException (nameof (mcr), "MarkCrossReferencesArgs should never be null."); } + // Publish the thread id before clearing the signal so that a reentrant call on + // this thread can never observe an unsignaled event with a stale id. + Volatile.Write (ref bridgeProcessingThreadId, Environment.CurrentManagedThreadId); + bridgeProcessingFinished.Reset (); + HandleContext.EnsureAllContextsAreOurs (mcr); } @@ -414,12 +489,19 @@ static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* mcr) throw new ArgumentNullException (nameof (mcr), "MarkCrossReferencesArgs should never be null."); } - ReadOnlySpan handlesToFree = ProcessCollectedContexts (mcr); + try { + ReadOnlySpan handlesToFree = ProcessCollectedContexts (mcr); // This call site is reachable on all platforms. 'JavaMarshal.FinishCrossReferenceProcessing(MarkCrossReferencesArgs*, ReadOnlySpan)' is only supported on: 'android'. #pragma warning disable CA1416 - JavaMarshal.FinishCrossReferenceProcessing (mcr, handlesToFree); + JavaMarshal.FinishCrossReferenceProcessing (mcr, handlesToFree); #pragma warning restore CA1416 + } finally { + // Must run even if processing throws, otherwise every thread that entered + // managed code from Java would block forever. + Volatile.Write (ref bridgeProcessingThreadId, 0); + bridgeProcessingFinished.Set (); + } } static unsafe ReadOnlySpan ProcessCollectedContexts (MarkCrossReferencesArgs* mcr) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs index e83d20fd046..b9cc01e0dbf 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs @@ -23,12 +23,13 @@ public JavaMarshalValueManager () public override void WaitForGCBridgeProcessing () { - // Intentionally empty. The Mono runtime's own implementation acknowledges this - // pattern is fundamentally flawed (see FIXME in sgen-bridge.c): a thread that - // passes the check can still race with bridge processing that starts immediately - // after. The wait cannot prevent the race, only reduce its window. On CoreCLR, - // JNI wrapper threads hold their own handle copies via JniObjectReference, so - // they are not affected by the bridge swapping control_block handles. + // Note the caveat from https://github.com/dotnet/android/pull/11119, which removed + // this wait: it cannot actually prevent the race it appears to guard, only shrink + // the window, and CoreCLR's JNI wrapper threads hold their own handle copies via + // JniObjectReference so they do not observe the bridge swapping control_block + // handles. It is restored purely to match Mono's observable blocking semantics -- + // it was measured to be performance-neutral. See JavaMarshalRegisteredPeers. + JavaMarshalRegisteredPeers.WaitForBridgeProcessing (); } public override void CollectPeers () diff --git a/src/Mono.Android/Microsoft.Android.Runtime/RuntimeFeature.cs b/src/Mono.Android/Microsoft.Android.Runtime/RuntimeFeature.cs index 718f0b6f656..7c9856b2766 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/RuntimeFeature.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/RuntimeFeature.cs @@ -13,6 +13,7 @@ static class RuntimeFeature const bool TrimmableTypeMapEnabledByDefault = false; const bool ObjectReferenceLoggingEnabledByDefault = false; const bool ManagedToJavaUsesAssemblyFullNameEnabledByDefault = false; + const bool WaitForGCBridgeProcessingEnabledByDefault = true; const string FeatureSwitchPrefix = "Microsoft.Android.Runtime.RuntimeFeature."; const string StartupHookProviderSwitch = "System.StartupHookProvider.IsSupported"; @@ -50,4 +51,10 @@ static class RuntimeFeature [FeatureSwitchDefinition ($"{FeatureSwitchPrefix}{nameof (ManagedToJavaUsesAssemblyFullName)}")] internal static bool ManagedToJavaUsesAssemblyFullName { get; } = AppContext.TryGetSwitch ($"{FeatureSwitchPrefix}{nameof (ManagedToJavaUsesAssemblyFullName)}", out bool isEnabled) ? isEnabled : ManagedToJavaUsesAssemblyFullNameEnabledByDefault; + + // When disabled, the trimmer can remove the GC bridge barrier and every + // WaitForGCBridgeProcessing() call site becomes an empty method body. + [FeatureSwitchDefinition ($"{FeatureSwitchPrefix}{nameof (WaitForGCBridgeProcessing)}")] + internal static bool WaitForGCBridgeProcessing { get; } = + AppContext.TryGetSwitch ($"{FeatureSwitchPrefix}{nameof (WaitForGCBridgeProcessing)}", out bool isEnabled) ? isEnabled : WaitForGCBridgeProcessingEnabledByDefault; } diff --git a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs index b2344f8fd2a..880789fe7c8 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs @@ -22,12 +22,13 @@ public TrimmableTypeMapValueManager () public override void WaitForGCBridgeProcessing () { - // Intentionally empty. The Mono runtime's own implementation acknowledges this - // pattern is fundamentally flawed (see FIXME in sgen-bridge.c): a thread that - // passes the check can still race with bridge processing that starts immediately - // after. The wait cannot prevent the race, only reduce its window. On CoreCLR, - // JNI wrapper threads hold their own handle copies via JniObjectReference, so - // they are not affected by the bridge swapping control_block handles. + // Note the caveat from https://github.com/dotnet/android/pull/11119, which removed + // this wait: it cannot actually prevent the race it appears to guard, only shrink + // the window, and CoreCLR's JNI wrapper threads hold their own handle copies via + // JniObjectReference so they do not observe the bridge swapping control_block + // handles. It is restored purely to match Mono's observable blocking semantics -- + // it was measured to be performance-neutral. See JavaMarshalRegisteredPeers. + JavaMarshalRegisteredPeers.WaitForBridgeProcessing (); } public override void CollectPeers () diff --git a/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.RuntimeConfig.targets b/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.RuntimeConfig.targets index fe4db586982..e57bbdd2278 100644 --- a/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.RuntimeConfig.targets +++ b/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.RuntimeConfig.targets @@ -19,6 +19,11 @@ See: https://github.com/dotnet/runtime/blob/b13715b6984889a709ba29ea8a1961db469f <_AndroidEnableDiagnosticCrashReporting Condition=" '$(_AndroidEnableDiagnosticCrashReporting)' == '' ">true + + <_AndroidWaitForGCBridgeProcessing Condition=" '$(_AndroidWaitForGCBridgeProcessing)' == '' ">true @@ -63,6 +68,11 @@ See: https://github.com/dotnet/runtime/blob/b13715b6984889a709ba29ea8a1961db469f Value="$(_AndroidEnableObjectReferenceLogging)" Trim="true" /> +