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

[API Proposal]: GCHandle<T> (like GCHandle, but this time it's great™️) #94134

Open
Sergio0694 opened this issue Oct 28, 2023 · 50 comments
Open
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Runtime.InteropServices
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Oct 28, 2023

This API proposal is extracted from #94113 (comment) and supersedes/fixes:

Overview

The GCHandle type currently has a number of issues/inefficiencies, that are impossible to completely fix without introducing some (major) breaking changes, which could cause all sorts of problems for people having a dependency on the current behaviors. It would be beneficial to instead add a new handle type, which solves all of these problems from the start:

  • Unnecessary validation check (null check) when getting/setting targets
  • Pin flag removal every time the handle target is resolved
  • Interlocked operation from Free
  • Not implementing IDisposable

We have a reference implementation which can be a starting point for this.

cc. @jkotas @AaronRobinsonMSFT @tannergooding

API Proposal

namespace System.Runtime.InteropServices;

public struct GCHandle<T> : System.IDisposable
    where T : class
{
    public GCHandle(T? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public T? Target { readonly get; set; }

    public static GCHandle<T> FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(GCHandle<T> value);

    public void Dispose();
}

public static class GCHandleExtensions
{
    public static T* UnsafeAddrOfPinnedArrayData<T>(this GCHandle<T[]> handle);
    public static char* UnsafeAddrOfPinnedStringData(this GCHandle<string> handle);
}

Open questions

  • I'm assuming that the Target getter can be updated to just do *(object*)_handle, hence being as fast as possible already (ie. not needing that internal call that GCHandle has in its current implementation). If that's the case, then there's no need to add a separate GetTargetUnsafe() method — Target would already be as fast as it can be from the start.
  • Should we consider using "better" names, since this is a new API? For instance:
    • The Alloc abbreviation is against the naming convention. Should it be Allocate? Or maybe Create?
    • Same for GetAddrOfPinnedObject(), should it be GetAddressOfPinnedObject()? Or maybe just PinnedObjectAddress?
  • Since we said the type should implement IDisposable, I've just added Dispose(). Do we also want a Free() method?
  • Shold we include FromIntPtr(IntPtr) and ToIntPtr() as well?

API Usage

Same as GCHandle, really:

// In a constructor (or wherever)
this.handle = new GCHandle<MyObject>(myObj);

// Later on
MyObject? target = this.handle.Target;

Alternative Designs

The alternative would be to change GCHandle, but as mentioned, it seems impossible to fix all issues with back-compat.

Risks

Not really any risk, since this would be a brand new type.

@Sergio0694 Sergio0694 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 28, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 28, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 28, 2023
@SingleAccretion SingleAccretion added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 28, 2023
@ghost
Copy link

ghost commented Oct 28, 2023

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

Issue Details

This API proposal is extracted from #94113 (comment) and supersedes/fixes:

Overview

The GCHandle type currently has a number of issues/inefficiencies, that are impossible to completely fix without introducing some (major) breaking changes, which could cause all sorts of problems for people having a dependency on the current behaviors. It would be beneficial to instead add a new handle type, which solves all of these problems from the start:

  • Unnecessary validation check (null check) when getting/setting targets
  • Pin flag removal every time the handle target is resolved
  • Interlocked operation from Free
  • Not implementing IDisposable

We have a reference implementation which can be a starting point for this.

cc. @jkotas @AaronRobinsonMSFT @tannergooding

API Proposal

namespace System.Runtime.InteropServices;

[StructLayout(LayoutKind.Sequential)]
public struct UnsafeGCHandle : System.IDisposable
{
    public static UnsafeGCHandle Alloc(object? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public object? Target { readonly get; set; }

    public readonly unsafe IntPtr GetAddrOfPinnedObject();
    public void Dispose();
}

Open questions

  • I'm assuming that the Target getter can be updated to just do *(object*)_handle, hence being as fast as possible already (ie. not needing that internal call that GCHandle has in its current implementation). If that's the case, then there's no need to add a separate GetTargetUnsafe() method — Target would already be as fast as it can be from the start.
  • Should we consider using "better" names, since this is a new API? For instance:
    • The Alloc abbreviation is against the naming convention. Should it be Allocate? Or maybe Create?
    • Same for GetAddrOfPinnedObject(), should it be GetAddressOfPinnedObject()? Or maybe just PinnedObjectAddress?
  • Since we said the type should implement IDisposable, I've just added Dispose(). Do we also want a Free() method?

API Usage

Same as GCHandle, really:

// In a constructor (or wherever)
this.handle = UnsafeGCHandle.Alloc(myObj);

// Later on
object? target = this.handle.Target();

Alternative Designs

The alternative would be to change GCHandle, but as mentioned, it seems impossible to fix all issues with back-compat.

Risks

Not really any risk, since this would be a brand new type.

Author: Sergio0694
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@ghost
Copy link

ghost commented Oct 28, 2023

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

Issue Details

This API proposal is extracted from #94113 (comment) and supersedes/fixes:

Overview

The GCHandle type currently has a number of issues/inefficiencies, that are impossible to completely fix without introducing some (major) breaking changes, which could cause all sorts of problems for people having a dependency on the current behaviors. It would be beneficial to instead add a new handle type, which solves all of these problems from the start:

  • Unnecessary validation check (null check) when getting/setting targets
  • Pin flag removal every time the handle target is resolved
  • Interlocked operation from Free
  • Not implementing IDisposable

We have a reference implementation which can be a starting point for this.

cc. @jkotas @AaronRobinsonMSFT @tannergooding

API Proposal

namespace System.Runtime.InteropServices;

[StructLayout(LayoutKind.Sequential)]
public struct UnsafeGCHandle : System.IDisposable
{
    public static UnsafeGCHandle Alloc(object? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public object? Target { readonly get; set; }

    public readonly unsafe IntPtr GetAddrOfPinnedObject();
    public void Dispose();
}

Open questions

  • I'm assuming that the Target getter can be updated to just do *(object*)_handle, hence being as fast as possible already (ie. not needing that internal call that GCHandle has in its current implementation). If that's the case, then there's no need to add a separate GetTargetUnsafe() method — Target would already be as fast as it can be from the start.
  • Should we consider using "better" names, since this is a new API? For instance:
    • The Alloc abbreviation is against the naming convention. Should it be Allocate? Or maybe Create?
    • Same for GetAddrOfPinnedObject(), should it be GetAddressOfPinnedObject()? Or maybe just PinnedObjectAddress?
  • Since we said the type should implement IDisposable, I've just added Dispose(). Do we also want a Free() method?

API Usage

Same as GCHandle, really:

// In a constructor (or wherever)
this.handle = UnsafeGCHandle.Alloc(myObj);

// Later on
object? target = this.handle.Target();

Alternative Designs

The alternative would be to change GCHandle, but as mentioned, it seems impossible to fix all issues with back-compat.

Risks

Not really any risk, since this would be a brand new type.

Author: Sergio0694
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices, untriaged

Milestone: -

@jkotas
Copy link
Member

jkotas commented Oct 28, 2023

  • [StructLayout(LayoutKind.Sequential)]

Unnecessary.

  • The Alloc abbreviation is against the naming convention. Should it be Allocate? Or maybe Create?

It should be a constructor (same as what we have done for DependentHandle).

  • GetAddrOfPinnedObject

This method needs to do several checks against the type of the object. It may be better to have GetAddrOfPinnedArray and GetAddrOfPinnedString to go with the spirit of maximally unsafe type.

  • Keep FromIntPtr/ToIntPtr?

@Sergio0694
Copy link
Contributor Author

Updated the proposal integrating your suggestions 🙂

For GetAddrOfPinnedArray, I take it we want to keep that abbreviation for historic reasons?
If not, should we maybe consider naming those methods eg. GetAddressOfPinnedArray or something?

Any thoughts on whether we can just make Target free (*(object*)_handle in release), or if we need GetTargetUnsafe()?
From our previous conversation it seems to me we should be able to just make that faster? But just double checking 😄

@jkotas
Copy link
Member

jkotas commented Oct 28, 2023

should we maybe consider naming those methods eg. GetAddressOfPinnedArray or something?

I assume that this API would be a shorthand for Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference<T>(Unsafe.As<T[]>(handle.Target))), so maybe GetArrayDataAsPointer? Also, make it return a pointer?

Any thoughts on whether we can just make Target free ((object)_handle in release),

Yes. I think the idea is that this is maximally unsafe type.

@tannergooding
Copy link
Member

Also, make it return a pointer?

Should From/ToIntPtr then be FromPointer and take void* instead?

Yes. I think the idea is that this is maximally unsafe type.

Given we have and allow object* now, and this is "maximally unsafe". Do we even need a type? Could this just be a pair of APIs on System.Runtime.CompilerServices.Unsafe? Something like:

public static object* CreateGCHandle(object? value, GCHandleType type = GCHandleType.Normal);
public static void DisposeGCHandle(object* value);

Given these two APIs, a user could trivially create their own unsafe wrapper with whatever name they wanted. They could still convert to and store it as IntPtr or a GCHandle or ...

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Oct 28, 2023

I was first a bit skeptical of going as far as exposing object* APIs, but (from Discord) Tanner did make a good point that just convinced me: if you're authoring some native object (eg. say a COM one) and eg. wanted some handle for a CoreWindow object (just to name any type you might need), would it be "better" to have an UnsafeGCHandle field, or literally just a CoreWindow* field? And the latter does kinda seem more explicit/intuitive in a way. And we'd just need two new APIs 😄

I'm not sure they belong on Unsafe though. Perhaps:

namespace System.Runtime.InteropServices;

public struct GCHandle
{
    public static object* CreateUnsafe(object? value, GCHandleType type = GCHandleType.Normal);
    public static void DisposeUnsafe(object* value);
}

?

Eg. in ComputeSharp I have this field. Would certainly be much more readable if it could just be Globals*.
And it also wouldn't need any additional Unsafe.As<Globals>(handle.Target!) clunkyness.

Additional benefit: after you initially cast the pointer type, you can also preserve nullability annotations.
Eg. you could have a SomeObject* field, or a SomeObject?* field to inform the rest of your code.

@tannergooding
Copy link
Member

Noting that using object* does limit the implementations the GC could give for the type. It might not be valid on all runtimes. But, it was a passing idea in my head, so I thought I'd share.

Alternative names than UnsafeGCHandle would also include ObjectHandle or some variant thereof. These could likewise just be *Unsafe variants on GCHandle itself. CreateUnsafe, GetTargetUnsafe, etc. There are pros and cons to all of these, of course.

@jkotas
Copy link
Member

jkotas commented Oct 28, 2023

Should From/ToIntPtr then be FromPointer and take void* instead?

From/ToIntPtr returns opaque handle.

Noting that using object* does limit the implementations the GC could give for the type. It might not be valid on all runtimes.

object* happens to work for reading with the current CoreCLR GC, but it does not work for writing. GC handles need a different write barrier than regular object references even today. I do not think we would want to lock ourselves into potential read barriers to match between object references and GC handles.

@alexrp
Copy link
Contributor

alexrp commented Oct 28, 2023

Has there been any consideration to making the handle struct generic, to make it more type-safe and ergonomic?

@Sergio0694
Copy link
Contributor Author

Well, alright then we can ignore the whole object* idea, perfect 😄

"assume that this API would be a shorthand for Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference<T>(Unsafe.As<T[]>(handle.Target))), so maybe GetArrayDataAsPointer? Also, make it return a pointer?"

I've updated the API shape integrating all the new suggestions:

  • Named the methods GetArrayDataPointer/GetStringDataPointer (for consistency with GetArrayDataReference)
  • Changed the return types to void*

Is there anything else we might be missing? The API shape looks pretty nice to me now? 🙂


Random thought: should the to/from methods for IntPtr be called ToIntPtr/FromIntPtr, or would a different name make sense, now that we have numeric IntPtr and that in general we're moving away from using IntPtr explicitly, and rather just using nint/nuint (since they're true type aliases now, so same as never use eg. Int32 directly).

Like, just spitballing here, maybe:

public static UnsafeGCHandle FromOpaqueHandle(nint value);
public static nint ToOpaqueHandle(UnsafeGCHandle value);

Or does this just not matter and it's better to keep that naming scheme for historic reasons maybe?

@jkotas
Copy link
Member

jkotas commented Oct 28, 2023

We have approved and introduced more APIs that follow the FromIntPtr/ToIntPtr pattern just a few months ago #67090 (comment)

@Sergio0694
Copy link
Contributor Author

Ah, cool, well then the API proposal should be pretty much ready 😄

@hamarb123
Copy link
Contributor

On a slightly related note: it's a shame that there's we can't express object pinned from IL in C# for short pinnings (which did seem legal to write last time I read through that section). If we get GetRawData, we could basically do it.

@jkoritzinsky
Copy link
Member

For GetStringDataPointer, should it return char* as that is always the data type?

Also, if we were to go the generic route, then we could limit GetStringDataPointer and GetArrayDataPointer to only being on handles of valid types by implementing them as extension methods.

@Sergio0694
Copy link
Contributor Author

Making GetStringDataPointer return char* makes a lot of sense to me. I'm not entirely sure I see the value in making the type generic considering that it's just meant to be this very low level thing (and would the increased metadata size be an issue for the runtime using this type internally?). Like, DependentHandle also isn't generic. To me it feels like it would be more wrapping classes potentially being generic, like eg. WeakReference<T> does? 🤔

@jkotas
Copy link
Member

jkotas commented Oct 28, 2023

Is GetArrayDataPointer going to work for both single and multidimensional arrays (that comes with a small perf penalty) or is GetArrayDataPointer going to work for single dimensional arrays only?

@Sergio0694
Copy link
Contributor Author

Mmhh... I was assuming that it'd roughly be the same as Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<Array>(*(object*)_handle))), so that it'd still be fast (IIRC the MD version of GetArrayDataReference only has an extra indirection and a couple of mov-s?), while working with all possible array types? It would seem unfortunate otherwise for this not to be usable with MD arrays, for people that might want to use them.

Alternatively, if (1) the overhead for the general MD logic was considered too much for the SZ case, and (2) we did want to have built-in helpers for this in general, should we consider just having a separate API that'd work in all cases?

As in, spitballing:

public readonly void* GetSZArrayDataPointer();
public readonly void* GetArrayDataPointer();

(I will say I can't really come up with a good name for these two here though)

@MichalPetryka
Copy link
Contributor

I think that getting rid of the pointer getters makes the most sense here, the users can already achieve then with Unsafe.As anyway.

@Sergio0694
Copy link
Contributor Author

I assume the intent is to make it a bit nicer. I mean yes you can do it manually but it's a bit ugly:

// String
char* p = (char*)Unsafe.AsPointer(ref Unsafe.AsRef(in Unsafe.As<string>(handle.Target!).GetPinnableReference()));

// SZ array
T* p = (T*)Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(handle.Target!)));

// MD array
T* p  = (T*)Unsafe.AsPointer(ref MemoryMarshal.GetArrayDataReference(Unsafe.As<Array>(handle.Target!)));

I mean it's both ugly and also not really the ideal code to have to write manually due to how "questionable" it is.
Having some convenience APIs on the handle type makes sense to me? Just like GCHandle also does 🤔

Related question — should these APIs return null if the target is null, like GCHandle.AddrOfPinnedObject() also does?

@jkotas
Copy link
Member

jkotas commented Oct 30, 2023

Should these APIs return null if the target is null, like GCHandle.AddrOfPinnedObject() also does?

To follow the idea of maximally unsafe type idea, these APIs should throw NullReferenceException when the target is null. (Same as MemoryMarshal.GetArrayDataReference throws NullReferenceException for null.)

@Sergio0694
Copy link
Contributor Author

Sounds good! So it seems the only open question is the naming? Should I just leave those names as placeholders and then we can just bikeshed those during API review, as long as the API shape itself is fine? To recap, that'd basically be, for those 3:

public readonly void* GetSZArrayDataPointer();
public readonly void* GetArrayDataPointer();
public readonly char* GetStringDataPointer();

Actually I have a question on GetSZArrayDataPointer: because we're not in a generic context (ie. we can't just do that Unsafe.As<T[]> there), how would the SZ version actually work? Is there some magic trick we can do that I'm missing?

Otherwise, perhaps we could just have GetArrayDataPointer, and if people really need to drop that extra indirection they can then manually get the target and do that unsafe blurb to cast to T[] and call the specialized GetArrayDataReference?

@jkotas
Copy link
Member

jkotas commented Oct 30, 2023

Generic wrapper would address this nicely. Note that you can always use UnsafeGCHandle<object> if you do not want to pay overhead for arbitrary T.

@jkotas
Copy link
Member

jkotas commented Oct 30, 2023

My main concern with the generic version is that is still feels weird. Arguably it feels weirder.

@jkoritzinsky idea was to make GetArrayDataPointer and similar method to be extension methods, so that these overloads apply only when the generic argument is string, Array, T[]: #94134 (comment)

@Sergio0694
Copy link
Contributor Author

Oh, right. So something like this?

namespace System.Runtime.InteropServices;

public struct GCHandle<T> : System.IDisposable
    where T : class
{
    public GCHandle(T? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public T? Target { readonly get; set; }

    public static UnsafeGCHandle FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(UnsafeGCHandle value);

    public void Dispose();
}

public static class GCHandleExtensions
{
    public static T* GetArrayDataPointer<T>(this GCHandle<T[]> handle);
    public static void* GetArrayDataPointer(this GCHandle<Array> handle);
    public static char* GetStringDataPointer(this GCHandle<string> handle);
}

@alexrp
Copy link
Contributor

alexrp commented Oct 30, 2023

To be clear, I meant that the generic parameter should be the type of the object being retained, i.e. GCHandle<int[]> keeps an int[] alive.

@jkoritzinsky idea was to make GetArrayDataPointer and similar method to be extension methods, so that these overloads apply only when the generic argument is string, Array, T[]: #94134 (comment)

Annoyingly, we can't use where T : Array due to language limitations...


Side note, I might be in the minority here, but I feel like having these Get*Pointer methods on the type is a conflation of concerns. If it's that annoying to get at the object data, then that to me is an indicator that the Unsafe API surface is insufficient to begin with.

@jkotas
Copy link
Member

jkotas commented Oct 30, 2023

GetArrayDataPointer(this GCHandle<Array> handle) does not make sense to me - it is very unlikely somebody would ever have a GCHandle like that. We would want to have where T : Array here, but C# does not allow it.

@Sergio0694
Copy link
Contributor Author

Yeah I was just not really sure how to express that one otherwise 😅
Maybe we could leave that one out for now, since there's no decent way to express it in C#?


About the type being generic, are we worried at all about the fact it would be "inconsistent" with DependentHandle? Like, it feels maybe a bit weird that this one is generic, but DependentHandle is not. Especially considering that someone could very easily just write their own thin generic wrapper around a non generic UnsafeGCHandle type 🤔

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 4, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Nov 4, 2023
@Sergio0694
Copy link
Contributor Author

Hey folks! Trying to revive this now that .NET 9 is done, perhaps we can make this happen in 10? 😄

To recap, the current API proposal integrating all the previous comments would be this:

namespace System.Runtime.InteropServices;

public struct GCHandle<T> : System.IDisposable
    where T : class
{
    public GCHandle(T? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public T? Target { readonly get; set; }

    public static GCHandle<T> FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(GCHandle<T> value);

    public void Dispose();
}

public static class GCHandleExtensions
{
    public static T* GetArrayDataPointer<T>(this GCHandle<T[]> handle);
    public static char* GetStringDataPointer(this GCHandle<string> handle);
}

If this looks good, perhaps we could mark this as ready to review and go from there? 🙂

@jkotas
Copy link
Member

jkotas commented Aug 30, 2024

GCHandle<T> looks fine to me.

I am not sure about GCHandleExtensions methods. They make it easy to get unmanaged pointer pointing into unpinned memory by mistake, just like Unsafe.AsPointer.

@AaronRobinsonMSFT
Copy link
Member

    public static GCHandle<T> FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(GCHandle<T> value);

Are we considering GCHandle<T> simply a helpful abstract and we would permit passing an IntPtr from a GCHandle into GCHandle<T>.FromIntPtr(...)?

Trying to understand if the intent is to have GCHandle<T> and GCHandle be interchangable in practice.

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Aug 30, 2024

"They make it easy to get unmanaged pointer pointing into unpinned memory by mistake, just like Unsafe.AsPointer"

I thought the intent there was to make things more convenient in the spirit of "maximally unsafe use" you mentioned? 😅

@hez2010
Copy link
Contributor

hez2010 commented Aug 30, 2024

Trying to understand if the intent is to have GCHandle<T> and GCHandle be interchangable in practice.

My thought is that we can deprecate GCHandle, then promote people to use GCHandle<T> instead.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2024

Are we considering GCHandle simply a helpful abstract and we would permit passing an IntPtr from a GCHandle into GCHandle.FromIntPtr(...)?

Non-generic GCHandle uses lower bit of the handle to indicate whether the handle is pinned. This proposal wants to remove this bit to reduce overhead per the top post. It means that IntPtrs from GCHandle and GCHandle<T> are not exchangeable.

I thought the intent there was to make things more convenient in the spirit of "maximally unsafe use" you mentioned?

I made that comment when the proposal was non-generic UnsafeGCHandle.

@AaronRobinsonMSFT
Copy link
Member

This proposal wants to remove this bit to reduce overhead per the top post. It means that IntPtrs from GCHandle and GCHandle are not exchangeable.

Right. This is an important semantic detail we need to discuss during API review.

@Sergio0694
Copy link
Contributor Author

Perhaps they should be FromIntPtrUnsafe and ToIntPtrUnsafe to make it clear they're particularly unsafe if misused? 🤔

@AaronRobinsonMSFT
Copy link
Member

That is possible, but that shouldn't change the conversation much. There will be an inherent confusion if something named GCHandle and GCHandle<T> aren't compatible in some way.

Do we have another API where one is generic, the other not, and they are fundamentally incompatible?

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Aug 30, 2024

"There will be an inherent confusion if something named GCHandle and GCHandle<T> aren't compatible in some way."

Just replying to this point, this makes me wonder whether perhaps the original non-generic UnsafeGCHandle would be better, as it would avoid this problem entirely. It would keep being non-generic, meaning people would have to either cast and pay the small perf penalty, or use Unsafe.As<T>(object) when getting the target. But that is the same thing they need to do with GCHandle today anyway, and the primary goal of this new proposal was to improve performance. Making the usability nicer was only a secondary concern, so perhaps that would be fine.

Other proposal, for context:

public struct UnsafeGCHandle : System.IDisposable
{
    public UnsafeGCHandle(object? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public object? Target { readonly get; set; }

    public static UnsafeGCHandle FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(UnsafeGCHandle value);

    public readonly void* GetArrayDataPointer();
    public readonly void* GetStringDataPointer();
    public void Dispose();
}

We might even call those two DataPointer methods eg. GetPinnedArrayDataPointer for extra clarity.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2024

Perhaps they should be FromIntPtrUnsafe and ToIntPtrUnsafe to make it clear they're particularly unsafe if misused?

The GCHandle<T> IntPtrs are not more unsafe. They are just not interchangeable with GCHandle IntPtrs. People are going to find fairly quickly that they have done something wrong if they take IntPtr from pinned GCHandle and create a generic GCHandle around it. I am not worried about it too much.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2024

I like the generic GCHandle<T> better than UnsafeGCHandle. It mirrors what we have done with WeakReference. The generic WeakReference<T> fixed number of design mistakes from the non-generic one.

We might even call those two DataPointer methods eg. GetPinnedArrayDataPointer for extra clarity.

Prior art for naming methods that do stuff like this is Marshal.UnsafeAddrOfPinnedArrayElement. If we want to have the convenience methods, they should probably follow the same naming pattern.

@Sergio0694
Copy link
Contributor Author

Sergio0694 commented Aug 30, 2024

Sounds good! Updated API proposal then, for context:

namespace System.Runtime.InteropServices;

public struct GCHandle<T> : System.IDisposable
    where T : class
{
    public GCHandle(T? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public T? Target { readonly get; set; }

    public static GCHandle<T> FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(GCHandle<T> value);

    public void Dispose();
}

public static class GCHandleExtensions
{
    public static T* UnsafeAddrOfPinnedArrayElement<T>(this GCHandle<T[]> handle);
    public static char* UnsafeAddrOfPinnedString(this GCHandle<string> handle);
}

Something like this?

I suppose the last one might be "OfPnnedStringData" or something similar as well, possibly.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2024

Element in the array overload name does not look right since the method does not take element index.

I suppose the last one might be "OfPnnedStringData" or something similar as well, possibly.

We have GCHandle.AddrOfPinnedObject that would make it UnsafeAddrOfPinnedArray/UnsafeAddrOfPinnedString.
We also have RuntimeHelpers.OffsetToStringData that would make it UnsafeAddrOfPinnedArrayData/UnsafeAddrOfPinnedStringData.

@agocke agocke added this to AppModel Sep 11, 2024
@Sergio0694
Copy link
Contributor Author

Going back to this now that .NET 10 planning is starting 🙂

"We also have RuntimeHelpers.OffsetToStringData that would make it UnsafeAddrOfPinnedArrayData/UnsafeAddrOfPinnedStringData."

I personally like this one better, I feel like the "Data" suffix makes it clear that it's giving you a pointer to the raw object data.

Revised API proposal:

namespace System.Runtime.InteropServices;

public struct GCHandle<T> : System.IDisposable
    where T : class
{
    public GCHandle(T? value, GCHandleType type = GCHandleType.Normal);

    public readonly bool IsAllocated { get; }
    public T? Target { readonly get; set; }

    public static GCHandle<T> FromIntPtr(IntPtr value);
    public static IntPtr ToIntPtr(GCHandle<T> value);

    public void Dispose();
}

public static class GCHandleExtensions
{
    public static T* UnsafeAddrOfPinnedArrayData<T>(this GCHandle<T[]> handle);
    public static char* UnsafeAddrOfPinnedStringData(this GCHandle<string> handle);
}

Let me know if this looks reasonable and/or if we need any more tweaks to get this ready to review 🙂
I'll also update the first post so the final proposed API shape is easier to see.
Thank you!

@Sergio0694 Sergio0694 changed the title [API Proposal]: UnsafeGCHandle (like GCHandle, but this time it's great™️) [API Proposal]: GCHandle<T> (like GCHandle, but this time it's great™️) Dec 23, 2024
@Sergio0694
Copy link
Contributor Author

This will also benefit ComWrappers, in particular when #110828 gets merged. If we can switch the RCW cache to use GCHandle<NativeObjectWrapper> we'll get both the perf improvements from the new GC handle type, and be able to skip the type checks we're currently paying for when using pattern matching on the returned target from those handles (without needing to use Unsafe.As<T>(object), which felt not too great to use here).

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Runtime.InteropServices
Projects
Status: No status
Development

No branches or pull requests

10 participants