[WIP] GC bridge integration for CoreCLR #10185
Conversation
| if (peer.Target is IDisposable disposable) | ||
| disposable.Dispose (); | ||
| if (handle.IsAllocated) | ||
| (handle.Target as IDisposable)?.Dispose (); |
There was a problem hiding this comment.
This seems a bit weird, but I guess this method is only called from Tests so we don't care too much ?
| } | ||
|
|
||
| public override void AddPeer (IJavaPeerable value) | ||
| { | ||
| if (RegisteredInstances == null) | ||
| throw new ObjectDisposedException (nameof (ManagedValueManager)); | ||
|
|
||
| WaitForGCBridgeProcessing (); |
There was a problem hiding this comment.
Not sure what race this wait is meant to prevent. Even if we do the wait here, the code below could still race with a bridge collection ?
| } | ||
| } | ||
|
|
||
| static unsafe void FreeReferenceTrackingHandle (GCHandle handle) |
There was a problem hiding this comment.
Ideally, we shouldn't free the handle ourselves, rather let the runtime do it for us, so we don't run into races with the GC.
I expect we might still want to free it early via Dispose. If that is the case, we would need to have certainty that this handle/context is not part of a current bridge GC. This would be the case if the C# object that this handle points to is not dead. So if we get hold of this GCHandle from the IJavaPeerable, then it is ok. If we just traverse the RegisteredInstances and free some handles from there, then this sounds potentially problematic.
| if (p.Target is not IJavaPeerable peer) | ||
| continue; | ||
| if (!JniEnvironment.Types.IsSameObject (peer.PeerReference, value.PeerReference)) | ||
| continue; | ||
| if (Replaceable (p)) { | ||
| FreeReferenceTrackingHandle (p); |
There was a problem hiding this comment.
Does this mean here that if we have a cross GCHandle C#1 -> Java1. And we try to add a new bridge object C#2 -> Java1. Then we attempt to free the first GCHandle and create a new one instead ? I don't fully understand the reasoning behind this behavior. Also, as described in the comment for FreeReferenceTrackingHandle it seems like the first GCHandle could be part of the gcbridge machinery, if C#1 is dead in managed world and we would race with the GC here.
| if (RegisteredInstances == null) | ||
| throw new ObjectDisposedException (nameof (ManagedValueManager)); | ||
|
|
||
| WaitForGCBridgeProcessing (); |
There was a problem hiding this comment.
I'm not sure if this wait achieves something. In general, for key synchronization pieces with the GC, I think we should add explicit comments regarding what we are trying to achieve, what race we try to prevent. Later, when we are smarter, we could see whether we actually need it or not, or have another solution for these problems.
| foreach (int i in indexesToRemove) { | ||
| // Remove the peer from the list | ||
| var handle = peers[i]; | ||
| FreeReferenceTrackingHandle (handle); |
There was a problem hiding this comment.
I think here we are only freeing handles that have value as the Target, which, if it was not obtained from the gchandle weak ref, then we know it shouldn't be part of the current bridge. This would mean that this should be safe, in theory.
| @@ -181,6 +282,8 @@ public override void RemovePeer (IJavaPeerable value) | |||
|
|
|||
| public override void FinalizePeer (IJavaPeerable value) | |||
| { | |||
| WaitForGCBridgeProcessing (); | |||
|
There are a lot of waits for bridge processing which I don't think are needed. I think the only place we need to wait for bridge processing is when we obtain a C# object ref from the java object (JniObjectReference?), not exactly sure where this location is. This is because by doing this we could insert into C# world an object that we thought was dead during last GC, end up calling Dispose on it racing with the GC. |
|
|
||
| void GCBridge::wait_for_bridge_processing () noexcept | ||
| { | ||
| std::shared_lock<std::shared_mutex> lock (processing_mutex); |
There was a problem hiding this comment.
This doesn't seem correct. In theory we could obtain this mutex, before the bridge worker thread actually acquires it, doing the BP2 stage. We would need at least an additional variable to mark whether we have a bridge in progress. Probably makes sense to use a condition variable for this.
|
The runtime implementation contains implicit wait for bridge processing when obtaining the Target of a WeakReference. If we would use this mechanism when obtaining the C# object from a Java object, then we probably won't need our own implementation in |
Work in progress.