From ef926a25b68b76b010fecd545de38efbf30c5ee1 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Mon, 25 Nov 2024 19:46:42 -0500 Subject: [PATCH 1/3] chore: Adjust lock instance --- .../UI/Xaml/DependencyObjectCollection.cs | 56 ++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs index 70003d443970..84feb290b784 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs @@ -1,4 +1,5 @@ using System; +using System.Buffers; using System.Collections; using System.Collections.Generic; using System.Diagnostics; @@ -25,7 +26,29 @@ public partial class DependencyObjectCollectionBase : DependencyObject public partial class DependencyObjectCollection : DependencyObjectCollectionBase, IList, IEnumerable, IEnumerable, IObservableVector where T : DependencyObject { - public event VectorChangedEventHandler VectorChanged; + private object _vectorChangedHandlersLock = new(); + + // Explicit handlers list to avoid the cost of multicast delegates handling + private List> _vectorChangedHandlers; + + public event VectorChangedEventHandler VectorChanged + { + add + { + lock (_vectorChangedHandlersLock) + { + (_vectorChangedHandlers ??= new()).Add(value); + } + } + + remove + { + lock (_vectorChangedHandlersLock) + { + (_vectorChangedHandlers ??= new()).Remove(value); + } + } + } private readonly List _list = new List(); @@ -205,7 +228,36 @@ internal List.Enumerator GetEnumeratorFast() => _list.GetEnumerator(); private void RaiseVectorChanged(CollectionChange change, int index) - => VectorChanged?.Invoke(this, new VectorChangedEventArgs(change, (uint)index)); + { + lock (_vectorChangedHandlersLock) + { + if (_vectorChangedHandlers is { Count: > 0 }) + { + var args = new VectorChangedEventArgs(change, (uint)index); + + if (_vectorChangedHandlers.Count == 1) + { + _vectorChangedHandlers[0].Invoke(this, args); + } + else + { + // Clone the array to account for reentrancy. + var handlersClone = ArrayPool>.Shared.Rent(_vectorChangedHandlers.Count); + _vectorChangedHandlers.CopyTo(handlersClone, 0); + + // Use the original count, the rented array may be larger. + var count = _vectorChangedHandlers.Count; + + for (int i = 0; i < count; i++) + { + handlersClone[i].Invoke(this, args); + } + + ArrayPool>.Shared.Return(handlersClone); + } + } + } + } private protected virtual void OnAdded(T d) { From 263a27838a8e02196bb1e9c0232dd20297698ac9 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Tue, 26 Nov 2024 09:03:39 -0500 Subject: [PATCH 2/3] chore: Adjust remove order, don't lock on execution, clear pooled array memory --- .../Given_DependencyObjectCollection.cs | 31 ++++++++ .../UI/Xaml/DependencyObjectCollection.cs | 72 ++++++++++++++----- 2 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_DependencyObjectCollection.cs diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_DependencyObjectCollection.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_DependencyObjectCollection.cs new file mode 100644 index 000000000000..2ea54933aa30 --- /dev/null +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_DependencyObjectCollection.cs @@ -0,0 +1,31 @@ +using System.Collections.Generic; +using System.Linq; +using System.Runtime.InteropServices.ObjectiveC; +using Microsoft.UI.Xaml; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Uno.UI.RuntimeTests.Tests.Windows_UI_Xaml; + +[TestClass] +public class Given_DependencyObjectCollection +{ + [TestMethod] + public void When_Add_Multiple_And_Invoke() + { + DependencyObjectCollection c = new(); + + List list = []; + + void One(object sender, object args) => list.Add("One"); + void Two(object sender, object args) => list.Add("Two"); + + c.VectorChanged += One; + c.VectorChanged += Two; + c.VectorChanged += One; + c.VectorChanged -= One; + + c.Add(c); + + Assert.IsTrue(list.SequenceEqual(["One", "Two"])); + } +} diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs index 84feb290b784..032f7c9d993c 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs @@ -45,7 +45,14 @@ public event VectorChangedEventHandler VectorChanged { lock (_vectorChangedHandlersLock) { - (_vectorChangedHandlers ??= new()).Remove(value); + var list = _vectorChangedHandlers ??= new(); + + var lastIndex = list.LastIndexOf(value); + + if (lastIndex != -1) + { + list.RemoveAt(lastIndex); + } } } } @@ -229,32 +236,59 @@ internal List.Enumerator GetEnumeratorFast() private void RaiseVectorChanged(CollectionChange change, int index) { - lock (_vectorChangedHandlersLock) + // Gets an executable list that does not need to be locked + int GetInvocationList(out VectorChangedEventHandler single, out VectorChangedEventHandler[] array) { - if (_vectorChangedHandlers is { Count: > 0 }) + lock (_vectorChangedHandlersLock) { - var args = new VectorChangedEventArgs(change, (uint)index); - - if (_vectorChangedHandlers.Count == 1) + if (_vectorChangedHandlers is { Count: > 0 }) { - _vectorChangedHandlers[0].Invoke(this, args); - } - else - { - // Clone the array to account for reentrancy. - var handlersClone = ArrayPool>.Shared.Rent(_vectorChangedHandlers.Count); - _vectorChangedHandlers.CopyTo(handlersClone, 0); + if (_vectorChangedHandlers.Count == 1) + { + single = _vectorChangedHandlers[0]; + array = null; + return 1; + } + else + { + single = null; - // Use the original count, the rented array may be larger. - var count = _vectorChangedHandlers.Count; + array = ArrayPool>.Shared.Rent(_vectorChangedHandlers.Count); + _vectorChangedHandlers.CopyTo(array, 0); - for (int i = 0; i < count; i++) - { - handlersClone[i].Invoke(this, args); + return _vectorChangedHandlers.Count; } + } + } + + single = null; + array = null; + return 0; + } + + var count = GetInvocationList(out var single, out var array); + + if (count > 0) + { + var args = new VectorChangedEventArgs(change, (uint)index); - ArrayPool>.Shared.Return(handlersClone); + if (count == 1) + { + single.Invoke(this, args); + } + else + { + for (int i = 0; i < count; i++) + { + ref var handler = ref array[i]; + handler.Invoke(this, args); + + // Clear the handle immediately, so we don't + // call ArrayPool.Return with clear. + handler = null; } + + ArrayPool>.Shared.Return(array); } } } From 7c41e285944556dffc5c140a828d7d957db4c738 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Tue, 26 Nov 2024 09:04:35 -0500 Subject: [PATCH 3/3] chore: Adjust comment --- src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs index 032f7c9d993c..976cef393567 100644 --- a/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs +++ b/src/Uno.UI/UI/Xaml/DependencyObjectCollection.cs @@ -28,7 +28,8 @@ public partial class DependencyObjectCollection : DependencyObjectCollectionB { private object _vectorChangedHandlersLock = new(); - // Explicit handlers list to avoid the cost of multicast delegates handling + // Explicit handlers list to avoid the cost of generic multicast + // delegates handling on mono's AOT. private List> _vectorChangedHandlers; public event VectorChangedEventHandler VectorChanged