Skip to content

[CoreCLR/NativeAOT] Simplify synchronization in ManagedValueManager #10796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
simonrozsival wants to merge 6 commits into
base: main
Choose a base branch
from
+71 −81
Open

[CoreCLR/NativeAOT] Simplify synchronization in ManagedValueManager #10796

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aca68c9
Remove bridgeProcessingSemaphore, embed GCHandle in HandleContext, dr…
simonrozsival Feb 11, 2026
6061907
Schedule CollectPeers on thread pool after bridge processing
simonrozsival Feb 11, 2026
6f8125e
Use dedicated System.Threading.Lock for RegisteredInstances
simonrozsival Feb 11, 2026
185d0b3
Apply field naming conventions: _prefix for instance, s_prefix for st…
simonrozsival Feb 11, 2026
2e3f56d
Refactor singleton access and remove synchronous cleanup from WaitFor…
Copilot Feb 13, 2026
c87e318
Fix remaining GetOrCreateInstance() references to Instance
simonrozsival Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 65 additions & 76 deletions src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Java;
using System.Threading;
using System.Threading.Tasks;
using Android.Runtime;
using Java.Interop;

Expand All @@ -22,12 +23,11 @@ class ManagedValueManager : JniRuntime.JniValueManager
{
const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

readonly Dictionary<int, List<ReferenceTrackingHandle>> RegisteredInstances = new ();
readonly ConcurrentQueue<IntPtr> CollectedContexts = new ();
readonly Lock _registeredInstancesLock = new ();
readonly Dictionary<int, List<ReferenceTrackingHandle>> _registeredInstances = new ();
readonly ConcurrentQueue<IntPtr> _collectedContexts = new ();

bool disposed;

static readonly SemaphoreSlim bridgeProcessingSemaphore = new (1, 1);
bool _disposed;

static Lazy<ManagedValueManager> s_instance = new (() => new ManagedValueManager ());
public static ManagedValueManager GetOrCreateInstance () => s_instance.Value;
Expand All @@ -41,31 +41,29 @@ unsafe ManagedValueManager ()

protected override void Dispose (bool disposing)
{
disposed = true;
_disposed = true;
base.Dispose (disposing);
}

void ThrowIfDisposed ()
{
if (disposed)
if (_disposed)
throw new ObjectDisposedException (nameof (ManagedValueManager));
}

public override void WaitForGCBridgeProcessing ()
{
bridgeProcessingSemaphore.Wait ();
bridgeProcessingSemaphore.Release ();
}

public unsafe override void CollectPeers ()
{
ThrowIfDisposed ();

while (CollectedContexts.TryDequeue (out IntPtr contextPtr)) {
Debug.Assert (contextPtr != IntPtr.Zero, "CollectedContexts should not contain null pointers.");
while (_collectedContexts.TryDequeue (out IntPtr contextPtr)) {
Debug.Assert (contextPtr != IntPtr.Zero, "_collectedContexts should not contain null pointers.");
HandleContext* context = (HandleContext*)contextPtr;

lock (RegisteredInstances) {
lock (_registeredInstancesLock) {
Remove (context);
}

Expand All @@ -75,7 +73,7 @@ public unsafe override void CollectPeers ()
void Remove (HandleContext* context)
{
int key = context->PeerIdentityHashCode;
if (!RegisteredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
if (!_registeredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
return;

for (int i = peers.Count - 1; i >= 0; i--) {
Expand All @@ -86,7 +84,7 @@ void Remove (HandleContext* context)
}

if (peers.Count == 0) {
RegisteredInstances.Remove (key);
_registeredInstances.Remove (key);
}
}
}
Expand All @@ -95,9 +93,6 @@ public override void AddPeer (IJavaPeerable value)
{
ThrowIfDisposed ();

// Remove any collected contexts before adding a new peer.
CollectPeers ();

var r = value.PeerReference;
if (!r.IsValid)
throw new ObjectDisposedException (value.GetType ().FullName);
Expand All @@ -107,11 +102,11 @@ public override void AddPeer (IJavaPeerable value)
JniObjectReference.Dispose (ref r, JniObjectReferenceOptions.CopyAndDispose);
}
int key = value.JniIdentityHashCode;
lock (RegisteredInstances) {
lock (_registeredInstancesLock) {
List<ReferenceTrackingHandle>? peers;
if (!RegisteredInstances.TryGetValue (key, out peers)) {
if (!_registeredInstances.TryGetValue (key, out peers)) {
peers = [new ReferenceTrackingHandle (value)];
RegisteredInstances.Add (key, peers);
_registeredInstances.Add (key, peers);
return;
}

Expand Down Expand Up @@ -160,8 +155,8 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal

int key = GetJniIdentityHashCode (reference);

lock (RegisteredInstances) {
if (!RegisteredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
lock (_registeredInstancesLock) {
if (!_registeredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
return null;

for (int i = peers.Count - 1; i >= 0; i--) {
Expand All @@ -173,7 +168,7 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal
}

if (peers.Count == 0)
RegisteredInstances.Remove (key);
_registeredInstances.Remove (key);
}
return null;
}
Expand All @@ -182,28 +177,28 @@ public override void RemovePeer (IJavaPeerable value)
{
ThrowIfDisposed ();

// Remove any collected contexts before modifying RegisteredInstances
// Remove any collected contexts before modifying _registeredInstances
CollectPeers ();

if (value == null)
throw new ArgumentNullException (nameof (value));

lock (RegisteredInstances) {
lock (_registeredInstancesLock) {
int key = value.JniIdentityHashCode;
if (!RegisteredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
if (!_registeredInstances.TryGetValue (key, out List<ReferenceTrackingHandle>? peers))
return;

for (int i = peers.Count - 1; i >= 0; i--) {
ReferenceTrackingHandle peer = peers [i];
IJavaPeerable target = peer.Target;
IJavaPeerable? target = peer.Target;
if (ReferenceEquals (value, target)) {
peers.RemoveAt (i);
peer.Dispose ();
}
GC.KeepAlive (target);
}
if (peers.Count == 0)
RegisteredInstances.Remove (key);
_registeredInstances.Remove (key);
}
}

Expand Down Expand Up @@ -284,9 +279,9 @@ public override List<JniSurfacedPeerInfo> GetSurfacedPeers ()
// Remove any collected contexts before iterating over all the registered instances
CollectPeers ();

lock (RegisteredInstances) {
var peers = new List<JniSurfacedPeerInfo> (RegisteredInstances.Count);
foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) {
lock (_registeredInstancesLock) {
var peers = new List<JniSurfacedPeerInfo> (_registeredInstances.Count);
foreach (var (identityHashCode, referenceTrackingHandles) in _registeredInstances) {
foreach (var peer in referenceTrackingHandles) {
if (peer.Target is IJavaPeerable target) {
peers.Add (new JniSurfacedPeerInfo (identityHashCode, new WeakReference<IJavaPeerable> (target)));
Expand All @@ -299,7 +294,7 @@ public override List<JniSurfacedPeerInfo> GetSurfacedPeers ()

unsafe struct ReferenceTrackingHandle : IDisposable
{
WeakReference<IJavaPeerable> _weakReference;
WeakReference<IJavaPeerable?> _weakReference;
HandleContext* _context;

public bool BelongsToContext (HandleContext* context)
Expand All @@ -308,7 +303,7 @@ public bool BelongsToContext (HandleContext* context)
public ReferenceTrackingHandle (IJavaPeerable peer)
{
_context = HandleContext.Alloc (peer);
_weakReference = new WeakReference<IJavaPeerable> (peer);
_weakReference = new (peer);
}

public IJavaPeerable? Target
Expand All @@ -321,7 +316,7 @@ public void Dispose ()

IJavaPeerable? target = Target;

GCHandle handle = HandleContext.GetAssociatedGCHandle (_context);
GCHandle handle = _context->AssociatedGCHandle;
HandleContext.Free (ref _context);
_weakReference.SetTarget (null);
if (handle.IsAllocated) {
Expand All @@ -337,20 +332,20 @@ public void Dispose ()
unsafe struct HandleContext
{
static readonly nuint Size = (nuint)Marshal.SizeOf<HandleContext> ();
static readonly Dictionary<IntPtr, GCHandle> referenceTrackingHandles = new ();

int identityHashCode;
IntPtr controlBlock;
int _identityHashCode;
IntPtr _controlBlock;
IntPtr _gcHandle; // GCHandle stored as IntPtr to keep blittable layout

public int PeerIdentityHashCode => identityHashCode;
public int PeerIdentityHashCode => _identityHashCode;
public bool IsCollected
{
get
{
if (controlBlock == IntPtr.Zero)
if (_controlBlock == IntPtr.Zero)
throw new InvalidOperationException ("HandleContext control block is not initialized.");

return ((JniObjectReferenceControlBlock*) controlBlock)->handle == IntPtr.Zero;
return ((JniObjectReferenceControlBlock*) _controlBlock)->handle == IntPtr.Zero;
}
}

Expand All @@ -363,40 +358,25 @@ private struct JniObjectReferenceControlBlock
public int refs_added;
}

public static GCHandle GetAssociatedGCHandle (HandleContext* context)
{
lock (referenceTrackingHandles) {
if (!referenceTrackingHandles.TryGetValue ((IntPtr) context, out GCHandle handle)) {
throw new InvalidOperationException ("Unknown reference tracking handle.");
}
public GCHandle AssociatedGCHandle => GCHandle.FromIntPtr (_gcHandle);

return handle;
}
}
#if DEBUG
static readonly HashSet<IntPtr> s_knownContexts = new ();

public static unsafe void EnsureAllContextsAreOurs (MarkCrossReferencesArgs* mcr)
{
lock (referenceTrackingHandles) {
lock (s_knownContexts) {
for (nuint i = 0; i < mcr->ComponentCount; i++) {
StronglyConnectedComponent component = mcr->Components [i];
EnsureAllContextsInComponentAreOurs (component);
}
}

static void EnsureAllContextsInComponentAreOurs (StronglyConnectedComponent component)
{
for (nuint i = 0; i < component.Count; i++) {
EnsureContextIsOurs ((IntPtr)component.Contexts [i]);
for (nuint j = 0; j < component.Count; j++) {
if (!s_knownContexts.Contains ((IntPtr)component.Contexts [j])) {
throw new InvalidOperationException ("Unknown reference tracking handle.");
}
}
}
}

static void EnsureContextIsOurs (IntPtr context)
{
if (!referenceTrackingHandles.ContainsKey (context)) {
throw new InvalidOperationException ("Unknown reference tracking handle.");
}
}
}
#endif

public static HandleContext* Alloc (IJavaPeerable peer)
{
Expand All @@ -405,13 +385,17 @@ static void EnsureContextIsOurs (IntPtr context)
throw new OutOfMemoryException ("Failed to allocate memory for HandleContext.");
}

context->identityHashCode = peer.JniIdentityHashCode;
context->controlBlock = peer.JniObjectReferenceControlBlock;
context->_identityHashCode = peer.JniIdentityHashCode;
context->_controlBlock = peer.JniObjectReferenceControlBlock;

GCHandle handle = JavaMarshal.CreateReferenceTrackingHandle (peer, context);
lock (referenceTrackingHandles) {
referenceTrackingHandles [(IntPtr) context] = handle;
context->_gcHandle = GCHandle.ToIntPtr (handle);

#if DEBUG
lock (s_knownContexts) {
s_knownContexts.Add ((IntPtr)context);
}
#endif

return context;
}
Expand All @@ -422,9 +406,11 @@ public static void Free (ref HandleContext* context)
return;
}

lock (referenceTrackingHandles) {
referenceTrackingHandles.Remove ((IntPtr)context);
#if DEBUG
lock (s_knownContexts) {
s_knownContexts.Remove ((IntPtr)context);
}
#endif

NativeMemory.Free (context);
context = null;
Expand All @@ -438,8 +424,9 @@ static unsafe void BridgeProcessingStarted (MarkCrossReferencesArgs* mcr)
throw new ArgumentNullException (nameof (mcr), "MarkCrossReferencesArgs should never be null.");
}

#if DEBUG
HandleContext.EnsureAllContextsAreOurs (mcr);
bridgeProcessingSemaphore.Wait ();
#endif
}

[UnmanagedCallersOnly]
Expand All @@ -452,7 +439,9 @@ static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* mcr)
ReadOnlySpan<GCHandle> handlesToFree = ProcessCollectedContexts (mcr);
JavaMarshal.FinishCrossReferenceProcessing (mcr, handlesToFree);

bridgeProcessingSemaphore.Release ();
// Schedule cleanup of _registeredInstances on a thread pool thread.
// The bridge thread must not take lock(_registeredInstances) — see deadlock notes.
Task.Run (GetOrCreateInstance ().CollectPeers);
Copy link
Member

@jonathanpeppers jonathanpeppers Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this call GetOrCreateInstance()? Is there a way to do the equivalent of Get() and return early if none found?

Copy link
Member

@jonathanpeppers jonathanpeppers Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static ManagedValueManager GetOrCreateInstance () => s_instance.Value; actually we might rename this to be Instance to help confusion in the future.

}

static unsafe ReadOnlySpan<GCHandle> ProcessCollectedContexts (MarkCrossReferencesArgs* mcr)
Expand All @@ -478,12 +467,12 @@ void ProcessContext (HandleContext* context)
return;
}

GCHandle handle = HandleContext.GetAssociatedGCHandle (context);
GCHandle handle = context->AssociatedGCHandle;

// Note: modifying the RegisteredInstances dictionary while processing the collected contexts
// Note: modifying the _registeredInstances dictionary while processing the collected contexts
// is tricky and can lead to deadlocks, so we remember which contexts were collected and we will free
// them later outside of the bridge processing loop.
instance.CollectedContexts.Enqueue ((IntPtr)context);
instance._collectedContexts.Enqueue ((IntPtr)context);

// important: we must not free the handle before passing it to JavaMarshal.FinishCrossReferenceProcessing
handlesToFree.Add (handle);
Expand Down
Loading