Skip to content
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

[NativeAOT] Objective-C Marshal: object tracker support #78280

Merged
merged 15 commits into from Dec 12, 2022

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Nov 12, 2022

With these changes the src/tests/Interop/ObjectiveC/ObjectiveCMarshalAPI/ test passes everything except _Validate_ExceptionPropagation.

The native part ( interoplibinterface.h and interoplibinterface_objc.cpp ) are based off the corresponding files from CoreCLR. Potentially parts could be shared between both runtimes, but it's not a lot of code.

Compared CoreCLR version, every Objective-C object takes 3x the GC handles. In addition the ref-counted GC handle, there is a dependent handle in the conditional weak table and a long weak handle to extend the lifetime ObjcTrackingInformation. If that is not acceptable, the tracking information could be moved into the SyncBlock like CoreCLR does. With the current implementation of NativeAOT sync blocks, that would reduce handle overhead to 2x that of CoreCLR, at the cost of making every macOS sync block bigger.

Related issue: #77472

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 12, 2022
@AustinWise AustinWise marked this pull request as ready for review November 12, 2022 23:37
@jkotas
Copy link
Member

jkotas commented Nov 13, 2022

cc @AaronRobinsonMSFT

@filipnavara
Copy link
Member

If that is not acceptable, the tracking information could be moved into the SyncBlock like CoreCLR does.

It would likely be possible to limit the overhead only to types that are marked with the ObjectiveCTrackedTypeAttribute at expense of making the SyncTable entry allocation/references more complex. I played with the idea but it very quickly became unwieldy.

Copy link
Contributor Author

@AustinWise AustinWise left a comment

Choose a reason for hiding this comment

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

I added some comments on things I'm not sure about.

@AustinWise
Copy link
Contributor Author

I updated the PR to change the way callbacks work. There are a couple of outstanding items (listed in the PR description) that I need to address before this can be merged.

@AustinWise
Copy link
Contributor Author

I addressed known issues with this PR (making it compatible with multiple class libs). This PR is now ready for further review.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

The compiler side looks good to me, the CoreLib interaction too, but I can't speak to the GC interaction details or objc interop in general. Cc @VSadov as well

@ghost
Copy link

ghost commented Nov 21, 2022

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

With these changes the src/tests/Interop/ObjectiveC/ObjectiveCMarshalAPI/ test passes everything except _Validate_ExceptionPropagation.

The native part ( interoplibinterface.h and interoplibinterface_objc.cpp ) are based off the corresponding files from CoreCLR. Potentially parts could be shared between both runtimes, but it's not a lot of code.

Compared CoreCLR version, every Objective-C object takes 3x the GC handles. In addition the ref-counted GC handle, there is a dependent handle in the conditional weak table and a long weak handle to extend the lifetime ObjcTrackingInformation. If that is not acceptable, the tracking information could be moved into the SyncBlock like CoreCLR does. With the current implementation of NativeAOT sync blocks, that would reduce handle overhead to 2x that of CoreCLR, at the cost of making every macOS sync block bigger.

Related issue: #77472

Author: AustinWise
Assignees: -
Labels:

area-Interop-coreclr, community-contribution, area-NativeAOT-coreclr

Milestone: -

@AaronRobinsonMSFT
Copy link
Member

FYI @rolfbjarne

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Does the NativeAOT GC run collections on multiple threads (like the server GC), or only on a single thread (like the workstation GC)?

@AustinWise
Copy link
Contributor Author

Does the NativeAOT GC run collections on multiple threads (like the server GC), or only on a single thread (like the workstation GC)?

According to this comment from the Xamarin repository, the Server GC is not supported:

https://github.com/xamarin/xamarin-macios/blob/main/runtime/coreclr-bridge.m#L203-L210

@jkotas
Copy link
Member

jkotas commented Nov 21, 2022

Does the NativeAOT GC run collections on multiple threads (like the server GC), or only on a single thread (like the workstation GC)?

Native AOT respects the same build configuration settings as standard CoreCLR. It uses workstation GC by default and server GC can be enabled by setting <ServerGarbageCollection>true</ServerGarbageCollection> in the project file.

According to this comment from the Xamarin repository, the Server GC is not supported

What happens if somebody enables server GC in a project that uses Xamarin interop? Does it just crash silently, or does it produce descriptive error somewhere?

@rolfbjarne
Copy link
Member

Does the NativeAOT GC run collections on multiple threads (like the server GC), or only on a single thread (like the workstation GC)?

According to this comment from the Xamarin repository, the Server GC is not supported:

xamarin/xamarin-macios@main/runtime/coreclr-bridge.m#L203-L210

That's exactly why I'm asking 😄

Does the NativeAOT GC run collections on multiple threads (like the server GC), or only on a single thread (like the workstation GC)?

Native AOT respects the same build configuration settings as standard CoreCLR. It uses workstation GC by default and server GC can be enabled by setting <ServerGarbageCollection>true</ServerGarbageCollection> in the project file.

According to this comment from the Xamarin repository, the Server GC is not supported

What happens if somebody enables server GC in a project that uses Xamarin interop? Does it just crash silently, or does it produce descriptive error somewhere?

I intended to make it an error, but it seems I never did that. I've filed an issue so I don't forget again: xamarin/xamarin-macios#16853

AustinWise and others added 5 commits December 3, 2022 14:22
This reverts commit 2abfe8d394b899810e955d79af0b9303f82c448d.
Specifically:

* Remove support for registering multiple begin/end callbacks, since the API was not designed with multiple managed worlds in mind.
* Use more asserts where it should not be possible for there to be no callback.
* Add more comments.
* Add missing GC-trigger disable when calling managed callouts.
* Add comments to CoreCLR and NativeAOT about constants that should have the same size.
* Remove superfluous use of C++ templates.
* Use the C# unmanaged function pointer types in more places, improving type safety
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

From the interop side of things, I am fine with this. I'd like to make sure @MichalStrehovsky is okay from the NativeAOT side and @rolfbjarne is aware and okay with support from the Xamarin side.

Thanks!

AustinWise and others added 2 commits December 4, 2022 16:25
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@AustinWise
Copy link
Contributor Author

AustinWise commented Dec 5, 2022

@AaronRobinsonMSFT Regarding your suggestions to change the type parameters on the unmanned function pointers from IntPtr to void*, these don't match the public the definitions on the public API ObjectiveCMarshal.Initialize. So compilation failed. When I tried updating the public API to use void*, I got the following error:

error CP0002: Member 'System.Runtime.InteropServices.ObjectiveC.ObjectiveCMarshal.Initialize(delegate* unmanaged, delegate* unmanaged<nint, int>, delegate* unmanaged<nint, void>, System.Runtime.InteropServices.ObjectiveC.ObjectiveCMarshal.UnhandledExceptionPropagationHandler)' exists on ref/net7.0/System.Private.CoreLib.dll but not on lib/net7.0/System.Private.CoreLib.dll

So I'm going to stick to using IntPtr for consistency with the public API.

@AaronRobinsonMSFT
Copy link
Member

So I'm going to stick to using IntPtr for consistency with the public API.

Ah. Okay. I missed that. We try to use void* now when possible since IntPtr degrades to nint and is sort of misleading in the majority of cases. If this isn't possible then please ignore.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Compiler side still looks good. I can't judge GC interactions or objc semantics - not reviewing that.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit bf9fc46 into dotnet:main Dec 12, 2022
@AustinWise AustinWise deleted the austin/ObjC-ReferenceTracker branch December 31, 2022 04:19
@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants