From 81c4a4af18da714ce0e76d7eccc5134a0a10af29 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 1 Oct 2020 13:48:04 +0200 Subject: [PATCH 1/4] Removed unsafe indexing from StrongReferenceMessenger --- .../Messaging/Internals/Object2.cs | 41 +++++++++++++++++++ .../Messaging/StrongReferenceMessenger.cs | 41 ++++++++----------- 2 files changed, 58 insertions(+), 24 deletions(-) create mode 100644 Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs diff --git a/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs b/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs new file mode 100644 index 00000000000..230b8fed7cb --- /dev/null +++ b/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs @@ -0,0 +1,41 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +namespace Microsoft.Toolkit.Mvvm.Messaging.Internals +{ + /// + /// A simple type representing an immutable pair of objects. + /// + /// + /// This type replaces a simple when specifically dealing with a + /// pair of handler and recipient. Unlike , this type is readonly. + /// + internal readonly struct Object2 + { + /// + /// Initializes a new instance of the struct. + /// + /// The input handler. + /// The input recipient. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public Object2(object handler, object recipient) + { + Handler = handler; + Recipient = recipient; + } + + /// + /// The current handler (which is actually some instance. + /// + public readonly object Handler; + + /// + /// The current recipient (guaranteed to match the type constraints for . + /// + public readonly object Recipient; + } +} diff --git a/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs b/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs index 5f4d5dccdec..40e59100419 100644 --- a/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs +++ b/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs @@ -325,14 +325,11 @@ public void Unregister(object recipient, TToken token) } /// - public unsafe TMessage Send(TMessage message, TToken token) + public TMessage Send(TMessage message, TToken token) where TMessage : class where TToken : IEquatable { - object[] handlers; - object[] recipients; - ref object handlersRef = ref Unsafe.AsRef(null); - ref object recipientsRef = ref Unsafe.AsRef(null); + Object2[] pairs; int i = 0; lock (this.recipientsMap) @@ -358,10 +355,7 @@ public unsafe TMessage Send(TMessage message, TToken token) return message; } - handlers = ArrayPool.Shared.Rent(totalHandlersCount); - recipients = ArrayPool.Shared.Rent(totalHandlersCount); - handlersRef = ref handlers[0]; - recipientsRef = ref recipients[0]; + pairs = ArrayPool.Shared.Rent(totalHandlersCount); // Copy the handlers to the local collection. // The array is oversized at this point, since it also includes @@ -379,40 +373,39 @@ public unsafe TMessage Send(TMessage message, TToken token) // Pick the target handler, if the token is a match for the recipient if (mappingEnumerator.Value.TryGetValue(token, out object? handler)) { - // We can manually offset here to skip the bounds checks in this inner loop when - // indexing the array (the size is already verified and guaranteed to be enough). - Unsafe.Add(ref handlersRef, (IntPtr)(void*)(uint)i) = handler!; - Unsafe.Add(ref recipientsRef, (IntPtr)(void*)(uint)i++) = recipient; + // This array access should always guaranteed to be valid due to the size of the + // array being set according to the current total number of registered handlers, + // which will always be greater or equal than the ones matching the previous test. + // We're still using a checked array access here though to make sure an out of + // bounds write can never happen even if an error was present in the logic above. + pairs[i++] = new Object2(handler!, recipient); } } } + // The rented array is often larger than the number of matching pairs + // to process, so we first slice the span with just the items we need. + Span pendingPairs = pairs.AsSpan(0, i); + try { // Invoke all the necessary handlers on the local copy of entries - for (int j = 0; j < i; j++) + foreach (ref Object2 pair in pendingPairs) { - // We're doing an unsafe cast to skip the type checks again. - // See the comments in the UnregisterAll method for more info. - object handler = Unsafe.Add(ref handlersRef, (IntPtr)(void*)(uint)j); - object recipient = Unsafe.Add(ref recipientsRef, (IntPtr)(void*)(uint)j); - // 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>(handler)(recipient, message); + Unsafe.As>(pair.Handler)(pair.Recipient, message); } } finally { // As before, we also need to clear it first to avoid having potentially long // lasting memory leaks due to leftover references being stored in the pool. - handlers.AsSpan(0, i).Clear(); - recipients.AsSpan(0, i).Clear(); + pendingPairs.Clear(); - ArrayPool.Shared.Return(handlers); - ArrayPool.Shared.Return(recipients); + ArrayPool.Shared.Return(pairs); } return message; From 0bc0648624fc277702cae8e399bb3f3d440f3e1b Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 1 Oct 2020 14:53:17 +0200 Subject: [PATCH 2/4] Code refactoring to WeakReferenceMessenger --- .../Messaging/WeakReferenceMessenger.cs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs b/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs index 72b1683f577..558b945f4b6 100644 --- a/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs +++ b/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs @@ -169,8 +169,7 @@ public TMessage Send(TMessage message, TToken token) where TMessage : class where TToken : IEquatable { - ArrayPoolBufferWriter recipients; - ArrayPoolBufferWriter handlers; + ArrayPoolBufferWriter pairs; lock (this.recipientsMap) { @@ -182,8 +181,7 @@ public TMessage Send(TMessage message, TToken token) return message; } - recipients = ArrayPoolBufferWriter.Create(); - handlers = ArrayPoolBufferWriter.Create(); + pairs = ArrayPoolBufferWriter.Create(); // We need a local, temporary copy of all the pending recipients and handlers to // invoke, to avoid issues with handlers unregistering from messages while we're @@ -197,32 +195,26 @@ public TMessage Send(TMessage message, TToken token) if (map.TryGetValue(token, out object? handler)) { - recipients.Add(pair.Key); - handlers.Add(handler!); + pairs.Add(new Object2(handler!, pair.Key)); } } } try { - ReadOnlySpan - recipientsSpan = recipients.Span, - handlersSpan = handlers.Span; - - for (int i = 0; i < recipientsSpan.Length; i++) + foreach (ref readonly Object2 pair in pairs.Span) { // Just like in the other messenger, here we need an unsafe cast to be able to // invoke a generic delegate with a contravariant input argument, with a less // derived reference, without reflection. This is guaranteed to work by how the // messenger tracks registered recipients and their associated handlers, so the // type conversion will always be valid (the recipients are the rigth instances). - Unsafe.As>(handlersSpan[i])(recipientsSpan[i], message); + Unsafe.As>(pair.Handler)(pair.Recipient, message); } } finally { - recipients.Dispose(); - handlers.Dispose(); + pairs.Dispose(); } return message; From 137c6bddaea7477dd3b4bf2858b6bc988ed1466c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 1 Oct 2020 19:14:58 +0200 Subject: [PATCH 3/4] Code refactoring to StrongReferenceMessenger --- .../Messaging/StrongReferenceMessenger.cs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs b/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs index 40e59100419..a8e45b15c2e 100644 --- a/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs +++ b/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs @@ -329,7 +329,8 @@ public TMessage Send(TMessage message, TToken token) where TMessage : class where TToken : IEquatable { - Object2[] pairs; + object[] rentedArray; + Span pairs; int i = 0; lock (this.recipientsMap) @@ -355,7 +356,9 @@ public TMessage Send(TMessage message, TToken token) return message; } - pairs = ArrayPool.Shared.Rent(totalHandlersCount); + // Rent the array and also assign it to a span, which will be used to access values. + // We're doing this to avoid the array covariance checks slowdown in the loops below. + pairs = rentedArray = ArrayPool.Shared.Rent(2 * totalHandlersCount); // Copy the handlers to the local collection. // The array is oversized at this point, since it also includes @@ -373,39 +376,37 @@ public TMessage Send(TMessage message, TToken token) // Pick the target handler, if the token is a match for the recipient if (mappingEnumerator.Value.TryGetValue(token, out object? handler)) { - // This array access should always guaranteed to be valid due to the size of the + // This span access should always guaranteed to be valid due to the size of the // array being set according to the current total number of registered handlers, // which will always be greater or equal than the ones matching the previous test. - // We're still using a checked array access here though to make sure an out of + // We're still using a checked span accesses here though to make sure an out of // bounds write can never happen even if an error was present in the logic above. - pairs[i++] = new Object2(handler!, recipient); + pairs[2 * i] = handler!; + pairs[(2 * i) + 1] = recipient; + i++; } } } - // The rented array is often larger than the number of matching pairs - // to process, so we first slice the span with just the items we need. - Span pendingPairs = pairs.AsSpan(0, i); - try { // Invoke all the necessary handlers on the local copy of entries - foreach (ref Object2 pair in pendingPairs) + for (int j = 0; j < i; j++) { // 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>(pair.Handler)(pair.Recipient, message); + Unsafe.As>(pairs[2 * j])(pairs[(2 * j) + 1], message); } } finally { // As before, we also need to clear it first to avoid having potentially long // lasting memory leaks due to leftover references being stored in the pool. - pendingPairs.Clear(); + Array.Clear(rentedArray, 0, 2 * i); - ArrayPool.Shared.Return(pairs); + ArrayPool.Shared.Return(rentedArray); } return message; From a99d6207b48627f6f5a7bc5c2ead5d862171991f Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 1 Oct 2020 19:15:05 +0200 Subject: [PATCH 4/4] Code refactoring to WeakReferenceMessenger --- .../Messaging/Internals/Object2.cs | 41 ------------------- .../Messaging/WeakReferenceMessenger.cs | 17 +++++--- 2 files changed, 11 insertions(+), 47 deletions(-) delete mode 100644 Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs diff --git a/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs b/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs deleted file mode 100644 index 230b8fed7cb..00000000000 --- a/Microsoft.Toolkit.Mvvm/Messaging/Internals/Object2.cs +++ /dev/null @@ -1,41 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.Runtime.CompilerServices; - -namespace Microsoft.Toolkit.Mvvm.Messaging.Internals -{ - /// - /// A simple type representing an immutable pair of objects. - /// - /// - /// This type replaces a simple when specifically dealing with a - /// pair of handler and recipient. Unlike , this type is readonly. - /// - internal readonly struct Object2 - { - /// - /// Initializes a new instance of the struct. - /// - /// The input handler. - /// The input recipient. - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public Object2(object handler, object recipient) - { - Handler = handler; - Recipient = recipient; - } - - /// - /// The current handler (which is actually some instance. - /// - public readonly object Handler; - - /// - /// The current recipient (guaranteed to match the type constraints for . - /// - public readonly object Recipient; - } -} diff --git a/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs b/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs index 558b945f4b6..d17e332ba5d 100644 --- a/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs +++ b/Microsoft.Toolkit.Mvvm/Messaging/WeakReferenceMessenger.cs @@ -169,7 +169,8 @@ public TMessage Send(TMessage message, TToken token) where TMessage : class where TToken : IEquatable { - ArrayPoolBufferWriter pairs; + ArrayPoolBufferWriter bufferWriter; + int i = 0; lock (this.recipientsMap) { @@ -181,7 +182,7 @@ public TMessage Send(TMessage message, TToken token) return message; } - pairs = ArrayPoolBufferWriter.Create(); + bufferWriter = ArrayPoolBufferWriter.Create(); // We need a local, temporary copy of all the pending recipients and handlers to // invoke, to avoid issues with handlers unregistering from messages while we're @@ -195,26 +196,30 @@ public TMessage Send(TMessage message, TToken token) if (map.TryGetValue(token, out object? handler)) { - pairs.Add(new Object2(handler!, pair.Key)); + bufferWriter.Add(handler!); + bufferWriter.Add(pair.Key); + i++; } } } try { - foreach (ref readonly Object2 pair in pairs.Span) + ReadOnlySpan pairs = bufferWriter.Span; + + for (int j = 0; j < i; j++) { // Just like in the other messenger, here we need an unsafe cast to be able to // invoke a generic delegate with a contravariant input argument, with a less // derived reference, without reflection. This is guaranteed to work by how the // messenger tracks registered recipients and their associated handlers, so the // type conversion will always be valid (the recipients are the rigth instances). - Unsafe.As>(pair.Handler)(pair.Recipient, message); + Unsafe.As>(pairs[2 * j])(pairs[(2 * j) + 1], message); } } finally { - pairs.Dispose(); + bufferWriter.Dispose(); } return message;