dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.36k stars 4.74k forks source link

[API Proposal]: RuntimeHelpers.ChangeType<TDelegate>(Delegate) API #57495

Closed Sergio0694 closed 1 year ago

Sergio0694 commented 3 years ago

Background and motivation

We're currently using Unsafe.As<T>(object) quite a bit to optimize our messenger types in the MVVM Toolkit (https://aka.ms/mvvmtoolkit/docs), but I've been thinking for a while now that it'd be nice to remove that call to avoid relying on implementation details of the runtime to make things work, and to make the functionality safer in the future. Just to recap on the current setup (I've added a more extensive summary here), we basically have this (simplifying):

// The message handler delegate, whic is generic on the recipient type
public delegate void Handler<in TRecipient>(TRecipient recipient)
     where TRecipient : class;

// Message broadcast
object handler = GetTheHandler();
object recipient = GetTheRecipient();

Unsafe.As<Handler<object>>(handler)(recipient); // yehaa! 🤠

Now, this works fine because we're always passing the "right" recipient, so the actual type safety when the delegate is invoked is respected. But, we're still aliasing a reference type with a type that doesn't actually match the one of the target instance, and even though the runtime/GC are fine with this now, it's not really ideal, so to say. It works, though.

As @jkotas mentioned in our previous conversation on this (here),

"The specific use of Unsafe.As that you have highlighted sounds reasonable to me. I understand that the generic constrains are not always expressive enough to do what you need."

Despite there not being a way to do this via the language (it's also arguably a pretty niche feature, though I know I'm not the only one using this), I was thinking it would be nice to at least explore some options to do this in a way that doesn't rely on aliasing reference types with incompatible types and potentially causing GC issues on other runtimes and whatnot.

cc. @GrabYourPitchforks as we've been talking about this quite a lot in the #lowlevel C# Discord 😄

Related issues:

API Proposal

namespace System.Runtime.CompilerServices
{
     public static class RuntimeHelpers
     {
          public static TDelegate ChangeType<TDelegate>(Delegate del)
              where TDelegate : Delegate;
     }
}

NOTE: the returned delegate would not just be a new delegate wrapping the input one, but a new instance of the requested type with the same internal state, resulting in the same method/target being used when invoked.

API Usage

// Message registration
ref object storedHandler = ref GetTheHandler();

storedHandler = RuntimeHelpers.ChangeType<Handler<object>>(handler);

// Message broadcast
object handler = GetTheHandler();
object recipient = GetTheRecipient();

((Handler<object>)handler)(recipient);

Same functionality, hopefully negligible performance loss during broadcast (I could also keep using Unsafe.As<T>(object) there if that variant cast was noticeable, but at least the behavior there would now be safe and no longer aliasing an invalid type), and no longer relying on hacks to make the whole thing work while being fast and without reflection 😄

Risks

Low risk. Changing the delegate type to an invalid type could result in crashes and type safety violations at runtime when the new delegate is invoked. But the API would be in an inherently unsafe type and namespace, so only advanced users knowing what they're doing would ever see the API, let alone actually use them in their code. Besides, you can still do all sorts of ugly things with Unsafe.As<T>(object) anyway, which is also in the same namespace 🙂

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

jkotas commented 3 years ago

the proposed API would just ask the runtime to create a delegate of the new type, by copying all the internal fields (so target instance, function pointers including optional stub, etc.).

So this would be type unsafe API. Is it really that much better than Unsafe.As that you are using today?

Sergio0694 commented 3 years ago

It would still be unsafe, yes. Callers would have to "pinky swear to respect the original type constraints" (quoting from Levi). The main advantage would be that it would avoid the whole aliasing reference types of incompatible types at runtime, which though it works fine on CoreCLR/Mono now, I'm not entirely sure it's... Uh... Necessarily a good thing to do 😄

jkotas commented 3 years ago

I think if we were to introduce an API for this, it should be proper type-safe API. Yes, it will be slow, the simple initial implementation at least, but that's fine.

Sergio0694 commented 3 years ago

Would such a safe API actually work in this scenario though? Like, in this case I'm deliberately breaking type safety, in that I have a contravariant type parameter in a delegate and I'm converting the signature to accept a less derived type instead, only with the guarantee that the type will match at runtime. Like, if the API was to check the signature at instantiation time, wouldn't that throw because the signature of the new delegate (eg. Action<string> ---> Action<object>) doesn't match?

Partially related question, mostly out of curiosity: should the current Unsafe.As<T>(object) trick working on CoreCLR/Mono be considered as relying on implementation details or is that technically fine according to ECMA as well? I've gone through the relative chapters but I'm still not entirely sure about this. Like, it should work but it's not like ECMA specifically makes any guarantees on VM/GC behavior if one decides to alias incompatible types at runtime. 😄

alexrp commented 3 years ago

Unsafe definitely relies on implementation details, and not just casting. IIRC, there's no particular guarantee in Ecma 335 that managed pointers have to be represented as actual pointers at all; an implementation could conceivably use some weird kind of GCHandle to implement managed pointers if it really wanted.

That said, the assumptions Unsafe makes are reasonable in practice. It's unlikely that any Ecma 335 implementation wouldn't be compatible with them.

jkotas commented 3 years ago

is that technically fine according to ECMA as well?

The ECMA does not generally say anything about the behavior when the type safety is violated. Since you would be still violating type safety even with this API (I have missed that in your write up), it would still be in the gray area.

It's unlikely that any Ecma 335 implementation wouldn't be compatible with them.

I think the highest risk of incompatibilities come from codegen optimizations. Some of the advanced codegen optimizations assume type safety. Your code may break in odd ways with type safety violations. (This is a potential problem. I do not have an actual example in hand to demonstrate this problem.)

Sergio0694 commented 3 years ago

"I think the highest risk of incompatibilities come from codegen optimizations. Some of the advanced codegen optimizations assume type safety."

Yeah, I remember @AndyAyersMS the other day was just about talking about how the fact we can now alias reference types might interfere with some JIT optimizations that might potentially have to be reverted, though Levi also said we might just decide not to care about people deliberately breaking type safety like that and avoid the performance regression for others not doing similar hacks (so almost everyone). Point is, this was one of the reasons why I wanted to at least start a conversation about potentially finding a safe (or at least, properly supported) way to achieve what I described in this issue, without relying on type aliasing 🙂

"Since you would be still violating type safety even with this API [...], it would still be in the gray area."

What I'm saying though is that assuming consumers of this API (advanced users that know exactly what they're doing) do respect the actual type constraints of arguments passed during involution (like I mentioned in the issue), you would effectively no longer have any type safety violations though, no? I mean of course if you then passed invalid params you'd break type safety, but if not, everything would then be valid and no longer relying on hacks. No reference type aliasing with incompatible types or anything, you'd just be invoking a delegate with a valid type (created by the runtime), wrapping a method that accepts parameters that are valid with the ones you're passing. So that'd be... Safe? 😄

GrabYourPitchforks commented 3 years ago

If we had a safe API I'd like it to hang directly off the Delegate type for discoverability.

If we wanted to have an unsafe API to create a delegate wrapped around an arbitrary 'this' object and function pointer, would it be sensible to expose the delegate ctors via https://github.com/dotnet/csharplang/discussions/4871 as previously mentioned? I had batted around the idea of a separate unsafe API for this a while back, but the feedback I got was that exposing the ctors would make a new unsafe API redundant.

@jkotas When you say violating type safety, are you referring to undefined behavior? ECMA does call out scenarios which are type-unsafe but which have well-defined behavior assuming the caller operates within some boundaries. But I don't know where this particular scenario falls. My understanding of the scenario is that Sergio wants a delegate whose parameter is typed as TBase but where the target method has a parameter typed as TDerived, and he pinky swears that he'll only ever pass a TDerived into the delegate's invoke method.

GrabYourPitchforks commented 3 years ago

Maybe an example would help my understanding. Ignore the Delegate type for a sec. Let's say I have this code:

static void DoIt(string s) { /* ... */ }

delegate*<string, void> del1 = &DoIt;
delegate*<object, void> del2 = (delegate*<object, void>)(void*)del1;
del2("Hello!");

It's clearly not type-safe to have an object -> void fnptr pointing to a string -> void method. But is it undefined behavior?

Sergio0694 commented 3 years ago

"would it be sensible to expose the delegate ctors via dotnet/csharplang#4871"

One of my issues with that is that that would also crash if the delegate is wrapping a dynamic method (not sure if there's other cases too). So that would be a regression for the MVVM Toolkit, for instance. I know that's arguably a niche scenario, but I also have no idea what all consumers of the library are doing with it, so I wouldn't want to switch to an alternative solution that would not work in some scenarios that are instead fully supported today 😥

"My understanding of the scenario is that Sergio wants a delegate whose parameter is typed as TBase but where the target method has a parameter typed as TDerived, and he pinky swears that he'll only ever pass a TDerived into the delegate's invoke method"

Yup, that is exactly right 😄 I will absolutely pinky swear to respect the type safety of the arguments that I pass when invoking, but my issue is that my broadcast methods cannot know what type arguments each stored handler have, so the only way to invoke them is to just lie and downcast each delegate to accept an object in place of each TFoo, and then relying on the fact that each recipient we retrieve for each invoked delegate is the right one.

jkotas commented 3 years ago

expose the delegate ctors

Note that the raw delegate ctor resolves the pointer back to MethodDesc first (that is about as expensive as going to MethodInfo), then it compares the signatures of the MethodDesc and the delegate, based on how the signatures compare creates the various thunks to shuffle the arguments, and finally creates the delegate.

The JITed code only calls the raw delegate ctors as fallback in rare situations (e.g. error cases like signature mismatches). For the common cases, the JIT treats the ldftn + delegate ctor pair as intrinsic, all of the above happens at JIT time and JITed code just calls a specific helper for the given delegate case.

So creating the delegate via the raw delegate ctor would be about as fast as creating the delegate from MethodInfo. None of current JIT optimizations would kick in. Is that still useful? Why not just create the delegate from the MethodInfo in the first place instead of going through function pointers?

When you say violating type safety, are you referring to undefined behavior?

Yes. For example: The delegate can be bound virtually. In future, we may do delegate inlining and speculative devirtualization for this case. Is it going to work properly when some of the signatures involved violate type safety? I would not bet on it.

jkotas commented 3 years ago

The guideline that I have shared at https://github.com/dotnet/runtime/issues/6176#issuecomment-227007187 still sounds about right.

GrabYourPitchforks commented 3 years ago

Why not just create the delegate from the MethodInfo in the first place instead of going through function pointers?

In Sergio's scenario CreateDelegate would fail because the actual method signature is not compatible with the type of the delegate being created. He's relying on the two signatures (real and projected) having compatible calling conventions. But you're right - perhaps even this is undocumented / undefined behavior and not something we can guarantee for all time.

One potential solution could be for Sergio to ref emit a thunk with the correct signature, then create the delegate pointing to that thunk. Within that ref emit code he could Unsafe.As to his heart's content. And I presume this technique would be fully supported and would have predictable behavior across all runtimes. But it seems a bit heavyweight. Is there anything - even if it relies on the unsafest of unsafe code - that can be done today as a workaround where we can also guarantee forward support? (I mean a formal statement of support, above and beyond the guidelines you lay out at https://github.com/dotnet/runtime/issues/6176#issuecomment-227007187.) If so, then maybe callers could leverage that ugly technique for now, giving us time to build a better API on top?

jkotas commented 3 years ago

Is there anything that can be done today

Rethink the design to avoid dependency on reflection for these sort of bindings and source generate all of it in type-safe, trimming and AOT-friendly way at build time.

The MVVM toolkit is targeted on desktop or mobile apps. The startup time for these type of apps has always been the paramount concern, and use of reflection has always been the source of problem. We have a constant source of customer escalations due to poor startup time of these type of apps. If there is an escalation around steady state, it is typically around GC pauses causing UI stutter. I do not recall the last time when plain vanilla steady state performance - that this is trying to slightly improve - has been a problem.

Sergio0694 commented 3 years ago

Levi beat me to it - yes, CreateDelegate wouldn't work for our scenario unfortunately as the fact remains that the new signature we're using to store delegates is in fact invalid, as we're taking a contravariant T type argument and just acting like it was just object, after pinky swearing that we'll actually only ever pass valid T instances when invoking to make sure things don't explode. One of the main points of this proposal (as I was expecting questions and potentially some pushback) was to at least start a conversation on this to figure out some way to achieve the same without a performance loss and in a fully supported way 🙂

"Rethink the design to avoid dependency on reflection for these sort of bindings [...] The startup time for these type of apps has always been the paramount concern, and use of reflection has always been the source of problem."

I'm a bit confused by that part Jan - we do have a pretty strict "no reflection" policy across the whole MVVM Toookit already (and it's only ever used in some specific scenarios, which will also go away with the new source generators support), but this was not one of those. The messenger types themselves use no reflection whatsoever already, was that in reply to Levi's idea of using dynamic IL to generate the thunks (which I wouldn't want to do either due to the overhead, slow startup, and dynamic IL not being supported in fully AOT scenarios), or was there a misunderstanding on how our messenger types work on their own? Just double checking 😄

Also I should note that we can't really change the messenger interface at this point, and also, I would actually much prefer a proper way to support it in the first place, instead of having to alter that purely to work around a lack of runtime/language support in this area. As things stand, I guess I'll just have to stick with the current Unsafe.As solution given that it's basically guaranteed to work on CoreCLR/Mono at least for now, and possibly reevaluate this in the future in case anything changed. I would have really liked some "still unsafe but supported" way of doing this though 🥲

jkotas commented 3 years ago

we do have a pretty strict "no reflection" policy across the whole MVVM Toookit already

If it was true, you would not need https://github.com/dotnet/runtime/issues/50333 and the MVVM code would not have references to System.Linq.Expressions, etc. I understand that you are trying to pull all sort of tricks to implement fast paths that try to avoid reflection, but that is not really "no reflection". My suggestion was no reflection and type safe by construction.

Sergio0694 commented 3 years ago

I feel like there might be a small misunderstanding here about the architecture of the MVVM Toolkit given those references to LINQ expressions and my other proposal that I feel like are not necessarily related to this, let me try to do a small recap of the current setup in case it helps. If there was no misunderstanding and I had just read your message wrong, I apologize 😄

We basically have:

This is what I meant earlier by saying that we have a "pretty strict "no reflection" policy across the whole MVVM Toookit already except for some very specific scenarios" (the two overloads I mentioned above, and even then with source generators we're only left with a single GetType().GetMethod() call, and compiled LINQ expressions aren't used at all anymore). What I mean is that neither that LINQ expressions path nor that reflection bit are effectively related to the messengers themselves.

This is why I don't feel like saying the library as a whole relies on reflection or on LINQ expressions is accurate, as it's really just this very specific and optional helper method. The ask in this issue to offer a way to convert the signature of a given delegate to avoid relying on that unsafe cast was about a core functionality of the messenger types themselves, which in fact do not ever rely on any kind of reflection, and as such are also already AOT and trimmer friendly. I also wouldn't really know how to "rethink the design" of the whole messaging part of the library to avoid relying on reflection, because as far as I can tell that is in fact already the case (also we wouldn't want to completely break all existing customers either). 😄

"My suggestion was no reflection and type safe by construction."

I do agree we're kinda falling short on this second bit, that is true. But we want to go fast Jan 🤣

jkotas commented 3 years ago

I also wouldn't really know how to "rethink the design" of the whole messaging part of the library to avoid relying on reflection, because as far as I can tell that is in fact already the case

The current solution with type safety violations is fragile. To make it robust and future proof, you would need to generate small stubs to avoid the bad type safety violations as @GrabYourPitchforks suggested. You do not want to generate them at runtime because of it would use reflection. What would it take to generate them at build time using source generators? It is what I meant by rethink the design.

Sergio0694 commented 3 years ago

I've given some more thought on the "generating stubs to invoke the handlers" and "we should avoid invalid type aliasing" issues as suggested by Jan, and I think I might've reached a prototype that seems to be working well, fit in into our existing architecture, support all the scenarios that are also already supported today, and offer comparable performance.

To recap, we have this handler:

public delegate void MessageHandler<in TRecipient, in TMessage>(TRecipient recipient, TMessage message)
    where TRecipient : class
    where TMessage : class;

And our IMessenger interface exposes this API:

void Register<TRecipient, TMessage, TToken>(TRecipient recipient, TToken token, MessageHandler<TRecipient, TMessage> handler)
    where TRecipient : class
    where TMessage : class
    where TToken : IEquatable<TToken>;

We have two ways of registering messages in the MVVM Toolkit: either by manually passing a delegate (eg. via a lambda expression), or by having your recipient implement the IRecipient<TMessage> interface and then calling one of the Register extensions. These will just call the interface method with IRecipient<TMessage> as the recipient type, and with the handler just proxying to recipient.Receive(message), with the input recipient and message. A soltion no longer relying on using Unsafe.As<T>(object) to reinterpret delegate types and lie on their signatures would need to support both of these, ideally with further optimizations to leverage the differences in how these two approaches work.

I've come up with the following implementation idea:

// Interface internally stored in the messenger types in place of delegates
internal interface IMessageHandlerDispatcher
{
    void Invoke(object recipient, object message);
}

// Wrapper dispatcher when a delegate handler is passed. This needs to track both
// generic arguments for the recipient and the message. The handler is stored as is.
internal sealed class MessageHandlerDispatcher<TRecipient, TMessage> : IMessageHandlerDispatcher
    where TRecipient : class
    where TMessage : class
{
    private readonly MessageHandler<TRecipient, TMessage> handler;

    public MessageHandlerDispatcher(MessageHandler<TRecipient, TMessage> handler)
    {
        this.handler = handler;
    }

    public void Invoke(object recipient, object message)
    {
        this.handler(Unsafe.As<TRecipient>(recipient), Unsafe.As<TMessage>(message));
    }
}

// Wrapper dispatcher when a recipient is registered as IRecipient<TMessage>. In this case
// the wrapper itself is stateless, so we can easily cache all instances and reuse them. This
// also avoids the extra delegate indirection we had previously with the stateless lambda.
internal sealed class MessageHandlerDispatcher<TMessage> : IMessageHandlerDispatcher
    where TMessage : class
{
    public static MessageHandlerDispatcher<TMessage> Instance { get; } = new();

    public void Invoke(object recipient, object message)
    {
        Unsafe.As<IRecipient<TMessage>>(recipient).Receive(Unsafe.As<TMessage>(message));
    }
}

Basically:

There are no more unsafe type casts with invalid object types, so there should no longer be any concerns about potentially undefined behaviors or things falling apart in the future in case the runtime/JIT added new optimizations regarding delegates invocation. Also with this solution, as mentioned above, all existing scenarios will keep being supported, and there is no need to introduce new source generators or to restrict the number of usages we can support with these types (including eg. dynamic methods and whatnot). Everything should keep working as before for end users.

I did some benchmarks with this prototype, for the IRecipient<TMessage> scenario:

Method Mean Error StdDev Ratio
Unsafe.As 22.05 ms 0.145 ms 0.128 ms 1.00
IWrapper 22.54 ms 0.113 ms 0.089 ms 1.02

It would seem the performance is comparable, just ever so slightly slower I guess because an interface call is slightly slower than a delegate invocation due to the virtual dispatch. Not a big deal though. I have a hunch that Jan and Levi would happily recommend taking a 2% perf hit (and the benchmark is an extreme scenario anyway, it'd be less noticeable in real world applications) in exchange for more type safety 😄

@jkotas @GrabYourPitchforks does this seem like a reasonable solution to you, given the scenario at hand and your previous feedbacks? I know this is a bit out of topic for the proposal itself, but I figured I'd pick your brain on this idea since we were talking about this. I wouldn't be opposed to closing this proposal either in case this was deemed a good alternative and there was no more interest in offering an API like the one originally proposed anymore. Thanks again for your time! 🙂


EDIT: after thinkering with the idea some more and following a suggestion from @EgorBo about also trying to manually do guarded devirt for the most common happy path, I've managed to completely remove that perf regression and actually end up with a slightly faster solution that I had before. The trick was to add an empty (cached and reused) marker class for the case where an IRecipient<TMessage> instance is registered, to then check and skip the double indirection like this:

object handler = GetTheHandler();
object recipient = GetTheRecipient();

if (handler.GetType() == typeof(MessageHandlerDispatcherMarker))
{
    Unsafe.As<IRecipient<TMessage>>(recipient).Receive(message);
}
else
{
    Unsafe.As<IMessageHandlerDispatcher>(handler).Invoke(recipient, message);
}

This works fine because MessageHandlerDispatcherMarker is internal and only assigned when a recipient is registered and it implements the IRecipient<TMessage> interface, so in that case we can just invoke the interface method on that target instance directly and bypass the wrapper (which would've been the other slow interface dispatch). We can't do this in the other case as there's an unknown TRecipient type argument, but that scenario is less likely to happen anyway.

Updated benchmarks with this trick:

Method Mean Error StdDev Ratio RatioSD
Unsafe.As 21.94 ms 0.415 ms 0.389 ms 1.00 0.00
IWrapper + opt 20.69 ms 0.215 ms 0.201 ms 0.94 0.02
Sergio0694 commented 3 years ago

Hey @GrabYourPitchforks, just wanted to touch base on this again to help clear the backlog. As we've discussed, the proposed workaround works great for me in the MVVM Toolkit and I'll probably end up doing that once work on that kicks off again on the .NET Community Toolkit. We can close this proposal if there if you don't think there might still be value for other scenarios. Doesn't seem to be that way after the conversation with Jan, but figured I'd ask again to confirm. Feel free to close if so! 😄

EDIT: spoke with Levi on Discord, suggested to still leave this open in case it's useful for others 🙂

jkotas commented 2 years ago

62067 is an example of a crash that can be triggered by unsafe delegate or function pointer casts with compatible signatures.

Note that the fix for #62067 fixed the unsafe casts of function pointers with compatible signatures; but unsafe casts of delegates with compatible signatures are still exposed to the problem.

Sergio0694 commented 2 years ago

"It's clearly not type-safe to have an object -> void fnptr pointing to a string -> void method. But is it undefined behavior?"

Well I guess now we also have an actual example for that 😄 Thank you for sharing! I'm even happier I've now removed all unsafe casts for delegates in the MVVM Toolkit now 😁 I went with that wrapper mentioned in my previous comment, which also ended up being faster anyway, so win-win.

jkotas commented 1 year ago

Closing as not actionable.