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

[meta] Mono ALC embedding primer #34601

Merged
merged 7 commits into from
Apr 19, 2020

Conversation

CoffeeFlux
Copy link
Contributor

After talking with @lambdageek , we decided we wanted the public API for ALCs to just use gchandles instead of a native struct and to handle conversion within the runtime.

This PR takes that approach, exposing the managed ALC fields to the runtime for cheap conversion and adding mono_assembly_load_full_alc and preload hook V3 to the unstable headers.

I considered trying to do something fancier with conversion for the preload hook but decided against it in the end; making a V3 that's intended for embedder usage is straightforward and should work fine. When invoking it, we create a new strong gchandle to keep the managed ALC pinned and then free it after the call is done.

I went with MonoManagedAssemblyLoadContext instead of MonoReflectionAssemblyLoadContext since ALCs aren't really part of reflection, but I have no strong attachment to the name and can change it if preferred to maintain consistency.

cc: @vargaz @mdh1418 @garuma

// Keep in sync with System.Runtime.Loader.AssemblyLoadContext
typedef struct {
MonoObject object;
MonoObject *unload_lock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would reorder this and put the runtime visible fields at the top, so the other fields don't need to be duplicated here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess thats not possible because its a partial class. In that case, this is going to be pretty brittle since fields can be added to the non-mono specific part.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be avoided by not using a GC handle, and instead passing an opaque handle (the alc pointer). In that case, probably another API is needed to obtain the managed alc object from the handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why I just added the note to keep them in sync. The alternative is I guess creating a Mono-specific field that just gets updated every time the main field is, but that seems like it would introduce so many other potential issues that it's not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with using an opaque handle is then you have to worry about unloading. You'd either have to be exposing public refcounting mechanisms or give embedders a handle with a contract to return it... The gchandle seems less troublesome.

Copy link
Contributor Author

@CoffeeFlux CoffeeFlux Apr 13, 2020

Choose a reason for hiding this comment

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

Maybe the ref count is a reasonable option, since that's happening anyway internally due to unloading and we should just make it public? I feel like the gchandle is much easier for people to deal with though, and realistically we're going to need a mono_alc_from_gchandle regardless.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also return a gchandle for some internal object we create ourselves and whose layout we control, which references the real alc object. Looks a bit weird, but it should work.
Or, we could keep a alc object->alc mapping in a hash table, but it would keep the alc object alive unless it uses a weak handle or something.

@vargaz
Copy link
Contributor

vargaz commented Apr 13, 2020

Looks ok otherwise.

@CoffeeFlux
Copy link
Contributor Author

Going to merge this for now once CI passes, and we can iterate in the future if necessary.

@CoffeeFlux CoffeeFlux merged commit 4bc2786 into dotnet:master Apr 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants