Enable Unsafe.As NotNullIfNotNull on .NET Standard 2.0

[Sergio0694]

Overview

The Unsafe.As<T>(object) API was not using the nullability attribute for the return value when not on .NET Standard 2.1:

runtime/src/libraries/System.Runtime.CompilerServices.Unsafe/ref/System.Runtime.CompilerServices.Unsafe.cs

Lines 21 to 24 in 37c5a3e

	#if NETSTANDARD2_1
	[return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull("o")]
	#endif
	public static T? As<T>(object? o) where T : class? { throw null; }

This caused some incorrect null reference warnings when using the library in a .NET Standard 2.0 project, eg.:

This PR simply removes the compiler directive so that the .NET Standard 2.0 target will still have access to that nullability attribute.

[stephentoub]

Assuming CI passes, LGTM.

[jkotas]

Thank you!

[Sergio0694]

Happy to help! 🎉

@jkotas small question, since this change is very small do you think there's a chance it will end up being integrated into the public 5.0 release of the S.R.CS.Unsafe package? I know the runtime itself has been branched off already for .NET 5 but not sure what the workflow is for external packages like this one, so thought I'd ask. Thanks! 🙂

[stephentoub]

@jeffhandley, looking over the bar we've set, this almost meets it: it won't introduce new warnings, it is related to new annotations in .NET 5, and it is based on feedback... but it's not a commonly-used API (it's very advanced, and this will only show up for netstandard2.0 consumers). Thoughts on whether to try to port it to release/5.0? It is very low risk.

[Sergio0694]

@stephentoub I'm using this API quite a bit in the Microsoft.Toolkit.Mvvm and Microsoft.Toolkit.HighPerformance packages (which both also have .NET Standard 2.0 as one of their targets), so having the annotation available would be pretty nice there. I was working on a new PR for both of them yesterday to integrate the new IsNullRef and NullRef APIs and as soon as I upgraded to the 5.0.0-rc1 package, StyleCop immediately started complaining about the warnings in Release n a few places 😅

That is to say, personally if this made it into the final 5.0 package it would be appreciated!

[jkotas]

What is the bar we are shooting for with nullability annotations for netstandard2.0?

I thought that the nullability annotations in netstandard2.0 are largely non-existent and there is no consistency, so this fix does not really move the needle on the nullability annotations when you are using netstandard2.0.

[stephentoub]

I thought that the nullability annotations in netstandard2.0 are largely non-existent and there is no consistency

Correct. The only APIs from dotnet/runtime that will have annotations for a netstandard2.0 consumer are those we publish in packages available independently of netcoreapp. But it's question of whether this actually hurts. Those unannotated APIs in netstandard2.0 are "oblivious", so the compiler won't warn about them. But if a nullable-enabled library consumes this package, it will end up producing more warnings than it should, because the API is annotated.

[Sergio0694]

@jkotas Yeah, I see no nullability annotations on .NET Standard 2.0 in general, but those ones from the .NET Standard 2.0 target of the S.R.CS.Unsafe 5.0.0-RC1 are causing warnings because the return TTo? is marked as nullable, but lacks that flow attribute.

[jeffhandley]

Thanks for reporting and fixing this, @Sergio0694. While this fix is very low risk, it doesn't meet the bar we've defined for fixing nullable annotation attributes in 5.0. Here's the bar we're using at this point. This checks off every aspect except the last bullet.

• Must not introduce new warnings. A few examples (but this is not exhaustive):
  • Changing the return type of a non-virtual member from nullable to non-nullable is OK; the other direction is NOT OK
  • Changing the return type of a virtual member is NOT OK
  • Changing a method parameter from non-nullable to nullable is OK; the other direction is NOT OK
• Must be related to annotations made during 5.0 (not in 3.0)
• Must be based on customer feedback
• Must affect a commonly-used API

Referring to the API usage data, this API is used in 1.1% of apps/libraries scanned. Sorry for the inconvenience on this.

[michael-hawker]

Thanks @jeffhandley, is the low usage the only blocker?

This API is helping to optimize our new MVVM Toolkit package for the Windows Community Toolkit. We'll be shipping it in the Nov/Dec timeframe.

This new .NET Standard based MVVM library is the successor to MVVMLight, so we're expecting it to get picked up quite a bit.

[stephentoub]

To be clear, is the impact more significant to you than adding a ! in a few places? While it'd be great if our annotations were perfect, it seems like a minor nuisance in a few places rather than something that would meaningfully impact your release. No? Thanks.

[jkotas]

This API is helping to optimize our new MVVM Toolkit package for the Windows Community Toolkit

If you are using this API a lot, you are doing something wrong. This API is a very unsafe construct, and it should be used only in a few most performance critical situations.

[Sergio0694]

Hey @jkotas, I guess I can answer to that 😄

There are a couple places where I'm using that API in the MVVM Toolkit, and they're indeed in a performance oriented type.
They're only being used on values being of private types, coming from private fields, and guaranteed to be correct due to the implementation of the type itself, so the only way this could break is if a dev actually tried to mess up with the internal fields using reflection, which is something that not even types from the BCL guard against, as far as I can tell (eg. Memory<T> will break the type safety too in the Span property getter if a dev used private reflection to store some arbitrary object there).

In particular, there's a usage of the Unsage.As<T>(object) API in the MVVM Toolkit that just can't be worked around without having to sacrifice quite a fair bit of performance and more memory usage, which is this line here:

// Assume we have these locals
object handler;
object recipient;
TMessage message;

// Here we perform an unsafe cast to enable covariance for delegate types.
// We know that the input recipient will always respect the type constraints
// of each original input delegate, and doing so allows us to still invoke
// them all from here without worrying about specific generic type arguments.
Unsafe.As<MessageHandler<object, TMessage>>(handler)(recipient, message);

Basically I have the following situation:

• I have this delegate type, which is used to register message recipients with the ability to specify both the recipient type (so the input to the lambda expression is already of the right type and they don't need to cast), and the message type:

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

• As you can see from the delegate, TRecipient is always a reference type
• The messenger class will guarantee that always the right recipients are passed

So doing that unsafe cast basically tricks the runtime into just calling the Invoke method of each actual handler just with an input object type (as we don't know the type used in each handler here) - we're essentially invoking a contravariant delegate as if it was covariant in that input argument. Again this is always guaranteed to be a valid type cast due to how the messenger itself works - each recipient will always match the original TRecipient constraint in each delegate being invoked. This way we get identical codegen to just invoking those handlers directly, even if we lack knowledge on each type in use.

The alternatives I've considered here would've been:

• Use Delegate.DynamicInvoke, which is just terrible for performance/memory usage. We wanted to have the broadcast method have an amortized allocation cost equal to 0, and also to avoid using reflection completely in this implementation.
• Wrap each input handler in another one with just an object parameter. This would've meant to completely defeat the whole point of structuring delegates this way to allow the C# compiler to cache them (as they're static), as we would've had to allocate a new display class capturing the input delegate every single time one was registered. Also each broadcast would've had twice the number of virtual calls, as each handler would've had to go through its proxy one doing the recipient cast first.

This is something I've actually talked about quite a bit with @333fred and @tannergooding too in the C# Discord server. Given the exact specifications of this type and the way it's implemented, using this trick should be perfectly valid, as all the various constraints and unsafe casts will always be guaranteed to be valid, so there will never be a type violation in doing so. This is really just a workaround for a lack of proper support for basically doing the equivalent of "try to invoke this delegate with these inputs, and just throw if they are not valid". With the exception that here we already know the inputs are valid, so we don't even check.

In an ideal world (as in, with you guys having unlimited time and budget to implement all sorts of proposals even for less requested features), my idea was that DynamicInvoke could be implemented as a JIT intrinsic, with the EE engine emitting the code there to just do safe casts of the inputs and throw otherwise - that way all the reflection would be avoided and the resulting codegen would be almost the same as just Invoke, with the only difference being the checks for the input parameters and boxing of value type parameters, if any, but that'd be perfectly acceptable. This is just a way to achieve that with the current runtime 🚀

On the other hand, as a result of this and a number of other (less unsafe) optimizations this new implementation is both much faster than all other competitor types from other common libraries, and using virtually no memory at all for broadcasts:

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
MvvmLight	600.35 ms	2.121 ms	1.771 ms	1.00	164000.0000	53000.0000	-	696481352 B
Caliburn	354.25 ms	2.090 ms	1.853 ms	0.59	14000.0000	-	-	58976000 B
Calcium	18.94 ms	0.028 ms	0.026 ms	0.03	1843.7500	-	-	7744042 B
MVVM Toolkit	12.47 ms	0.072 ms	0.056 ms	0.02	-	-	-	-

Hope this helps to clarify why the usage of that arguably very niche API in that library in particular 😊

[jkotas]

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.

But the other uses of Unsafe.* in this file look questionable. For example, the indexing using Unsafe.Add without bounds checks in the same function: https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/79127cf6a76ea1b0673c0376e43d41f91b2df509/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs#L384 . It is unlikely to provide significant performance advantage given everything else going on in this function and the correctness of it depends on a lot of other code in DictionarySlim and other types. Use of Unsafe.Add like this would be unlikely to pass codereview in dotnet/runtime.

[Sergio0694]

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.

I'm really glad to hear that! Thank you for taking the time to read through that and glance over the code, I appreciate it 😊
I can agree that's arguably pretty much a hack, so I'm always grateful to have more experienced people go through it.

As for the other point on those bounds checks being removed, I did that after doing some profiling, which seemed to indicate that some amount of time was spent just in those two assignments in the first tight loop in the Send method:

Removing those checks cut the reported CPU time there in half, and resulted in an overall 10% improvement in my benchmarks:

Method	Mean	Error	StdDev	Ratio
WCT2_Checks	13.81 ms	0.016 ms	0.013 ms	1.00
WCT_Unsafe	12.66 ms	0.081 ms	0.072 ms	0.91

My rationale here was that a ~10% overall speedup seemed enough to consider this bounds checks removal worth it in this case. The safety here is guaranteed by the fact that there is a dedicated property (here, with comment) to track the total number of items in each mapping, which is used to retrieve the rented array from the pool. Doing that guarantees that the rented array will always be as large or greater than necessary even in case every existing handler in that loop matches the token and is added to the array.

[jkotas]

In dotnet/runtime, we would likely give up this ~10% speedup to avoid this type of unsafe code.

Here are some ideas where you can save the cycles instead:

• Convert the object array to Span. It will avoid the covariance checks that are likely contributing to the 10%.
• Use just a single object array to store both (e.g. handlers on odd indices and recipients on even indices)

[jkotas]

The safety here is guaranteed by the fact that there is a dedicated property

What if there is a corner case bug in DictionarySlim where the property returns wrong size?

[Sergio0694]

Hey @jkotas - thank you so much for the feedback and suggestions! 😊

I made some changes starting from them and made a new PR that completely removes all Unsafe.Add usages from the entire Microsoft.Toolkit.Mvvm package, and with no effective performance loss at all from what I can see now 🚀

To reply to your previous points:

Here are some ideas where you can save the cycles instead: [...]

Yup, basically ended up applying both those suggestions, I've documented everything in the new PR.

What if there is a corner case bug in DictionarySlim where the property returns wrong size?

Just for sake of clarify, the DictionarySlim we use is not directly from the Microsoft.Experimental.Collections package, but it's a custom type that used the source of that as a base, which I've then customized and optimized specifically for our needs in the messenger types in the MVVM Toolkit. Of course, your point is still perfectly valid and a bug might very well slip into the codebase in that area at some point, so this PR should made the whole thing much more resilient and easier to debug 😄

As a final note, to get back to @michael-hawker's original point, I was wondering if the expected adoption of the MVVM Toolkit would be enough to have this PR (I mean this one for .NET 5) meet the requirements for the .NET 5 release? Otherwise as Stephen said, I can just sprinkle a bunch of ! operators where needed, so it wouldn't be a breaking change, sure.

[jkotas]

I was wondering if the expected adoption of the MVVM Toolkit would be enough to have this PR (I mean this one for .NET 5) meet the requirements for the .NET 5 release

It would not. This issue does not affect users of MVVM Toolkit. It just affects internal implementation details of MVVM Toolkit.

[Sergio0694]

I understand, yeah that's a fair point. Thank you for your time! 😊