Removed unsafe indexing from StrongReferenceMessenger

[Sergio0694]

Follow up to #3424

PR Type

What kind of change does this PR introduce?

• Refactoring (no functional changes, no api changes)

What is the current behavior?

There were a couple of Unsafe.Add usages in the StrongReferenceClass, in particular:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/79127cf6a76ea1b0673c0376e43d41f91b2df509/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs#L384-L385

While these two were working just fine, following the conversation here it was suggested by @jkotas to refactor this to avoid those unsafe indexing operations, to ensure an out of bounds write could never be performed even in case a bug popped up in the previous logic, or in case a future refactor/change introduced one. This would've also made the code a bit less error prone in general. He also offered some suggestions and ideas that I started from to do this refactoring, so thanks! 😄

Changes in this PR

• Added a new internal Object2 type. This is a value type that stores a pair of handler and recipient for a message
• Renting a single object array of twice the size, and interleaving the local handler and recipients at even and off indices.
• Using a Span<object> for all the indexing operations to avoid the array covariance checks.
• This type is now used in the Send methods for both messenger types. Benefits of this change:
  • We only need to rent a single array from the pool. We also then only need to clear and return one.
  • No covariance checks when setting values in the array, as this time we're using a value type indexing a Span<object>.
  • We have bounds checks now, but only 1 and not 2, since each item holds a pair of values.
  • The broadcast part only has a single array to go through now, so we can replace the for with unsafe offsetting with just a ref foreach and still get bounds checks being removed, while having code that's easier to read and to verify.

Final result

Benchmarks

Here's the cherry on top - removing those Unsafe.Add calls while adding this new type and refactoring made it so that instead of that previous ~10% performance hit I saw in my benchmarks, performance now are virtually identical. I run my benchmark a few times and the new version with the changes in this PR ranged from being up to 2% slower, to being 4% faster (!).
If we just say that on average the performance is virtually identical, we still have the advantage of safer code now though 🎉

Method	Mean	Error	StdDev	Ratio
WCT_Unsafe	11.43 ms	0.034 ms	0.030 ms	1.00
WCT_Checked	11.51 ms	0.088 ms	0.078 ms	1.01
WCT_Unsafe	11.87 ms	0.049 ms	0.043 ms	1.00
WCT_Checked	11.44 ms	0.026 ms	0.021 ms	0.96

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

[ghost]

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[jkotas]

I meant something like this:

var pooledArray = ArrayPool<Object>.Shared.Rent(2 * totalHandlersCount);
var pairs = pooledArray.AsSpan(0, 2 * totalHandlersCount);

...

pairs[2 * i] = handler;
pairs[2 * i + 1] = recipient;
i++;

...

for (int j = 0; j < i; j++)
{
     Unsafe.As<MessageHandler<object, TMessage>>(pairs[2 * i])(pairs[2 * i + 1], message);
} 

...

pairs.Clear();
ArrayPool<object>.Shared.Return(pooledArray);

[Sergio0694]

Oh I see now, thanks! I had read your previous comment backwards, now it makes perfect sense 😄
I made the changes in 137c6bd and a99d620, hope that looks good now!

I've re-run the benchmarks and despite the additional bounds checks the performance seems to go from 5-6% faster to 4-5% slower than the original implementation, so on average I'd say we should basically be very close to that, which is perfectly fine!
Also - wow, now I really understand why Fred wishes he could just remove the array covariance feature 🤣
I wasn't aware that was this much slower than just doing bounds checks, I'll keep that in mind for the future!

[jkotas]

LGTM!

[Sergio0694]

Awesome! Thank you for your help Jan, really appreciate it! 😄

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.