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]: RuntimeHelpers.ChangeType<TDelegate>(Delegate) API #57495
Comments
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. |
So this would be type unsafe API. Is it really that much better than Unsafe.As that you are using today? |
It would still be unsafe, yes. Callers would have to "pinky swear to respect the original type constraints" (quoting from Levi). |
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. |
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. Partially related question, mostly out of curiosity: should the current |
That said, the assumptions |
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.
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.) |
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 🙂
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? 😄 |
If we had a safe API I'd like it to hang directly off the 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 dotnet/csharplang#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. |
Maybe an example would help my understanding. Ignore the 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 |
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 😥
Yup, that is exactly right 😄 |
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?
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. |
The guideline that I have shared at #6176 (comment) still sounds about right. |
In Sergio's scenario 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 |
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. |
Levi beat me to it - yes,
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 |
If it was true, you would not need #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. |
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 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). 😄
I do agree we're kinda falling short on this second bit, that is true. But we want to go fast Jan 🤣 |
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. |
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 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 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
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 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 Updated benchmarks with this trick:
|
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 🙂 |
#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. |
Well I guess now we also have an actual example for that 😄 |
Closing as not actionable. |
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):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),
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
API Usage
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 🙂The text was updated successfully, but these errors were encountered: