-
Notifications
You must be signed in to change notification settings - Fork 551
[CoreCLR] Implement GC bridge #10198
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.
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
base: main
Are you sure you want to change the base?
Conversation
src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs
Show resolved
Hide resolved
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]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be sure .Contexts [j]
will always be HandleContext*
? I'd feel "safer" if it was an as
statement followed by a null
check. I think we should try avoiding exceptions in this code, even if they're very unlikely to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that somebody could use the JavaMarshal.CreateReferenceTrackingHandle
in their code and pass a pointer to a different structure. In this case, I am not sure what we can do about it. We cannot use as
or is
with pointer types, also that memory itself is native and not managed.
I wonder if we need to have a Dictionary<IntPtr, GCHandle>
where we would track all the native memory poitners we create and which handles they correspond to. This would replace the weird reference to the GCHandle in WeakPeerReference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do what we can. I had to look up MarkCrossReferencesArgs
struct to see what it is and indeed we can't verify that the context is actually a HandleContext*
in the current shape of the code. The dictionary idea might be a very good one.
I think what we absolutely must do is to verify that mcr->Components
isn't null
and that mcr->Components[x].Contexts
isn't null before using them here.
We can't do anything to check whether the array subscript is valid, so we unfortunately need to trust the caller that they got the data right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will explore the idea with the dictionary and see if it is feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I change the implementation of the value manager to keep a dictionary of the context -> handle mappings and only accept contexts which were produced by us.
I don't think there is a good way to implement this on the native side without too much overhead. We currently assume that all the contexts passed to us by the GC are our contexts of the right "type" with the right size. That might not be necessarily the case, although I think it's extremely unlikely that it would become a problem.
class GCBridge | ||
{ | ||
public: | ||
static void wait_for_bridge_processing () noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a simple enough method, might want to make it inline here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that it could be inlined here but I prefer having all the places which touch the mutex+semaphore to be next to each other in the .cc
file. If you're ok with it, I would keep this part of the code as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new GC bridge to support improved garbage collection coordination between the managed and Java runtimes, replacing #10185 and aligning with the related runtime changes. Key changes include the implementation of GC bridge interfaces and processing logic in native code, updates to pinvoke tables and build configuration, and modifications in the ManagedValueManager and various JNI bridging files to use the new GC bridge callbacks.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/clr/include/host/gc-bridge.hh | Adds new structures and functions to support native GC bridge callbacks. |
src/native/clr/include/host/bridge-processing.hh | Introduces types and functions for processing cross-references. |
src/native/clr/host/pinvoke-tables.include | Updates the internal pinvoke table, increasing the expected count. |
src/native/clr/host/gc-bridge.cc | Implements the GC bridge’s processing thread and callback logic. |
src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs | Transitions peer management to use the new GC bridge initialization. |
(Other JNI-related files) | Replaces outdated references to JNIEnvInit.ValueManager with JniEnvironment.Runtime.ValueManager for consistency. |
Comments suppressed due to low confidence (1)
src/Mono.Android/Android.Runtime/JNIEnvInit.cs:40
- Since all references now use JniEnvironment.Runtime.ValueManager, consider removing this unused field to avoid confusion and improve code maintainability.
}
Replaces #10185
Implements dotnet/runtime#115506
Builds on top of dotnet/runtime#116310 - this PR is expected to fail to build until this runtime PR is merged and flows into
main
Description
TBD
/cc @BrzVlad @jonathanpeppers @grendello