Skip to content

GC Bridge for Android scenarios #114184

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AaronRobinsonMSFT
Copy link
Member

This PR contains the VM side of the work and the introduction of a new GC Handle type.

The GC work still needs to be completed. This is currently just a place to share work.

  • API needs approval
  • Tests

Prior to check-in, update all places with "!TODO-JAVA!".
Implement scanning of the new GC Handle and calling
a new VM callout to process the resulting data.
Implement the VM side of the GC Handle processing
and callout to mark cross references.
@ghost
Copy link

ghost commented Apr 2, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@ghost
Copy link

ghost commented Apr 2, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@AaronRobinsonMSFT AaronRobinsonMSFT added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 2, 2025
@@ -161,6 +161,11 @@ if(FEATURE_OBJCMARSHAL)
add_compile_definitions(FEATURE_OBJCMARSHAL)
endif()

if(FEATURE_JAVAMARSHAL)
add_compile_definitions(FEATURE_JAVAMARSHAL)
add_compile_definitions(FEATURE_GCBRIDGE)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need two different FEATURE_... ifdefs for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point there aren't any. While I was implementing it the logic was clearly distinct though. There is the general concept of the GC Bridge and also the specific Java API and infrastructure that exposes that data. It was natural to separate the two given how we've implemented other interop scenarios involving the GC. It is entirely reasonable to collapse them, but making them distinct now is much easier than retroactively splitting them in the future.

@@ -14,7 +14,7 @@
#endif

#define INITIAL_HANDLE_TABLE_ARRAY_SIZE 10
#define HANDLE_MAX_INTERNAL_TYPES 12
#define HANDLE_MAX_INTERNAL_TYPES 16
Copy link
Member

Choose a reason for hiding this comment

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

Are there perf consequences from bumping this limit? Should we start recycling deprecated HDNTYPE values (e.g. HNDTYPE_ASYNCPINNED)?

Copy link
Member

Choose a reason for hiding this comment

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

just need to be careful that recycling wouldnt cause backcompat issues for standalone gc.

GCScan::GcScanWithBridge(GCHeap::Promote,
condemned_gen_number, max_generation,
&sc);
drain_mark_queue();
Copy link
Member

Choose a reason for hiding this comment

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

wonder whether drain_mark_queue is required after each scan or can it be combined with the one above ?

condemned_gen_number, max_generation,
&sc);
drain_mark_queue();
// fire_mark_event (ETW::???, current_promoted_bytes, last_promoted_bytes);
Copy link
Member

Choose a reason for hiding this comment

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

assume this will be a TODO to enable later?

}
CONTRACTL_END;

#ifdef FEATURE_GCBRIDGE
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt the full function be under a #ifdef ?

@Maoni0
Copy link
Member

Maoni0 commented Apr 3, 2025

I don't think changes on the GC side need to be reviewed at this point. I know they will be different but we will address those when we actually start implementation.

@dotnet-policy-service dotnet-policy-service bot removed this from the 10.0.0 milestone May 3, 2025
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

jonathanpeppers added a commit to dotnet/android that referenced this pull request May 8, 2025
Context: dotnet/runtime#114184
Context: https://github.com/jonathanpeppers/BridgeSandbox

So far, I:

* Built dotnet/runtime, I built this branch:

https://github.com/jonathanpeppers/runtime/tree/gcbridge_impl

* Put the relevant arm64 packs in `packages/` folder and configured a
  `NuGet.config` to use them.

* Setup a `UpdateRuntimePacks` target in the `Mono.Android.csproj` to
  use the 10.0.0-dev packs.

* Make use of the new `JavaMarshal.CreateReferenceTrackingHandle()` API

* This is WIP there are still more APIs.
jonathanpeppers added a commit to dotnet/android that referenced this pull request May 8, 2025
Context: dotnet/runtime#114184
Context: https://github.com/jonathanpeppers/BridgeSandbox
Context: dotnet/runtime@main...BrzVlad:runtime:feature-clr-gcbridge

So far, I:

* Built dotnet/runtime, I built this branch:

https://github.com/jonathanpeppers/runtime/tree/gcbridge_impl

* Put the relevant arm64 packs in `packages/` folder and configured a
  `NuGet.config` to use them.

* Setup `build-tools\scripts\custom-runtime.targets` to use the
  10.0.0-dev dotnet/runtime packs.

* In `ManagedValueManager.cs`...

* Call a new native method on startup:

    var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingFinished);

* Pass the returned function pointer to:

    JavaMarshal.Initialize (mark_cross_references_ftn);

* In `AddPeer(IJavaPeerable value)` call `JavaMarshal.CreateReferenceTrackingHandle()`

* In `RemovePeer(IJavaPeerable value)` call:

    static unsafe void FreeHandle (GCHandle handle)
    {
        IntPtr context = JavaMarshal.GetContext (handle);
        NativeMemory.Free((void*)context);
    }
jonathanpeppers added a commit to dotnet/android that referenced this pull request May 8, 2025
Context: dotnet/runtime#114184
Context: https://github.com/jonathanpeppers/BridgeSandbox
Context: dotnet/runtime@main...BrzVlad:runtime:feature-clr-gcbridge

So far, I:

* Built dotnet/runtime, I built this branch:

https://github.com/jonathanpeppers/runtime/tree/gcbridge_impl

* Put the relevant arm64 packs in `packages/` folder and configured a
  `NuGet.config` to use them.

* Setup `build-tools\scripts\custom-runtime.targets` to use the
  10.0.0-dev dotnet/runtime packs.

* In `ManagedValueManager.cs`...

* Call a new native method on startup:

    var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingFinished);

* Pass the returned function pointer to:

    JavaMarshal.Initialize (mark_cross_references_ftn);

* In `AddPeer(IJavaPeerable value)` call `JavaMarshal.CreateReferenceTrackingHandle()`

* In `RemovePeer(IJavaPeerable value)` call:

    static unsafe void FreeHandle (GCHandle handle)
    {
        IntPtr context = JavaMarshal.GetContext (handle);
        NativeMemory.Free((void*)context);
    }
jonathanpeppers added a commit to dotnet/android that referenced this pull request May 8, 2025
Context: dotnet/runtime#114184
Context: https://github.com/jonathanpeppers/BridgeSandbox
Context: dotnet/runtime@main...BrzVlad:runtime:feature-clr-gcbridge

So far, I:

* Built dotnet/runtime, I built this branch:

https://github.com/jonathanpeppers/runtime/tree/gcbridge_impl

* Put the relevant arm64 packs in `packages/` folder and configured a
  `NuGet.config` to use them.

* Setup `build-tools\scripts\custom-runtime.targets` to use the
  10.0.0-dev dotnet/runtime packs.

* In `ManagedValueManager.cs`...

* Call a new native method on startup:

    var mark_cross_references_ftn = RuntimeNativeMethods.clr_initialize_gc_bridge (&BridgeProcessingFinished);

* Pass the returned function pointer to:

    JavaMarshal.Initialize (mark_cross_references_ftn);

* In `AddPeer(IJavaPeerable value)` call `JavaMarshal.CreateReferenceTrackingHandle()`

* In `RemovePeer(IJavaPeerable value)` call:

    static unsafe void FreeHandle (GCHandle handle)
    {
        IntPtr context = JavaMarshal.GetContext (handle);
        NativeMemory.Free((void*)context);
    }
jonpryor added a commit to dotnet/java-interop that referenced this pull request May 10, 2025
Context: dotnet/runtime#114184
Context: dotnet/android#10125
Context: dotnet/android#10125 (comment)

Part of adding a GC bridge to CoreCLR are the new APIs:

	namespace System.Runtime.InteropServices.Java;
	public struct ComponentCrossReference {
	    public nint SourceGroupIndex, DestinationGroupIndex;
	}
	public unsafe struct StronglyConnectedComponent {
	    public nint Count;
	    public IntPtr* Context;
	}
	public static partial class JavaMarshal {
	    public static unsafe void Initialize(
	        delegate* unmanaged<
	            System.IntPtr,                  // sccsLen
	            StronglyConnectedComponent*,    // sccs
	            System.IntPtr,                  // ccrsLen
	            ComponentCrossReference*,       // ccrs
	            void> markCrossReferences);
	    public static GCHandle CreateReferenceTrackingHandle(object obj, IntPtr context);
	    public static IntPtr GetContext(GCHandle obj);
	}

Of note is the "data flow" of `context`:

  * `JavaMarshal.CreateReferenceTrackingHandle()` has a "`context`"
    parameter.

  * The `context` parameter to
    `JavaMarshal.CreateReferenceTrackingHandle()` is the return value
    of `JavaMarshal.GetContext()`

  * The `context` parameter to
    `JavaMarshal.CreateReferenceTrackingHandle()` is stored within
    `StronglyConnectedComponent.Context`.

  * The `markCrossReferences` parameter of `JavaMarshal.Initialize()`
    is called by the GC bridge and given a native array of
    `StronglyConnectedComponent` instances, which contains `Context`.

The short of it is that the proposed GC bridge doesn't contain direct
access to the `IJavaPeerable` instances in play.  Instead, it has
access to "context" which must contain the JNI Object Reference
information that the `markCrossReferences` callback needs access to.

Furthermore, the `context` pointer value *cannot change*, i.e. it
needs to be a native pointer value a'la **malloc**(3), ***not*** a
value which can be moved by the GC.  (The *contents* can change; the
pointer value cannot.))

While we're still prototyping this, what we currently believe we need
is the JNI object reference, JNI object reference type, and (maybe?)
the JNI Weak Global Reference value and "refs added" values.

Update `IJavaPeerable` to add a `JniObjectReferenceControlBlock`
property which can be used as the `context` parameter:

	partial interface IJavaPeerable {
	    IntPtr JniObjectReferenceControlBlock => 0;
	}

This supports usage of:

	IJavaPeerable   value   = …
	GCHandle        handle  = JavaMarshal.CreateReferenceTrackingHandle(
	    value,
	    value.JniObjectReferenceControlBlock
	);
@jonpryor
Copy link
Contributor

API design and implementation questions:

Why constrain to Android? It would be very very very useful (to me) if we could use this on Desktop environments (dotnet/java-interop unit tests!).

Why Java in the various public API names? I don't see any use of JNI in the current implementation, and it looks like all JNI use could be constrained to the markCrossReferences delegate callback. This doesn't appear to be tied to Java in any way.

My alternate proposal is to instead put some of these members into a System.Runtime.InteropServices.GCBridge namespace:

namespace System.Runtime.InteropServices.GCBridge;

public struct ComponentCrossReference {
    public nint SourceGroupIndex, DestinationGroupIndex;
}
public unsafe struct StronglyConnectedComponent {
    public nint Count;
    public IntPtr* Context;
}

If we can relax the "only Android" requirement, it then makes sense (to me) to add some of these members to GCHandle, e.g.:

namespace System.Runtime.InteropServices;

partial struct GCHandle {
    public static GCHandle AllocReferenceTracking(object? value, nint context);
    public nint Context {get;}
}

though it would also make sense to "hide" this corner case by keeping these members out of GCHandle, as originally done with JavaMarshal.

That leaves the JavaMarshal.Initialize() method. The markCrossReferences parameter isn't versionable; should it be, by using a versionable struct parameter? Additionally, what are the semantics of JavaMarshal.Initialize()? What can (should) happen if it's called more than once? Or can it be called only once? (Currently, a call replaces any previous callback; I'm uncertain how good of an idea this is.)

I would thus propose:

namespace System.Runtime.InteropServices.GCBridge;

public struct MarkCrossReferences {
    public System.IntPtr               componentsLen;
    public StronglyConnectedComponent* components;
    public System.IntPtr               crossReferencesLen;
    public ComponentCrossReference*    crossReferences;
}

partial static class GCBridgeMarshal {
    // May only be invoked once; throws on 2nd+ invocation
    public static unsafe void InitializeOnce(
            delegate* unmanaged<MarkCrossReferences*, void> markCrossReferences
    );

    // If we don't want to add members to GCHandle…
    public static GCHandle CreateReferenceTrackingHandle(object obj, IntPtr context);
    public static IntPtr GetContext(GCHandle obj);
}

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented May 10, 2025

Why constrain to Android? It would be very very very useful (to me) if we could use this on Desktop environments (dotnet/java-interop unit tests!).

Agree in principle. This was simply narrowing where we support java interop. It isn't planned to be something we would recommend for users, but I do agree that having it testable on desktop is a big win and something we should be open to.

Why Java in the various public API names? I don't see any use of JNI in the current implementation, and it looks like all JNI use could be constrained to the markCrossReferences delegate callback. This doesn't appear to be tied to Java in any way.

Following the ObjectiveC and Swift interop stories. The GCBridge suggestion sounds reasonable to me.

though it would also make sense to "hide" this corner case by keeping these members out of GCHandle, as originally done with JavaMarshal.

Yep, these APIs should be narrowly used.

That leaves the JavaMarshal.Initialize() method. The markCrossReferences parameter isn't versionable; should it be, by using a versionable struct parameter?

I don't see the need for a version API here. Happy to discuss further, but this in't an area where I think we will be doing much innovation and if we do it is likely to impact more than just the struct.

Additionally, what are the semantics of JavaMarshal.Initialize()? What can (should) happen if it's called more than once? Or can it be called only once? (Currently, a call replaces any previous callback; I'm uncertain how good of an idea this is.)

It can only be called once. It will throw if it is called again. This was also modeled after the ObjectiveC APIs.

I would thus propose:

I like the updated proposal. I don't think InitializeOnce is needed though. Initialize is often used and the documentation is clear on that behavior.

@jkotas
Copy link
Member

jkotas commented May 11, 2025

My two cents: I like the Java in the name. This is a 3rd variant of what can be called "GC bridge" that we are introducing APIs for. Two existing GC bridges are COM/WinRT and ObjectiveC. The design objective for all of them has been backward compatibility for MAUI workloads. I do not see any of them as a general-purpose solution.

I don't see any use of JNI in the current implementation, and it looks like all JNI use could be constrained to the markCrossReferences delegate callback.

For example, the runtime side of the ObjectiveC APIs does not have any ObjectiveC calls either and it can be used by any system that does lifetime management like ObjectiveC in theory.

@MichalPetryka
Copy link
Contributor

Agree in principle. This was simply narrowing where we support java interop. It isn't planned to be something we would recommend for users, but I do agree that having it testable on desktop is a big win and something we should be open to.

Technically .NET 5 promised it for all platforms 😄
image

@jkotas
Copy link
Member

jkotas commented May 11, 2025

Technically .NET 5 promised it for all platforms

This goal was revised shortly after that blog post was published. (We are not blogging about plans anymore since the plans change and blogs like that just created confusion.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime.InteropServices new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants