[API Proposal]: GCHandle<T> (like GCHandle, but this time it's great™️) #94134
Comments
Sergio0694
added
the
api-suggestion
dotnet-issue-labeler
bot
added
the
needs-area-label
ghost
added
the
SingleAccretion
added
area-System.Runtime
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsThis API proposal is extracted from #94113 (comment) and supersedes/fixes:
OverviewThe
We have a reference implementation which can be a starting point for this. cc. @jkotas @AaronRobinsonMSFT @tannergooding API Proposalnamespace 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
API UsageSame as // In a constructor (or wherever)
this.handle = UnsafeGCHandle.Alloc(myObj);
// Later on
object? target = this.handle.Target();Alternative DesignsThe alternative would be to change RisksNot really any risk, since this would be a brand new type.
|
SingleAccretion
added
area-System.Runtime.InteropServices
and removed
area-System.Runtime
labels
|
Tagging subscribers to this area: @dotnet/interop-contrib Issue DetailsThis API proposal is extracted from #94113 (comment) and supersedes/fixes:
OverviewThe
We have a reference implementation which can be a starting point for this. cc. @jkotas @AaronRobinsonMSFT @tannergooding API Proposalnamespace 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
API UsageSame as // In a constructor (or wherever)
this.handle = UnsafeGCHandle.Alloc(myObj);
// Later on
object? target = this.handle.Target();Alternative DesignsThe alternative would be to change RisksNot really any risk, since this would be a brand new type.
|
Unnecessary.
It should be a constructor (same as what we have done for
This method needs to do several checks against the type of the object. It may be better to have
|
|
Updated the proposal integrating your suggestions 🙂 For Any thoughts on whether we can just make |
I assume that this API would be a shorthand for
Yes. I think the idea is that this is maximally unsafe type. |
Should
Given we have and allow 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 |
|
I was first a bit skeptical of going as far as exposing I'm not sure they belong on 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 Additional benefit: after you initially cast the pointer type, you can also preserve nullability annotations. |
|
Noting that using Alternative names than |
From/ToIntPtr returns opaque handle.
|
|
Has there been any consideration to making the handle struct generic, to make it more type-safe and ergonomic? |
|
Well, alright then we can ignore the whole
I've updated the API shape integrating all the new suggestions:
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 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? |
|
We have approved and introduced more APIs that follow the FromIntPtr/ToIntPtr pattern just a few months ago #67090 (comment) |
|
Ah, cool, well then the API proposal should be pretty much ready 😄 |
|
On a slightly related note: it's a shame that there's we can't express |
|
For GetStringDataPointer, should it return 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. |
|
Making |
|
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? |
|
Mmhh... I was assuming that it'd roughly be the same as 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) |
|
I think that getting rid of the pointer getters makes the most sense here, the users can already achieve then with Unsafe.As anyway. |
|
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. Related question — should these APIs return |
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.) |
|
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 Otherwise, perhaps we could just have |
|
Generic wrapper would address this nicely. Note that you can always use |
|
Going back to this now that .NET 10 planning is starting 🙂
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 🙂 |
Sergio0694
changed the title
|
This will also benefit |
tannergooding
added
api-ready-for-review
How about introducing a dedicated type for pinned GC handle? It would ensure that the extension methods are only ever used on pinned handles: struct PinnedGCHandle<T>
{
public PinnedGCHandle(T? value);
// the rest is same as GCHandle<T>
}
class GCHandleExtensions
{
public static T* AddrOfArrayData<T>(this PinnedGCHandle<T[]> handle);
public static char* AddrOfStringData(this PinnedGCHandle<string> handle);
} |
I was thinking that since we're going for "maximally unsafe type", that could just be undefined? Eg. I'd imagine on CoreCLR/NAOT it'd throw a
I figured that'd also just be undefined behavior, for the same reason as above.
I have two questions about this:
|
I made the comment about "maximally unsafe type" when the proposal was non-generic UnsafeGCHandle. The generic
UnsafeAddrOfPinnedArrayData/UnsafeAddrOfPinnedStringData has additional concern of Target being null. It does not sound right for that to be undefined.
It would be still allowed to create a GCHandle as pinned. You would not have the convenience methods to get the pointer to pinned data. Instead, you would have to use |
Would it be reasonable to say:
Related question: should we rather have it return a
Makes sense. So the revised API shape would be: 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 struct PinnedGCHandle<T> : System.IDisposable
where T : class
{
public PinnedGCHandle(T? value);
public readonly bool IsAllocated { get; }
public T? Target { readonly get; set; }
public static PinnedGCHandle<T> FromIntPtr(IntPtr value);
public static IntPtr ToIntPtr(PinnedGCHandle<T> value);
public void Dispose();
}
public static class PinnedGCHandleGCHandleExtensions
{
public static T* UnsafeAddrOfPinnedArrayData<T>(this PinnedGCHandle<T[]> handle);
public static char* UnsafeAddrOfPinnedStringData(this PinnedGCHandle<string> handle);
}Additional caveats:
Does this look good? 🙂 |
Undefined behavior typically includes hard to diagnose runtime crash. If we want to guarantee that it throws an exception, we can as well specify the exception that is going to thrown. I think guaranteeing NullReferenceException would not be a significant burden.
Convenience methods in interop namespace typically handle nulls gracefully if it makes sense. For example, the existing
It should still follow |
|
Makes perfect sense for the updated name and guaranteeing a NRE, as well as just returning a null pointer 😄
Of course! I just meant that it won't have to bother using an interlocked exchange to mark reset its handle value 👍 Updated API surface and caveats: 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 struct PinnedGCHandle<T> : System.IDisposable
where T : class
{
public PinnedGCHandle(T? value);
public readonly bool IsAllocated { get; }
public T? Target { readonly get; set; }
public static PinnedGCHandle<T> FromIntPtr(IntPtr value);
public static IntPtr ToIntPtr(PinnedGCHandle<T> value);
public void Dispose();
}
public static class PinnedGCHandleGCHandleExtensions
{
public static T* AddrOfArrayData<T>(this PinnedGCHandle<T[]> handle);
public static char* AddrOfStringData(this PinnedGCHandle<string> handle);
}Additional caveats:
|
|
Small follow up. I know we can't do public static class PinnedGCHandleGCHandleExtensions
{
public static T* AddrOfArrayData<T>(this PinnedGCHandle<T[,]> handle);
public static T* AddrOfArrayData<T>(this PinnedGCHandle<T[,,]> handle);
} |
|
Another follow up. @jkotas I was looking at the current We can't do that check in the new type, because the design is we don't want to store the pinned flag in the handle anymore. So when someone assigns |
|
Just to check, will we be allowing any type for I don't think any of the other |
Presumably it would not actually be hard for roslyn to support I would think this is a better solution if roslyn could consider the feature relatively quickly (by vNext) (obviously, if the feature would take a while for them to get to or something like that, we probably wouldn't want to hold up this feature for that). |
How about simply public static class PinnedGCHandleGCHandleExtensions
{
public static T* AddrOfArrayData<T>(this PinnedGCHandle<T> handle);
}then verify the |
|
That method doesn't do the same thing at all. If you call it on a Edit: misread, but the issue is the same. You don't want a pointer back that's of the same |
We can always add these later if somebody demonstrates that these methods are needed often enough. One can always handle these cases using
The non-pinnable concept has been an artificial limitation in the existing GCHandle. The implementation of GC handles should handle pinning of objects with references just fine. |
Then it can be returning a public static class PinnedGCHandleGCHandleExtensions
{
public static nint AddrOfArrayData<T>(this PinnedGCHandle<T> handle);
} |
|
That would be much less convenient than the strongly typed version that's currently in the proposed. What I could potentially see having would rather be something like (in addition to the other two extensions): public static void* AddrOfObjectData<T>(this PinnedGCHandle<T> handle);Which could be used in all other cases (not just arrays), and I know stuff like this has been requested before too. |
Well, I would think it would make sense to get the actual I personally think this API would be useful, but it only really makes sense if we have the building block ( |
|
Yup, I don't disagree. I did say "potentially", it doesn't have to be in this first proposal. The current API shape is fine I think 🙂 |
There is one too many |
|
oops, typo, I meant to just say |
namespace System.Runtime.InteropServices;
public struct GCHandle<T> : System.IDisposable
where T : class?
{
public GCHandle(T value);
public readonly bool IsAllocated { get; }
public readonly T Target { get; set; }
public static GCHandle<T> FromIntPtr(IntPtr value);
public static IntPtr ToIntPtr(GCHandle<T> value);
public void Dispose();
}
public struct PinnedGCHandle<T> : System.IDisposable
where T : class?
{
public PinnedGCHandle(T value);
public readonly bool IsAllocated { get; }
public readonly T Target { get; set; }
public readonly void* GetAddressOfObjectData();
public static PinnedGCHandle<T> FromIntPtr(IntPtr value);
public static IntPtr ToIntPtr(PinnedGCHandle<T> value);
public void Dispose();
}
public struct WeakGCHandle<T> : System.IDisposable
where T : class?
{
public WeakGCHandle(T value, bool trackResurrection = false);
public readonly bool IsAllocated { get; }
public readonly bool TryGetTarget([NotNullWhen(true)] out T? target);
public readonly void SetTarget(T target);
public static WeakGCHandle<T> FromIntPtr(IntPtr value);
public static IntPtr ToIntPtr(WeakGCHandle<T> value);
public void Dispose();
}
public static class GCHandleExtensions
{
public static unsafe T* GetAddressOfArrayData<T>(this PinnedGCHandle<T[]?> handle);
public static unsafe char* GetAddressOfStringData(this PinnedGCHandle<string?> handle);
} |
bartonjs
added
api-approved
This API proposal is extracted from #94113 (comment) and supersedes/fixes:
Overview
The
GCHandletype 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:FreeIDisposableWe have a reference implementation which can be a starting point for this.
cc. @jkotas @AaronRobinsonMSFT @tannergooding
API Proposal
Additional caveats
Targeton a handle that's not allocated will throwNullReferenceExceptionAddrOf*on a handle that's not allocated will throwNullReferenceExceptionAddrOf*when the target isnullwill return anullpointerFromIntPtr/ToIntPtrif the handle type is mismatched is undefined behaviorDisposeis not thread safe (but still no-op after the first call, just not in a thread-safe manner)API Usage
Same as
GCHandle, really: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.
The text was updated successfully, but these errors were encountered: