Improvements for Delegate.GetHashCode() for CsWinRT (and NativeAOT)

[Sergio0694]

Overview

We're working on improving the binary size of assemblies using CsWinRT and NativeAOT, and talking with @MichalStrehovsky we noticed that the new trimming optimization to stop delegate targets from being considered reflection-visible by default (#96166) is not kicking in at all, as it's blocked by the use of Delegate.Method in our EventRegistrationTokenTable<T> type (see here).

We need to calculate a "good" pseudo-random value for a given delegate, which will be the starting value for an event registration token passed to the WinRT side. Currently we're using Delegate.GetHashCode() only when the delegate is multicast, because unfortunately when that is not the case, the returned hashcode only depends on the target of the delegate, and on the delegate type, but not on the specific method being wrapped by the delegate:

runtime/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

Lines 151 to 154 in 46d3b27

	if (_methodPtrAux == IntPtr.Zero)
	return (_target != null ? RuntimeHelpers.GetHashCode(_target) * 33 : 0) + GetType().GetHashCode();
	else
	return GetType().GetHashCode();

The way we're working around that is to check if the delegate is unicast, and in that case do handler.Method.GetHashCode(), which breaks that size optimization entirely. I can't really think of another way of getting some other value from the delegate without touching Method at all. Additionally, Michal also mentioned that the GetHashCode() implementation on NativeAOT could be improved.

Dropping that Method property entirely saves over 200 KB in a minimal WinRT component sample:

I don't have an exact ask here, so opening this more to try to figure out what the best approach to this could be. For instance, would it be possible to change the Delegate.GetHashCode() implementation for unicast delegates to also account for the method being invoked? Or, if that was a nonstarter due to the overhead it would add to all uses (even when callers would be fine with the current behavior), could perhaps a new API make sense to compute a "better" hashcode for a delegate? Strawman idea:

namespace System.Runtime.CompilerServices;

public static class RuntimeHelpers
{
    public static int GetDelegateHashCode(Delegate? d);
}

This could compute a "better", slightly more expensive hashcode, that would combine all target methods as well.
Alternatively, could there be some other way of retrieving a "hashcode of the target method" that's trim friendly?

To also try things from another angle — @AaronRobinsonMSFT @jkoritzinsky I know you have experience in this area specifically (I mean WinRT event handling in general). Would you know whether it might be possible to actually just stop this approach entirely and not just, for instance, use a completely random value for the lower 32 bits instead? Reading all the comments in CsWinRT it's not really clear why that's not already being done. The comments say the current approach "will also give us a shot at preventing unregistration rom becoming an O(N) operation", but given the unregistration is just performed with a token that's coming from outside this class (ie. the one that callers would've stored when registering), it's not clear to me at all how this would relate to how that initial token value was calculated, and especially to whether it was a pseudo-random value depending on the exact delegate instance or not.

To clarify: the goal here is to drop that Method use in CsWinRT to allow the optimization in #96166 to work. I'd be happy with any approach that would allow that, be it some new API, some other equivalent workaround, or perhaps just a different logic to compute that token entirely from CsWinRT's side, assuming that didn't cause other problems or performance regressions.

Also cc. @manodasanW

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Overview

We're working on improving the binary size of apps/libraries using CsWinRT and NativeAOT, and talking with @MichalStrehovsky we noticed that the new trimming optimization to stop delegate targets from being considered reflection-visible by default (#96166) is not kicking in at all, as it's blocked by the use of Delegate.Method in our EventRegistrationTokenTable<T> type (see here).

We need to calculate a "good" pseudo-random value for a given delegate, which will be the starting value for an event registration token passed to the WinRT side. Currently we're using Delegate.GetHashCode() only when the delegate is multicast, because unfortunately when that is not the case, the returned hashcode only depends on the target of the delegate, and on the delegate type, but not on the specific method being wrapped by the delegate:

runtime/src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

Lines 151 to 154 in 46d3b27

	if (_methodPtrAux == IntPtr.Zero)
	return (_target != null ? RuntimeHelpers.GetHashCode(_target) * 33 : 0) + GetType().GetHashCode();
	else
	return GetType().GetHashCode();

The way we're working around that is to check if the delegate is unicast, and in that case do handler.Method.GetHashCode(), which breaks that size optimization entirely. I can't really think of another way of getting some other value from the delegate without touching Method at all. Additionally, Michal also mentioned that the `GetHashCode() implementation on NativeAOT could be improved.

Dropping that Method property entirely saves over 200 KB in a minimal WinRT component sample:

I don't have an exact ask here, so opening this more to try to figure out what the best approach to this could be. For instance, would it be possible to change the Delegate.GetHashCode() implementation for unicast delegates to also account for the method being invoked? Or, if that was a nonstarter due to the overhead it would add to all uses (even when callers would be fine with the current behavior), could perhaps a new API make sense to compute a "better" hashcode for a delegate? Strawman idea:

namespace System.Runtime.CompilerServices;

public static class RuntimeHelpers
{
    public static int GetDelegateHashCode(Delegate? d);
}

This could compute a "better", slightly more expensive hashcode, that would combine all target methods as well.
Alternatively, could there be some other way of retrieving a "hashcode of the target method" that's trim friendly?

To also try things from another angle — @AaronRobinsonMSFT @jkoritzinsky I know you have experience in this area specifically (I mean WinRT event handling in general). Would you know whether it might be possible to actually just stop this approach entirely and not just, for instance, use a completely random value for the lower 32 bits instead? Reading all the comments in CsWinRT it's not really clear why that's not already being done. The comments say the current approach "will also give us a shot at preventing unregistration rom becoming an O(N) operation", but given the unregistration is just performed with a token that's coming from outside this class (ie. the one that callers would've stored when registering), it's not clear to me at all how this would relate to how that initial token value was calculated, and especially to whether it was a pseudo-random value depending on the exact delegate instance or not.

To clarify: the goal here is to drop that Method use in CsWinRT to allow the optimization in #96166 to work. I'd be happy with any approach that would allow that, be it some new API, some other equivalent workaround, or perhaps just a different logic to compute that token entirely from CsWinRT's side, assuming that didn't cause other problems or performance regressions.

Also cc. @manodasanW

Author:	Sergio0694
Assignees:	-
Labels:	area-System.Runtime.CompilerServices, area-NativeAOT-coreclr
Milestone:	-

[AaronRobinsonMSFT]

Dropping that Method property entirely saves over 200 KB in a minimal WinRT component sample:

Is this a fixed cost or does it grow with size of application? Put another way, how does the 200 KB grow relative to application size.

would it be possible to change the Delegate.GetHashCode() implementation for unicast delegates to also account for the method being invoked?

I'd be leary of this change simply due to the fact that I could easily construct a scenario where users have taken an indirect dependency on Delegate.GetHashCode() being stable for type and agnostic of the target method.

could perhaps a new API make sense to compute a "better" hashcode for a delegate?

I think calling this "hash code" is something we should avoid. Something akin to GetTargetAwareID(), but that is also clunky.

Would you know whether it might be possible to actually just stop this approach entirely and not just, for instance, use a completely random value for the lower 32 bits instead?

Honestly, I don't recall. This is a question best for @Scottj1s or @jkoritzinsky.

[Sergio0694]

"Is this a fixed cost or does it grow with size of application?"

The cost is not fixed, but it will grow linearly with the number of delegates being used (as in, including with different instances of the same delegate types, pointing to different methods). The fact CsWinRT touches that .Method property on an arbitrary Delegate object makes NativeAOT consider all delegate targets in the entire application to be considered reflection visible. Worth noting that this also means that the return type of those methods, as well as the type of all parameters of those methods will then also be considered to be reflection visible, etc. So especially with UI applications, which normally have particularly large amounts of delegates all over the place (eg. for event handlers, commands, bindings, etc.) this can add up.

"I'd be leary of this change simply due to the fact that I could easily construct a scenario where users have taken an indirect dependency on Delegate.GetHashCode() being stable for type and agnostic of the target method."

Yeah that makes sense. A separate API seems like a good solution there, plus it makes this completely pay-for-play.

"I think calling this "hash code" is something we should avoid. Something akin to GetTargetAwareID(), but that is also clunky."

Just spitballing here:

• GetTargetAwareHashCode() (yes I know you said to avoid "hash code", but this feels kinda accurate)
• GetTargetAwareIdentifierToken()
• GetDelegateIdentifierToken()

I kinda like GetDelegateIdentifierToken(). It's self describing, and it does say it's just "some identifier token" 🤔

[jkotas]

it's blocked by the use of Delegate.Method in our EventRegistrationTokenTable type (see here).

I do not understand why this needs to be this complicated. What is not going to work well if this method is replaced by a random number?

private static EventRegistrationToken GetPreferredToken(T handler)
   => new EventRegistrationToken { Value = (long)Random.Shared.NextInt64() };

BTW: This has a redundant call of m_tokens.TryGetValue.

[Sergio0694]

"I do not understand why this needs to be this complicated. What is not going to work well if this method is replaced by a random number?"

That is not entirely clear to me either, and part of why I opened this issue hoping someone with more experience in this area in particular could shed some light on this. We found some old branch in the built-in WinRT support that was just using Delegate.GetHashCode() for all cases (this commit, from @AaronRobinsonMSFT), but it was apparently never merged, which makes me think there was a specific reason why we currently have this logic with the special hash code of the delegate target. It's just not really clear to me why that's the case and what issues, if any, we would have with a more random token 😅

I did think about using Random.Shared.NextInt64(), though I was also thinking if we were to really go that route (as in, using random numbers), we might just use (long)typeof(T).GetHashCode() << 32 | Random.Shared.Next(), so that the upper 32 bits would still identify the type of delegate being used, which the comment says can be useful when investigating crash dumps containing corrupt tokens, as at least it allows identifying what type of handler was being used there 🤔

"This has a redundant call of m_tokens.TryGetValue."

Yup, noticed that too yesterday! Will open a PR to drop that. Was thinking we can actually skip the second lookup entirely on .NET 6+ and just use a single Remove call with the overload that also returns the removed value in case of success. Thank you!

EDIT: opened microsoft/CsWinRT#1445 to fix that.

[jkotas]

Do we know about any issues that were diagnosed thanks to the current encoding scheme? I understand that encoding the delegate target breadcrumbs can help in theory, but it is still fairly non-trivial to use this breadcrumb to diagnose issues with dangling or corrupted tokens.

(long)typeof(T).GetHashCode() << 32 | Random.Shared.Next(), so that the upper 32 bits would still identify the type of delegate being used

I think it should be good enough., for now at least. Instead of Random.Shared.Next(), it may be better to just keep incrementing an integer, starting with some unique value (e.g. 0x42424242). If you run into a dangling token, the integer value can give you some idea about when the token was created and whether it is likely to be valid.

A few more ideas for optimizations:

• typeof(T).GetHashCode() can be cached
• The dictionary key can be int32 (low 32-bits of the token) instead of int64

[Sergio0694]

Opened microsoft/CsWinRT#1448 integrating all of these suggestions 🙂

Still waiting on confirmation from WinRT folks (or Jeremy) that there's not some magic/cursed/magic reason why we actually did specifically need that logic for the preferred token to depend on the delegate target, but hopefully that's not the case. Plus, at least we have an actual PR to leave comments on now, which should help 😄

[Sergio0694]

Alright special shoutout to @jkoritzinsky for helping to look through all the old sources for the built-in WinRT interop and figuring out what was going on and why we had that infrastructure with those arguably weird comments. Here's what happened. The original built-in WinRT interop support was using EventRegistrationTokenTable<T> for both events in RCW and CCW objects. This meant that in the RCW case, as the original comment said:

"We will frequently be called upon to look up a token value given only a delegate to start from.
[...]
"While calculating this initial value will be somewhat more expensive than just using a counter for events that have few registrations, it will also give us a shot at preventing unregistration from becoming an O(N) operation."

This comment only made sense in that scenario, where they also had APIs such as RemoveEventHandler(T handler). In that case, yes, getting an initial good "guess" for the token based on the input delegate did make perfect sense, as it could greatly reduce the number of operations being performed to actually find the token that was associated with that delegate.

When support for WinRT was moved out of the runtime, events for RCW objects moved to a different mechanism, and EventRegistrationTokenTable<T>, just like today, ended up only being used for the CCW scenario. But in that case, you would never actually have to lookup a value given a handler. Rather, you'd only ever be asked to remove a handler and retrieve it given a token. So all of those comments just did not apply anymore, and they were just accidentally left there after the code they were actually referring to got deleted. There's no longer any concerns about "O(n) operations" using just an incremental counter.

So to recap, we can totally do microsoft/CsWinRT#1448 using the suggestions from Jan, and changing the logic there is completely safe now. I'm just going to close this issue then, since there's no actual API needed anymore 😄

Thanks everyone!