From 679b4bff3306cfd7c52f89a3bf3d0287a64ae188 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Fri, 22 Nov 2024 16:11:05 -0500 Subject: [PATCH 01/14] perf(brush): Don't use reflection to invoke brush updates --- .../Helpers/WeakEvents/WeakEventManager.cs | 142 ------------------ src/Uno.UI/UI/Xaml/Controls/Border/Border.cs | 5 +- .../Border/BorderLayerRenderer.Android.cs | 7 +- .../Border/BorderLayerRenderer.iOSmacOS.cs | 18 +-- .../Border/BorderLayerRenderer.wasm.cs | 10 +- .../UI/Xaml/Controls/TextBlock/TextBlock.cs | 5 +- .../Xaml/Controls/TextBlock/TextBlock.skia.cs | 5 +- .../TextBox/MultilineTextBoxView.iOS.cs | 4 +- .../TextBox/SinglelineTextBoxView.iOS.cs | 4 +- .../UI/Xaml/Controls/TextBox/TextBox.cs | 9 +- .../Controls/TextBox/TextBoxView.Android.cs | 3 +- .../Controls/TextBox/TextBoxView.macOS.cs | 3 +- .../Xaml/Controls/TextBox/TextBoxView.wasm.cs | 4 +- ...IFrameworkElementImplementation.Android.tt | 6 +- .../IFrameworkElementImplementation.iOS.tt | 6 +- .../IFrameworkElementImplementation.macOS.tt | 5 +- src/Uno.UI/UI/Xaml/Media/Brush.cs | 36 +++-- src/Uno.UI/UI/Xaml/Shapes/Shape.cs | 8 +- src/Uno.UI/Uno.UI.netcoremobile.csproj | 11 +- 19 files changed, 93 insertions(+), 198 deletions(-) delete mode 100644 src/Uno.UI/Helpers/WeakEvents/WeakEventManager.cs diff --git a/src/Uno.UI/Helpers/WeakEvents/WeakEventManager.cs b/src/Uno.UI/Helpers/WeakEvents/WeakEventManager.cs deleted file mode 100644 index 0cd4d354cf4b..000000000000 --- a/src/Uno.UI/Helpers/WeakEvents/WeakEventManager.cs +++ /dev/null @@ -1,142 +0,0 @@ -#nullable enable - -using System; -using System.Buffers; -using System.Collections.Generic; -using System.Diagnostics; -using System.Reflection; -using System.Runtime.CompilerServices; - -// Mostly from https://github.com/dotnet/maui/blob/c92d44c68fe81c57ef8caec2506e6788309b8ff4/src/Core/src/WeakEventManager.cs -// But adjusted a little bit - -namespace Uno.UI.Helpers -{ - internal sealed class WeakEventManager - { - private readonly Dictionary> _eventHandlers = new(StringComparer.Ordinal); - - public void AddEventHandler(Action? handler, [CallerMemberName] string eventName = "") - { - if (string.IsNullOrEmpty(eventName)) - throw new ArgumentNullException(nameof(eventName)); - - if (handler == null) - throw new ArgumentNullException(nameof(handler)); - - AddEventHandler(eventName, handler.Target, handler.GetMethodInfo()); - } - - public void HandleEvent(string eventName) - { - if (_eventHandlers.TryGetValue(eventName, out List? target)) - { - // clone the target array just in case one of the subscriptions calls RemoveEventHandler - var targetClone = ArrayPool.Shared.Rent(target.Count); - target.CopyTo(targetClone, 0); - var count = target.Count; - for (int i = 0; i < count; i++) - { - Subscription subscription = targetClone[i]; - bool isStatic = subscription.Subscriber == null; - if (isStatic) - { - // For a static method, we'll just pass null as the first parameter of MethodInfo.Invoke - subscription.Handler.Invoke(null, null); - continue; - } - - object? subscriber = subscription.Subscriber?.Target; - - if (subscriber == null) - { - // The subscriber was collected, so there's no need to keep this subscription around - target.Remove(subscription); - } - else - { - subscription.Handler.Invoke(subscriber, null); - } - } - - ArrayPool.Shared.Return(targetClone); - } - } - - public void RemoveEventHandler(Action? handler, [CallerMemberName] string eventName = "") - { - if (string.IsNullOrEmpty(eventName)) - throw new ArgumentNullException(nameof(eventName)); - - if (handler == null) - throw new ArgumentNullException(nameof(handler)); - - RemoveEventHandler(eventName, handler.Target, handler.GetMethodInfo()); - } - - private void AddEventHandler(string eventName, object? handlerTarget, MethodInfo methodInfo) - { - if (!_eventHandlers.TryGetValue(eventName, out List? targets)) - { - targets = new List(); - _eventHandlers.Add(eventName, targets); - } - else - { - targets.RemoveAll(subscription => subscription.Subscriber is { IsAlive: false }); - } - - if (handlerTarget == null) - { - // This event handler is a static method - targets.Add(new Subscription(null, methodInfo)); - return; - } - - targets.Add(new Subscription(new WeakReference(handlerTarget), methodInfo)); - } - - private void RemoveEventHandler(string eventName, object? handlerTarget, MethodInfo methodInfo) - { - if (!_eventHandlers.TryGetValue(eventName, out List? subscriptions)) - return; - - for (int n = subscriptions.Count - 1; n >= 0; n--) - { - Subscription current = subscriptions[n]; - - if (current.Subscriber != null && !current.Subscriber.IsAlive) - { - // If not alive, remove and continue - subscriptions.RemoveAt(n); - continue; - } - - if (current.Subscriber?.Target == handlerTarget && current.Handler == methodInfo) - { - // Found the match, we can break - subscriptions.RemoveAt(n); - break; - } - } - } - - private readonly struct Subscription : IEquatable - { - public Subscription(WeakReference? subscriber, MethodInfo handler) - { - Subscriber = subscriber; - Handler = handler ?? throw new ArgumentNullException(nameof(handler)); - } - - public readonly WeakReference? Subscriber; - public readonly MethodInfo Handler; - - public bool Equals(Subscription other) => Subscriber == other.Subscriber && Handler == other.Handler; - - public override bool Equals(object? obj) => obj is Subscription other && Equals(other); - - public override int GetHashCode() => Subscriber?.GetHashCode() ?? 0 ^ Handler.GetHashCode(); - } - } -} diff --git a/src/Uno.UI/UI/Xaml/Controls/Border/Border.cs b/src/Uno.UI/UI/Xaml/Controls/Border/Border.cs index db368da9ffb4..a6b8ad9e339b 100644 --- a/src/Uno.UI/UI/Xaml/Controls/Border/Border.cs +++ b/src/Uno.UI/UI/Xaml/Controls/Border/Border.cs @@ -275,6 +275,7 @@ private void OnBorderThicknessChanged(Thickness oldValue, Thickness newValue) #if !__SKIA__ private Action _borderBrushChanged; + private IDisposable _brushChangedSubscription; #endif #if __ANDROID__ @@ -305,7 +306,9 @@ private void OnBorderBrushChanged(Brush oldValue, Brush newValue) #if __SKIA__ this.UpdateBorderBrush(); #else - Brush.SetupBrushChanged(oldValue, newValue, ref _borderBrushChanged, _borderBrushChanged ?? (() => + _brushChangedSubscription?.Dispose(); + + _brushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _borderBrushChanged, _borderBrushChanged ?? (() => { UpdateBorder(); })); diff --git a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs index c3ea30d794a3..2cce87d9f5f8 100644 --- a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs +++ b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs @@ -261,8 +261,11 @@ private IDisposable InnerCreateLayers( // because even though the brush instance is the same, there are additional properties // that BorderLayerState tracks on Android. This is not ideal and we should avoid it by refactoring // this file to handle brush changes on the same brush instance on its own instead. - Brush.SetupBrushChanged(_currentState.Background, background, ref _backgroundChanged, () => Update(true), false); - Brush.SetupBrushChanged(_currentState.BorderBrush, borderBrush, ref _borderChanged, () => Update(true), false); + _backgroundBrushChangedSubscription?.Dispose(); + _backgroundBrushChangedSubscription = Brush.SetupBrushChanged(background, ref _backgroundChanged, () => Update(true), false); + + _borderBrushChangedSubscription?.Dispose(); + _borderBrushChangedSubscription = Brush.SetupBrushChanged(borderBrush, ref _borderChanged, () => Update(true), false); return disposables; } diff --git a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.iOSmacOS.cs b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.iOSmacOS.cs index 117b7fd22928..e8c64e41d70f 100644 --- a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.iOSmacOS.cs +++ b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.iOSmacOS.cs @@ -228,8 +228,8 @@ void OnImageChanged(_Image _) { Action onInvalidateRender = () => backgroundLayer.FillColor = Brush.GetFallbackColor(background); onInvalidateRender(); - background.InvalidateRender += onInvalidateRender; - new DisposableAction(() => background.InvalidateRender -= onInvalidateRender).DisposeWith(disposables); + + background.RegisterInvalidateRender(onInvalidateRender).DisposeWith(disposables); } else { @@ -284,8 +284,7 @@ GradientBrush gradientBorder when gradientBorder.CanApplyToBorder(cornerRadius) else { onInvalidateRender(); - borderBrush.InvalidateRender += onInvalidateRender; - new DisposableAction(() => borderBrush.InvalidateRender -= onInvalidateRender).DisposeWith(disposables); + borderBrush.RegisterInvalidateRender(onInvalidateRender).DisposeWith(disposables); } } @@ -334,9 +333,7 @@ GradientBrush gradientBorder when gradientBorder.CanApplyToBorder(cornerRadius) Action onInvalidateRender = () => backgroundLayer.FillColor = Brush.GetFallbackColor(background); onInvalidateRender(); - background.InvalidateRender += onInvalidateRender; - new DisposableAction(() => background.InvalidateRender -= onInvalidateRender) - .DisposeWith(disposables); + background.RegisterInvalidateRender(onInvalidateRender).DisposeWith(disposables); // This is required because changing the CornerRadius changes the background drawing // implementation and we don't want a rectangular background behind a rounded background. @@ -362,10 +359,8 @@ imageData.NativeImage is _Image uiImage && else if (background is XamlCompositionBrushBase) { Action onInvalidateRender = () => backgroundLayer.FillColor = Brush.GetFallbackColor(background); - background.InvalidateRender += onInvalidateRender; + background.RegisterInvalidateRender(onInvalidateRender).DisposeWith(disposables); onInvalidateRender(); - new DisposableAction(() => background.InvalidateRender -= onInvalidateRender) - .DisposeWith(disposables); // This is required because changing the CornerRadius changes the background drawing // implementation and we don't want a rectangular background behind a rounded background. @@ -417,8 +412,7 @@ GradientBrush gradientBorder when gradientBorder.CanApplyToBorder(cornerRadius) Action onInvalidateRender = () => layer.FillColor = Brush.GetFallbackColor(borderBrush); onInvalidateRender(); - borderBrush.InvalidateRender += onInvalidateRender; - new DisposableAction(() => borderBrush.InvalidateRender -= onInvalidateRender).DisposeWith(disposables); + borderBrush.RegisterInvalidateRender(onInvalidateRender).DisposeWith(disposables); } } diff --git a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.wasm.cs b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.wasm.cs index fd3d8daeaf86..5d9965781ad8 100644 --- a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.wasm.cs +++ b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.wasm.cs @@ -20,6 +20,8 @@ partial class BorderLayerRenderer { private Action _backgroundChanged; private Action _borderChanged; + private IDisposable _borderBrushChangedSubscription; + private IDisposable _brushChangedSubscription; partial void UpdatePlatform(bool forceUpdate) { @@ -56,8 +58,9 @@ partial void UpdatePlatform(bool forceUpdate) private void SetAndObserveBorder(BorderLayerState newState) { SetBorder(newState); - Brush.SetupBrushChanged( - _currentState.BorderBrush, + + _borderBrushChangedSubscription?.Dispose(); + _borderBrushChangedSubscription = Brush.SetupBrushChanged( newState.BorderBrush, ref _borderChanged, () => @@ -217,7 +220,8 @@ private void SetAndObserveBackgroundBrush(BorderLayerState newState, ref Action if (newOnInvalidateRender is not null) { - Brush.SetupBrushChanged(oldValue, newValue, ref brushChanged, newOnInvalidateRender); + _brushChangedSubscription?.Dispose(); + _brushChangedSubscription = Brush.SetupBrushChanged(newValue, ref brushChanged, newOnInvalidateRender); } } diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.cs b/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.cs index 8051b4f06de9..8941520ee87f 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.cs @@ -40,6 +40,7 @@ public partial class TextBlock : DependencyObject, IThemeChangeAware { private InlineCollection _inlines; private string _inlinesText; // Text derived from the content of Inlines + private IDisposable _foregroundBrushChangedSubscription; #if !__WASM__ // Used for text selection which is handled natively @@ -485,7 +486,9 @@ Brush Foreground private void Subscribe(Brush oldValue, Brush newValue) { var newOnInvalidateRender = _foregroundChanged ?? (() => OnForegroundChanged()); - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundChanged, newOnInvalidateRender); + + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundChanged, newOnInvalidateRender); } private void OnForegroundChanged() diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs b/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs index e2717ed5a46f..116156733681 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.skia.cs @@ -26,6 +26,7 @@ partial class TextBlock : FrameworkElement, IBlock private readonly TextVisual _textVisual; private Action? _selectionHighlightColorChanged; private MenuFlyout? _contextMenu; + private IDisposable? _selectionHighlightBrushChangedSubscription; private readonly Dictionary _flyoutItems = new(); private readonly VirtualKeyModifiers _platformCtrlKey = OperatingSystem.IsMacOS() ? VirtualKeyModifiers.Windows : VirtualKeyModifiers.Control; @@ -357,7 +358,9 @@ private void OnSelectionHighlightColorChanged(SolidColorBrush? oldBrush, SolidCo { oldBrush ??= DefaultBrushes.SelectionHighlightColor; newBrush ??= DefaultBrushes.SelectionHighlightColor; - Brush.SetupBrushChanged(oldBrush, newBrush, ref _selectionHighlightColorChanged, () => OnSelectionHighlightColorChangedPartial(newBrush)); + + _selectionHighlightBrushChangedSubscription?.Dispose(); + _selectionHighlightBrushChangedSubscription = Brush.SetupBrushChanged(newBrush, ref _selectionHighlightColorChanged, () => OnSelectionHighlightColorChangedPartial(newBrush)); } partial void OnSelectionHighlightColorChangedPartial(SolidColorBrush brush); diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/MultilineTextBoxView.iOS.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/MultilineTextBoxView.iOS.cs index 8b53fa7c0730..37b676b20cf8 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/MultilineTextBoxView.iOS.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/MultilineTextBoxView.iOS.cs @@ -26,6 +26,7 @@ public partial class MultilineTextBoxView : UITextView, ITextBoxView, Dependency private readonly WeakReference _textBox; private WeakReference _window; private Action _foregroundChanged; + private IDisposable _foregroundBrushChangedSubscription; public override void Paste(NSObject sender) => HandlePaste(() => base.Paste(sender)); @@ -234,7 +235,8 @@ public void OnForegroundChanged(Brush oldValue, Brush newValue) { if (newValue is SolidColorBrush scb) { - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundChanged, () => ApplyColor()); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundChanged, () => ApplyColor()); void ApplyColor() { diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/SinglelineTextBoxView.iOS.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/SinglelineTextBoxView.iOS.cs index 7d0635312c36..91a18eaf8e88 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/SinglelineTextBoxView.iOS.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/SinglelineTextBoxView.iOS.cs @@ -22,6 +22,7 @@ public partial class SinglelineTextBoxView : UITextField, ITextBoxView, Dependen private SinglelineTextBoxDelegate _delegate; private readonly WeakReference _textBox; private Action _foregroundChanged; + private IDisposable _foregroundBrushChangedSubscription; public SinglelineTextBoxView(TextBox textBox) { @@ -204,7 +205,8 @@ public void OnForegroundChanged(Brush oldValue, Brush newValue) { if (newValue is SolidColorBrush scb) { - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundChanged, () => ApplyColor()); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundChanged, () => ApplyColor()); void ApplyColor() { diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.cs index a9b8d293542e..18bc396a19d0 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.cs @@ -62,6 +62,8 @@ public partial class TextBox : Control, IFrameworkTemplatePoolAware private Action _selectionHighlightColorChanged; private Action _foregroundBrushChanged; + private IDisposable _selectionHighlightBrushChangedSubscription; + private IDisposable _foregroundBrushChangedSubscription; #pragma warning restore CS0067, CS0649 private ContentPresenter _header; @@ -523,7 +525,8 @@ protected override void OnFontWeightChanged(FontWeight oldValue, FontWeight newV protected override void OnForegroundColorChanged(Brush oldValue, Brush newValue) { - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundBrushChanged, () => OnForegroundColorChangedPartial(newValue)); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundBrushChanged, () => OnForegroundColorChangedPartial(newValue)); } partial void OnForegroundColorChangedPartial(Brush newValue); @@ -573,7 +576,9 @@ private void OnSelectionHighlightColorChanged(SolidColorBrush oldBrush, SolidCol { oldBrush ??= DefaultBrushes.SelectionHighlightColor; newBrush ??= DefaultBrushes.SelectionHighlightColor; - Brush.SetupBrushChanged(oldBrush, newBrush, ref _selectionHighlightColorChanged, () => OnSelectionHighlightColorChangedPartial(newBrush)); + + _selectionHighlightBrushChangedSubscription?.Dispose(); + _selectionHighlightBrushChangedSubscription = Brush.SetupBrushChanged(newBrush, ref _selectionHighlightColorChanged, () => OnSelectionHighlightColorChangedPartial(newBrush)); } partial void OnSelectionHighlightColorChangedPartial(SolidColorBrush brush); diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs index 4434466b2161..27eb2641984b 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs @@ -310,7 +310,8 @@ private void OnForegroundChanged(Brush oldValue, Brush newValue) { if (newValue is SolidColorBrush scb) { - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundChanged, () => ApplyColor()); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundChanged, () => ApplyColor()); void ApplyColor() { diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs index b2345db431dd..1d3bb5dfc92b 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs @@ -177,7 +177,8 @@ public NSRange SelectedRange public void OnForegroundChanged(Brush oldValue, Brush newValue) { - Brush.SetupBrushChanged(oldValue, newValue, ref _foregroundChanged, () => ApplyColor()); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _foregroundChanged, () => ApplyColor()); void ApplyColor() { diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.wasm.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.wasm.cs index eaae7da079b3..1d23a28fc281 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.wasm.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.wasm.cs @@ -20,6 +20,7 @@ internal partial class TextBoxView : FrameworkElement { private readonly TextBox _textBox; private Action _foregroundChanged; + private IDisposable _foregroundBrushChangedSubscription; private bool _browserContextMenuEnabled = true; private bool _isReadOnly; @@ -44,7 +45,8 @@ private void OnForegroundChanged(DependencyPropertyChangedEventArgs e) { if (e.NewValue is SolidColorBrush scb) { - Brush.SetupBrushChanged(e.OldValue as Brush, scb, ref _foregroundChanged, () => SetForeground(scb)); + _foregroundBrushChangedSubscription?.Dispose(); + _foregroundBrushChangedSubscription = Brush.SetupBrushChanged(scb, ref _foregroundChanged, () => SetForeground(scb)); } } diff --git a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.Android.tt b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.Android.tt index 21e35db05b86..085fb15c2111 100644 --- a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.Android.tt +++ b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.Android.tt @@ -181,12 +181,14 @@ namespace <#= mixin.NamespaceName #> } Action _brushChanged; + IDisposable _backgroundBrushChangedSubscription; protected virtual void OnBackgroundChanged(DependencyPropertyChangedEventArgs e) { - var oldValue = e.OldValue as Brush; var newValue = e.NewValue as Brush; - Brush.SetupBrushChanged(oldValue, newValue, ref _brushChanged, () => SetBackgroundColor(Brush.GetFallbackColor(newValue))); + + _backgroundBrushChangedSubscription?.Dispose(); + _backgroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _brushChanged, () => SetBackgroundColor(Brush.GetFallbackColor(newValue))); } #endregion diff --git a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.iOS.tt b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.iOS.tt index e124cf977e9e..8eff9ce29a25 100644 --- a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.iOS.tt +++ b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.iOS.tt @@ -869,12 +869,14 @@ namespace <#= mixin.NamespaceName #> } Action _brushChanged; + IDisposable _backgroundBrushChangedSubscription; protected virtual void OnBackgroundChanged(DependencyPropertyChangedEventArgs e) { - var oldValue = e.OldValue as Brush; var newValue = e.NewValue as Brush; - Brush.SetupBrushChanged(oldValue, newValue, ref _brushChanged, () => Layer.BackgroundColor = Brush.GetFallbackColor(newValue)); + + _backgroundBrushChangedSubscription?.Dispose(); + _backgroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _brushChanged, () => Layer.BackgroundColor = Brush.GetFallbackColor(newValue)); } #endregion diff --git a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.macOS.tt b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.macOS.tt index 6c0a719065c8..b360cc6f7f84 100644 --- a/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.macOS.tt +++ b/src/Uno.UI/UI/Xaml/IFrameworkElementImplementation.macOS.tt @@ -863,17 +863,18 @@ namespace <#= mixin.NamespaceName #> Action _brushChanged; + IDisposable _backgroundBrushChangedSubscription; protected virtual void OnBackgroundChanged(DependencyPropertyChangedEventArgs e) { - var oldValue = e.OldValue as Brush; var newValue = e.NewValue as Brush; if (newValue is not null) { WantsLayer = true; } - Brush.SetupBrushChanged(oldValue, newValue, ref _brushChanged, () => Layer.BackgroundColor = Brush.GetFallbackColor(newValue)); + _backgroundBrushChangedSubscription?.Dispose(); + _backgroundBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _brushChanged, () => Layer.BackgroundColor = Brush.GetFallbackColor(newValue)); } #endregion diff --git a/src/Uno.UI/UI/Xaml/Media/Brush.cs b/src/Uno.UI/UI/Xaml/Media/Brush.cs index 153d45ec9d1b..29080205ad88 100644 --- a/src/Uno.UI/UI/Xaml/Media/Brush.cs +++ b/src/Uno.UI/UI/Xaml/Media/Brush.cs @@ -1,6 +1,7 @@ #nullable enable using System; +using System.Collections.Generic; using System.ComponentModel; using Microsoft.UI.Composition; using Uno.Disposables; @@ -9,6 +10,7 @@ #if HAS_UNO_WINUI using Windows.UI; +using Windows.UI.Core; #endif namespace Microsoft.UI.Xaml.Media @@ -16,13 +18,15 @@ namespace Microsoft.UI.Xaml.Media [TypeConverter(typeof(BrushConverter))] public partial class Brush : DependencyObject { - private readonly WeakEventManager _weakEventManager = new(); + private List? _invalidateRenderHandlers; - internal event Action? InvalidateRender - { - add => _weakEventManager.AddEventHandler(value); - remove => _weakEventManager.RemoveEventHandler(value); - } + internal IDisposable RegisterInvalidateRender(Action handler) + => WeakEventHelper.RegisterEvent( + _invalidateRenderHandlers ??= [], + handler, + (h, s, e) => + (h as Action)?.Invoke() + ); protected Brush() { @@ -46,30 +50,36 @@ internal static Color GetFallbackColor(Brush brush) public static implicit operator Brush(string colorCode) => SolidColorBrushHelper.Parse(colorCode); - internal static void SetupBrushChanged(Brush? oldValue, Brush? newValue, ref Action? onInvalidateRender, Action newOnInvalidateRender, bool initialInvoke = true) + internal static IDisposable? SetupBrushChanged(Brush? newValue, ref Action? onInvalidateRender, Action newOnInvalidateRender, bool initialInvoke = true) { - if (oldValue is not null && onInvalidateRender is not null) - { - oldValue.InvalidateRender -= onInvalidateRender; - } if (initialInvoke) { newOnInvalidateRender(); } + if (newValue is not null) { onInvalidateRender = newOnInvalidateRender; - newValue.InvalidateRender += onInvalidateRender; + return newValue.RegisterInvalidateRender(onInvalidateRender); } else { onInvalidateRender = null; } + + return null; } private protected void OnInvalidateRender() { - _weakEventManager.HandleEvent(nameof(InvalidateRender)); + if (_invalidateRenderHandlers is not null) + { + foreach (var action in _invalidateRenderHandlers) + { + action(this, null); + } + } + #if __SKIA__ SynchronizeCompositionBrush(); #endif diff --git a/src/Uno.UI/UI/Xaml/Shapes/Shape.cs b/src/Uno.UI/UI/Xaml/Shapes/Shape.cs index fbf6754abfd6..a4c20fc368ea 100644 --- a/src/Uno.UI/UI/Xaml/Shapes/Shape.cs +++ b/src/Uno.UI/UI/Xaml/Shapes/Shape.cs @@ -19,6 +19,8 @@ public abstract partial class Shape : FrameworkElement #if !__SKIA__ private Action _brushChanged; private Action _strokeBrushChanged; + private IDisposable _brushChangedSubscription; + private IDisposable _strokeBrushChangedSubscription; #endif /// @@ -62,7 +64,8 @@ private void OnFillChanged(Brush oldValue, Brush newValue) // In this case, we don't really want to listen to brush changes as the Brush is responsible for synchronizing its internal composition brush OnFillBrushChanged(); #else - Brush.SetupBrushChanged(oldValue, newValue, ref _brushChanged, () => OnFillBrushChanged()); + _brushChangedSubscription?.Dispose(); + _brushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _brushChanged, () => OnFillBrushChanged()); #endif } @@ -98,7 +101,8 @@ private void OnStrokeChanged(Brush oldValue, Brush newValue) // In this case, we don't really want to listen to brush changes as the Brush is responsible for synchronizing its internal composition brush OnStrokeBrushChanged(); #else - Brush.SetupBrushChanged(oldValue, newValue, ref _strokeBrushChanged, () => OnStrokeBrushChanged()); + _strokeBrushChangedSubscription?.Dispose(); + _strokeBrushChangedSubscription = Brush.SetupBrushChanged(newValue, ref _strokeBrushChanged, () => OnStrokeBrushChanged()); #endif } diff --git a/src/Uno.UI/Uno.UI.netcoremobile.csproj b/src/Uno.UI/Uno.UI.netcoremobile.csproj index 69a19239075a..3bb7854e34ce 100644 --- a/src/Uno.UI/Uno.UI.netcoremobile.csproj +++ b/src/Uno.UI/Uno.UI.netcoremobile.csproj @@ -46,14 +46,6 @@ - - True - True - - - True - True - True True @@ -144,5 +136,8 @@ + + + From 8a23ca1525b277dc3463e6e88661462ce7b4cf62 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Fri, 22 Nov 2024 20:18:13 -0500 Subject: [PATCH 02/14] chore: Adjust android, macos --- .../UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs | 2 ++ src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs | 1 + src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs | 1 + 3 files changed, 4 insertions(+) diff --git a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs index 2cce87d9f5f8..1bb8e740c1e4 100644 --- a/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs +++ b/src/Uno.UI/UI/Xaml/Controls/Border/BorderLayerRenderer.Android.cs @@ -33,6 +33,8 @@ partial class BorderLayerRenderer private static Paint? _fillPaint; private Action? _backgroundChanged; private Action? _borderChanged; + private IDisposable? _backgroundBrushChangedSubscription; + private IDisposable? _borderBrushChangedSubscription; private static ImageSource? GetBackgroundImageSource(BorderLayerState? state) => (state?.Background as ImageBrush)?.ImageSource; diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs index 27eb2641984b..519f0eac9140 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs @@ -28,6 +28,7 @@ internal partial class TextBoxView : EditText, DependencyObject internal TextBox? Owner => _ownerRef?.Target as TextBox; private Action? _foregroundChanged; + private IDisposable? _foregroundBrushChangedSubscription; private bool _isDisposed; public TextBoxView(TextBox owner) diff --git a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs index 1d3bb5dfc92b..292331cbf95c 100644 --- a/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs +++ b/src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.macOS.cs @@ -21,6 +21,7 @@ internal partial class TextBoxView : _TextField, ITextBoxView, DependencyObject, private readonly WeakReference _textBox; private Action _foregroundChanged; + private IDisposable _foregroundBrushChangedSubscription; public TextBoxView(TextBox textBox) { From 92335c3c4001ce29254f6296d6ce86cbe9b35267 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sat, 23 Nov 2024 09:47:50 -0500 Subject: [PATCH 03/14] chore: Adjust UWP build --- src/Uno.UI/UI/Xaml/Media/Brush.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Uno.UI/UI/Xaml/Media/Brush.cs b/src/Uno.UI/UI/Xaml/Media/Brush.cs index 29080205ad88..cad2f09520f8 100644 --- a/src/Uno.UI/UI/Xaml/Media/Brush.cs +++ b/src/Uno.UI/UI/Xaml/Media/Brush.cs @@ -7,10 +7,10 @@ using Uno.Disposables; using Uno.UI.Helpers; using Uno.UI.Xaml; +using Windows.UI.Core; #if HAS_UNO_WINUI using Windows.UI; -using Windows.UI.Core; #endif namespace Microsoft.UI.Xaml.Media From cee9464a4fc634620316664481314be517acae71 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sat, 23 Nov 2024 12:52:57 -0500 Subject: [PATCH 04/14] chore: Use GC events to cleanup the handlers collection --- .../Internal-WeakEventHelper.md | 32 ++- src/SamplesApp/SamplesApp.Wasm/Program.cs | 1 + .../SamplesApp.Wasm/SamplesApp.Wasm.csproj | 3 + .../Tests/Windows_UI/Given_WeakEventHelper.cs | 234 ++++++++++++++++++ src/Uno.UI/Microsoft/UI/Input/InputCursor.cs | 11 +- src/Uno.UI/UI/Xaml/Media/Brush.cs | 12 +- src/Uno.UI/UI/Xaml/Window/Window.cs | 23 +- src/Uno.UWP/UI/Core/WeakEventHelper.cs | 158 +++++++++--- 8 files changed, 396 insertions(+), 78 deletions(-) create mode 100644 src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs diff --git a/doc/articles/uno-development/Internal-WeakEventHelper.md b/doc/articles/uno-development/Internal-WeakEventHelper.md index 785a3d926e78..95efe86f61d9 100644 --- a/doc/articles/uno-development/Internal-WeakEventHelper.md +++ b/doc/articles/uno-development/Internal-WeakEventHelper.md @@ -8,7 +8,7 @@ The WeakEventHelper class is an internal method that is designed to provide a memory-friendly environment for registering to events internally in Uno. This class is not exposed to the end-user because its patterns do not fit with the -original UWP event-based designs of the API. +original WinUI event-based designs of the API. ## The RegisterEvent method @@ -17,35 +17,43 @@ that both the source and the target are weak. The source must be kept alive by another longer-lived reference, and the target is kept alive by the return disposable. -If the returned disposable is collected, the handler will also be -collected. Conversely, if the provided list is collected -raising the event will produce nothing. +If the provided handler is collected, the registration will +be collected as well. The returned disposable is not tracked, which means that it will +not remove the registration when collected. + +The WeakEventCollection automatically manages its internal registration list using GC events. Here's a usage example: - private List _sizeChangedHandlers = new List(); + ```csharp + private WeakEventHelper.WeakEventCollection? _sizeChangedHandlers; internal IDisposable RegisterSizeChangedEvent(WindowSizeChangedEventHandler handler) { return WeakEventHelper.RegisterEvent( - _sizeChangedHandlers, + _sizeChangedHandlers ??= new(), handler, (h, s, e) => (h as WindowSizeChangedEventHandler)?.Invoke(s, (WindowSizeChangedEventArgs)e) ); } + internal void RaiseEvent() + { + _sizeChangedHandlers?.Invoke(this, new WindowSizeChangedEventArgs()); + } + ``` + The RegisterEvent method is intentionally non-generic to avoid the cost related to AOT performance. The performance cost is shifted to downcast and upcast checks in the `EventRaiseHandler` handlers. The returned disposable must be used as follows : - private SerialDisposable _sizeChangedSubscription = new SerialDisposable(); + ```csharp + private IDisposable? _sizeChangedSubscription; ... - _sizeChangedSubscription.Disposable = null; + _sizeChangedSubscription?.Dispose(); - if (Owner != null) - { - _sizeChangedSubscription.Disposable = Window.Current.RegisterSizeChangedEvent(OnCurrentWindowSizeChanged); - } + _sizeChangedSubscription = Window.Current.RegisterSizeChangedEvent(OnCurrentWindowSizeChanged); + ``` diff --git a/src/SamplesApp/SamplesApp.Wasm/Program.cs b/src/SamplesApp/SamplesApp.Wasm/Program.cs index 3e404a467dd8..c5a5fdba0adc 100644 --- a/src/SamplesApp/SamplesApp.Wasm/Program.cs +++ b/src/SamplesApp/SamplesApp.Wasm/Program.cs @@ -24,6 +24,7 @@ public static void Main(string[] args) #endif Microsoft.UI.Xaml.Application.Start(_ => _app = new App()); + } } } diff --git a/src/SamplesApp/SamplesApp.Wasm/SamplesApp.Wasm.csproj b/src/SamplesApp/SamplesApp.Wasm/SamplesApp.Wasm.csproj index a116abef56df..bbeae6d475d2 100644 --- a/src/SamplesApp/SamplesApp.Wasm/SamplesApp.Wasm.csproj +++ b/src/SamplesApp/SamplesApp.Wasm/SamplesApp.Wasm.csproj @@ -17,6 +17,9 @@ false true + + false + diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs new file mode 100644 index 000000000000..f8cb75732c43 --- /dev/null +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -0,0 +1,234 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text; +using System.Threading.Tasks; +using Microsoft.UI.Xaml.CustomAttributes; +using Uno.Buffers; +using Windows.Graphics.Capture; +using Windows.UI.Core; + +namespace Uno.UI.RuntimeTests.Tests.Windows_UI; + +[TestClass] +public class Given_WeakEventHelper +{ + [TestMethod] + public void When_Explicit_Dispose() + { + WeakEventHelper.WeakEventCollection SUT = new(); + + var invoked = 0; + Action action = () => invoked++; + + var disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + + disposable.Dispose(); + + // When disposed invoking events won't call the original action + // the registration has been disposed. + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + } + + [TestMethod] + public void When_Registration_Collected() + { + WeakEventHelper.WeakEventCollection SUT = new(); + + var invoked = 0; + Action action = () => invoked++; + + void Do() + { + var disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + + disposable = null; + } + + Do(); + + GC.Collect(2); + GC.WaitForPendingFinalizers(); + + // Even if the disposable is collected, the event should still be invoked + // as the disposable does not track the event registration. + SUT.Invoke(this, null); + + Assert.AreEqual(2, invoked); + } + + [TestMethod] + public void When_Target_Collected() + { + WeakEventHelper.WeakEventCollection SUT = new(); + + var invoked = 0; + IDisposable disposable = null; + + void Do() + { + Action action = () => invoked++; + + disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + } + + Do(); + + GC.Collect(2); + GC.WaitForPendingFinalizers(); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + } + + [TestMethod] + public void When_Many_Targets_Collected() + { + WeakEventHelper.WeakEventCollection SUT = new(); + + var invoked = 0; + IDisposable disposable = null; + + void Do() + { + Action action = () => invoked++; + + disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + } + + for (int i = 0; i < 100; i++) + { + Do(); + } + + GC.Collect(2); + GC.WaitForPendingFinalizers(); + + SUT.Invoke(this, null); + + Assert.AreEqual(5050, invoked); + + // Ensure that everything has been collected. + SUT.Invoke(this, null); + + Assert.AreEqual(5050, invoked); + } + + [TestMethod] + public void When_Collection_Disposed() + { + WeakEventHelper.WeakEventCollection SUT = new(); + + var invoked = 0; + + Action action = () => invoked++; + + var disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + + SUT.Dispose(); + } + + [TestMethod] + public async Task When_Collection_Collected() + { + WeakReference actionRef = null; + WeakReference collectionRef = null; + + void Do() + { + WeakEventHelper.WeakEventCollection SUT = new(); + collectionRef = new(SUT); + + var invoked = 0; + + Action action = () => invoked++; + actionRef = new(actionRef); + + var disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + + SUT.Dispose(); + } + + Do(); + + var sw = Stopwatch.StartNew(); + + while ((actionRef.IsAlive || collectionRef.IsAlive) && sw.ElapsedMilliseconds < 5000) + { + await Task.Delay(10); + GC.Collect(2); + GC.WaitForPendingFinalizers(); + } + + Assert.IsFalse(actionRef.IsAlive); + Assert.IsFalse(collectionRef.IsAlive); + } + + [TestMethod] + public void When_Empty_Trim_Stops() + { + TestPlatformProvider trimProvider = new(); + WeakEventHelper.WeakEventCollection SUT = new(trimProvider); + + var invoked = 0; + + Action action = () => invoked++; + + Assert.IsNull(trimProvider.Invoke()); + + var disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + + Assert.IsTrue(trimProvider.Invoke()); + + SUT.Invoke(this, null); + + Assert.AreEqual(1, invoked); + + disposable.Dispose(); + + Assert.IsFalse(trimProvider.Invoke()); + + Assert.AreEqual(1, invoked); + } + + private class TestPlatformProvider : WeakEventHelper.ITrimProvider + { + private object _target; + private Func _callback; + + public void RegisterTrimCallback(Func callback, object target) + { + _target = target; + _callback = callback; + } + + public bool? Invoke() => _callback?.Invoke(_target); + } +} diff --git a/src/Uno.UI/Microsoft/UI/Input/InputCursor.cs b/src/Uno.UI/Microsoft/UI/Input/InputCursor.cs index b0e1fbb145d3..323a0c1593f4 100644 --- a/src/Uno.UI/Microsoft/UI/Input/InputCursor.cs +++ b/src/Uno.UI/Microsoft/UI/Input/InputCursor.cs @@ -11,8 +11,7 @@ public partial class InputCursor internal partial class InputCursor #endif { - - private List _disposedHandlers = new List(); + private WeakEventHelper.WeakEventCollection _disposedHandlers = new(); internal bool IsDisposed { get; private set; } @@ -27,11 +26,9 @@ public void Dispose() if (!IsDisposed) { IsDisposed = true; - var handlers = new List(_disposedHandlers); - foreach (var action in handlers) - { - action(this, null); - } + + _disposedHandlers.Invoke(this, null); + _disposedHandlers.Dispose(); } } diff --git a/src/Uno.UI/UI/Xaml/Media/Brush.cs b/src/Uno.UI/UI/Xaml/Media/Brush.cs index cad2f09520f8..69082d4c77e5 100644 --- a/src/Uno.UI/UI/Xaml/Media/Brush.cs +++ b/src/Uno.UI/UI/Xaml/Media/Brush.cs @@ -18,11 +18,11 @@ namespace Microsoft.UI.Xaml.Media [TypeConverter(typeof(BrushConverter))] public partial class Brush : DependencyObject { - private List? _invalidateRenderHandlers; + private WeakEventHelper.WeakEventCollection? _invalidateRenderHandlers; internal IDisposable RegisterInvalidateRender(Action handler) => WeakEventHelper.RegisterEvent( - _invalidateRenderHandlers ??= [], + _invalidateRenderHandlers ??= new(), handler, (h, s, e) => (h as Action)?.Invoke() @@ -72,13 +72,7 @@ internal static Color GetFallbackColor(Brush brush) private protected void OnInvalidateRender() { - if (_invalidateRenderHandlers is not null) - { - foreach (var action in _invalidateRenderHandlers) - { - action(this, null); - } - } + _invalidateRenderHandlers?.Invoke(this, null); #if __SKIA__ SynchronizeCompositionBrush(); diff --git a/src/Uno.UI/UI/Xaml/Window/Window.cs b/src/Uno.UI/UI/Xaml/Window/Window.cs index 2b7d00ae89d8..26a252038f5b 100644 --- a/src/Uno.UI/UI/Xaml/Window/Window.cs +++ b/src/Uno.UI/UI/Xaml/Window/Window.cs @@ -47,8 +47,8 @@ partial class Window private bool _splashScreenDismissed; private WindowType _windowType; - private List _sizeChangedHandlers = new List(); - private List? _backgroundChangedHandlers; + private WeakEventHelper.WeakEventCollection? _sizeChangedHandlers; + private WeakEventHelper.WeakEventCollection? _backgroundChangedHandlers; internal Window(WindowType windowType) { @@ -340,13 +340,7 @@ internal Brush? Background { _background = value; - if (_backgroundChangedHandlers != null) - { - foreach (var action in _backgroundChangedHandlers) - { - action(this, EventArgs.Empty); - } - } + _backgroundChangedHandlers?.Invoke(this, EventArgs.Empty); } } @@ -355,7 +349,7 @@ internal IDisposable RegisterBackgroundChangedEvent(EventHandler handler) _backgroundChangedHandlers ??= new(), handler, (h, s, e) => - (h as EventHandler)?.Invoke(s, (EventArgs)e) + (h as EventHandler)?.Invoke(s, (EventArgs)e!) ); /// @@ -365,18 +359,15 @@ internal IDisposable RegisterBackgroundChangedEvent(EventHandler handler) internal IDisposable RegisterSizeChangedEvent(Microsoft.UI.Xaml.WindowSizeChangedEventHandler handler) { return WeakEventHelper.RegisterEvent( - _sizeChangedHandlers, + _sizeChangedHandlers ??= new(), handler, (h, s, e) => - (h as Microsoft.UI.Xaml.WindowSizeChangedEventHandler)?.Invoke(s, (WindowSizeChangedEventArgs)e) + (h as Microsoft.UI.Xaml.WindowSizeChangedEventHandler)?.Invoke(s, (WindowSizeChangedEventArgs)e!) ); } private void OnWindowSizeChanged(object sender, WindowSizeChangedEventArgs e) { - foreach (var action in _sizeChangedHandlers) - { - action(this, e); - } + _sizeChangedHandlers?.Invoke(this, e); } } diff --git a/src/Uno.UWP/UI/Core/WeakEventHelper.cs b/src/Uno.UWP/UI/Core/WeakEventHelper.cs index d9680f9d61c7..1c9d0e64cf14 100644 --- a/src/Uno.UWP/UI/Core/WeakEventHelper.cs +++ b/src/Uno.UWP/UI/Core/WeakEventHelper.cs @@ -1,8 +1,12 @@ -using System; +#nullable enable + +using System; using System.Collections.Generic; using System.Text; +using Uno.Buffers; using Uno.Disposables; using Uno.Extensions; +using Windows.Security.Cryptography.Core; namespace Windows.UI.Core { @@ -14,7 +18,7 @@ internal class WeakEventHelper /// The delegate to call with and . /// The source of the event raise /// The args used to raise the event - public delegate void EventRaiseHandler(Delegate @delegate, object source, object args); + public delegate void EventRaiseHandler(Delegate @delegate, object source, object? args); /// /// An abstract handler to be called when raising the weak event. @@ -24,7 +28,116 @@ internal class WeakEventHelper /// The args of the event, which must match the explicit cast /// performed in the matching instance. /// - public delegate void GenericEventHandler(object source, object args); + public delegate void GenericEventHandler(object source, object? args); + + /// + /// Provides an abstraction for GC trimming registration + /// + internal interface ITrimProvider + { + /// + /// Registers a callback to be called when GC triggerd + /// + /// Function called with as the first parameter + /// instance to be provided when invoking + void RegisterTrimCallback(Func callback, object target); + } + + /// + /// Default trimming implementation which uses the actual GC implementation + /// + private class DefaultTrimProvider : ITrimProvider + { + public void RegisterTrimCallback(Func callback, object target) + { + Windows.Foundation.Gen2GcCallback.Register(callback, target); + } + } + + /// + /// A collection of weak event registrations + /// + public class WeakEventCollection : IDisposable + { + private record WeakHandler(WeakReference Target, GenericEventHandler Handler); + + private object _lock = new object(); + private List _handlers = []; + private readonly ITrimProvider _provider; + + public WeakEventCollection(ITrimProvider? provider = null) + { + _provider = provider ?? new DefaultTrimProvider(); + } + + private bool Trim() + { + lock (_lock) + { + for (int i = 0; i < _handlers.Count; i++) + { + if (!_handlers[i].Target.IsAlive) + { + _handlers.RemoveAt(i); + i--; + } + } + + return _handlers.Count != 0; + } + } + + /// + /// Invokes all the alive registered handlers + /// + public void Invoke(object sender, object? args) + { + lock (_lock) + { + for (int i = 0; i < _handlers.Count; i++) + { + _handlers[i].Handler(sender, args); + } + } + } + + /// + /// Do not use directly, use instead. + /// Registers an handler to be called when the event is raised. + /// + /// A disposable that can be used to unregister the provided handler. The disposable instance is not tracked by the GC and will not collect the registration. + internal IDisposable Register(WeakReference target, GenericEventHandler handler) + { + lock (_lock) + { + WeakHandler key = new(target, handler); + _handlers.Add(key); + + if (_handlers.Count == 1) + { + _provider.RegisterTrimCallback(_ => Trim(), this); + } + + return Disposable.Create(RemoveRegistration); + + void RemoveRegistration() + { + lock (_lock) + { + _handlers.Remove(key); + } + } + } + } + + public void Dispose() + { + lock (_lock) + { + _handlers.Clear(); + } + } + } /// /// Provides a bi-directional weak event handler management. @@ -39,30 +152,21 @@ internal class WeakEventHelper /// another longer-lived reference, and the target is kept alive by the /// return disposable. /// - /// If the returned disposable is collected, the handler will also be - /// collected. Conversely, if the is collected - /// raising the event will produce nothing. + /// If either the or the are + /// collected, raising the event will produce nothing. + /// + /// The returned disposable instance itself is not tracked by the GC and will not collect the registration. /// - internal static IDisposable RegisterEvent(IList list, Delegate handler, EventRaiseHandler raise) + internal static IDisposable RegisterEvent(WeakEventCollection list, Delegate handler, EventRaiseHandler raise) { var wr = new WeakReference(handler); - GenericEventHandler genericHandler = null; + GenericEventHandler? genericHandler = null; // This weak reference ensure that the closure will not link // the caller and the callee, in the same way "newValueActionWeak" // does not link the callee to the caller. - var instanceRef = new WeakReference>(list); - - Action removeHandler = () => - { - var thatList = instanceRef.GetTarget(); - - if (thatList != null) - { - thatList.Remove(genericHandler); - } - }; + var instanceRef = new WeakReference(list); genericHandler = (s, e) => { @@ -72,23 +176,9 @@ internal static IDisposable RegisterEvent(IList list, Deleg { raise(weakHandler, s, e); } - else - { - removeHandler(); - } }; - list.Add(genericHandler); - - return Disposable.Create(() => - { - removeHandler(); - - // Force a closure on the callback, to make its lifetime as long - // as the subscription being held by the callee. - handler = null; - }); + return list.Register(wr, genericHandler); } - } } From 26687ad2d821685e159b60af64f1a7ed2cfb4b9c Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sat, 23 Nov 2024 13:49:51 -0500 Subject: [PATCH 05/14] chore: Adjust doc --- doc/articles/uno-development/Internal-WeakEventHelper.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/articles/uno-development/Internal-WeakEventHelper.md b/doc/articles/uno-development/Internal-WeakEventHelper.md index 95efe86f61d9..ff854ad38c73 100644 --- a/doc/articles/uno-development/Internal-WeakEventHelper.md +++ b/doc/articles/uno-development/Internal-WeakEventHelper.md @@ -48,7 +48,7 @@ performance cost is shifted to downcast and upcast checks in the `EventRaiseHand The returned disposable must be used as follows : - ```csharp + ```csharp private IDisposable? _sizeChangedSubscription; ... From fc84e1232bd410fa6343a01b27bbed29471576cc Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sat, 23 Nov 2024 15:09:39 -0500 Subject: [PATCH 06/14] chore: Adjust comment, unused using --- .../Tests/Windows_UI/Given_WeakEventHelper.cs | 1 - src/Uno.UWP/UI/Core/WeakEventHelper.cs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs index f8cb75732c43..2aa340968942 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -5,7 +5,6 @@ using System.Runtime.CompilerServices; using System.Text; using System.Threading.Tasks; -using Microsoft.UI.Xaml.CustomAttributes; using Uno.Buffers; using Windows.Graphics.Capture; using Windows.UI.Core; diff --git a/src/Uno.UWP/UI/Core/WeakEventHelper.cs b/src/Uno.UWP/UI/Core/WeakEventHelper.cs index 1c9d0e64cf14..806ef896a463 100644 --- a/src/Uno.UWP/UI/Core/WeakEventHelper.cs +++ b/src/Uno.UWP/UI/Core/WeakEventHelper.cs @@ -36,9 +36,9 @@ internal class WeakEventHelper internal interface ITrimProvider { /// - /// Registers a callback to be called when GC triggerd + /// Registers a callback to be called when GC triggered /// - /// Function called with as the first parameter + /// Function called with as the first parameter /// instance to be provided when invoking void RegisterTrimCallback(Func callback, object target); } From a305b1cab098b4d6991f530f7e9ddc8115adc7a4 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sat, 23 Nov 2024 15:36:51 -0500 Subject: [PATCH 07/14] chore: Adjust output path, uno test only --- build/ci/.azure-devops-wasm-uitests.yml | 2 +- .../Tests/Windows_UI/Given_WeakEventHelper.cs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/build/ci/.azure-devops-wasm-uitests.yml b/build/ci/.azure-devops-wasm-uitests.yml index 8ba323358a8c..dcab7f230f47 100644 --- a/build/ci/.azure-devops-wasm-uitests.yml +++ b/build/ci/.azure-devops-wasm-uitests.yml @@ -42,7 +42,7 @@ jobs: - task: CopyFiles@2 displayName: 'Publish Wasm Site (net9.0)' inputs: - SourceFolder: $(build.sourcesdirectory)/src/SamplesApp/SamplesApp.Wasm/bin/Release/net9.0/browser-wasm/publish/wwwroot + SourceFolder: $(build.sourcesdirectory)/src/SamplesApp/SamplesApp.Wasm/bin/Release/net9.0/publish/wwwroot Contents: '**/*.*' TargetFolder: $(build.artifactstagingdirectory)/site-net9.0-$(XAML_FLAVOR_BUILD) CleanTargetFolder: false diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs index 2aa340968942..29ffc612d36c 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -1,4 +1,5 @@ -using System; +#if HAS_UNO +using System; using System.Collections.Generic; using System.Diagnostics; using System.Linq; @@ -231,3 +232,4 @@ public void RegisterTrimCallback(Func callback, object target) public bool? Invoke() => _callback?.Invoke(_target); } } +#endif From ad137dbc035deb52e010e1eddb9b8483f3d326b1 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Sun, 24 Nov 2024 10:20:25 -0500 Subject: [PATCH 08/14] chore: Adjust collection --- .../Tests/Windows_UI/Given_WeakEventHelper.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs index 29ffc612d36c..824191cb947c 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -104,13 +104,13 @@ public void When_Many_Targets_Collected() WeakEventHelper.WeakEventCollection SUT = new(); var invoked = 0; - IDisposable disposable = null; + List disposable = new(); void Do() { Action action = () => invoked++; - disposable = WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke()); + disposable.Add(WeakEventHelper.RegisterEvent(SUT, action, (s, e, a) => (s as Action).Invoke())); SUT.Invoke(this, null); } @@ -120,17 +120,19 @@ void Do() Do(); } - GC.Collect(2); - GC.WaitForPendingFinalizers(); - SUT.Invoke(this, null); Assert.AreEqual(5050, invoked); + disposable.Clear(); + + GC.Collect(2); + GC.WaitForPendingFinalizers(); + // Ensure that everything has been collected. SUT.Invoke(this, null); - Assert.AreEqual(5050, invoked); + Assert.AreEqual(5150, invoked); } [TestMethod] From 6ca6b65ad74187455bc51dce769a7d5ccdbdf5aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Laban?= Date: Sun, 24 Nov 2024 12:20:42 -0500 Subject: [PATCH 09/14] chore: adjust count --- .../Tests/Windows_UI/Given_WeakEventHelper.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs index 824191cb947c..11bc0084cdcb 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -1,4 +1,4 @@ -#if HAS_UNO +#if HAS_UNO using System; using System.Collections.Generic; using System.Diagnostics; @@ -122,7 +122,7 @@ void Do() SUT.Invoke(this, null); - Assert.AreEqual(5050, invoked); + Assert.AreEqual(5150, invoked); disposable.Clear(); From 4be9ddd3013a2a00d37c69f821a0b1a7cb9ea6de Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Mon, 25 Nov 2024 15:23:31 -0500 Subject: [PATCH 10/14] chore: Keep the delegate active, freeze invocation collection --- .../Tests/Windows_UI/Given_WeakEventHelper.cs | 2 +- src/Uno.UWP/UI/Core/WeakEventHelper.cs | 38 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs index 11bc0084cdcb..2d33b095c8f7 100644 --- a/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs +++ b/src/Uno.UI.RuntimeTests/Tests/Windows_UI/Given_WeakEventHelper.cs @@ -95,7 +95,7 @@ void Do() SUT.Invoke(this, null); - Assert.AreEqual(1, invoked); + Assert.AreEqual(2, invoked); } [TestMethod] diff --git a/src/Uno.UWP/UI/Core/WeakEventHelper.cs b/src/Uno.UWP/UI/Core/WeakEventHelper.cs index 806ef896a463..d60eaff42feb 100644 --- a/src/Uno.UWP/UI/Core/WeakEventHelper.cs +++ b/src/Uno.UWP/UI/Core/WeakEventHelper.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Runtime.InteropServices; using System.Text; using Uno.Buffers; using Uno.Disposables; @@ -94,9 +95,27 @@ public void Invoke(object sender, object? args) { lock (_lock) { - for (int i = 0; i < _handlers.Count; i++) + if (_handlers.Count == 1) { - _handlers[i].Handler(sender, args); + // Fast path for single registrations + var handler = _handlers[0]; + handler.Handler(sender, args); + } + else if (_handlers.Count > 1) + { + // Clone the array to account for reentrancy. + var handlers = ArrayPool.Shared.Rent(_handlers.Count); + _handlers.CopyTo(handlers, 0); + + // Use the original count, the rented array may be larger. + var count = _handlers.Count; + + for (int i = 0; i < count; i++) + { + handlers[i].Handler(sender, args); + } + + ArrayPool.Shared.Return(handlers); } } } @@ -105,8 +124,8 @@ public void Invoke(object sender, object? args) /// Do not use directly, use instead. /// Registers an handler to be called when the event is raised. /// - /// A disposable that can be used to unregister the provided handler. The disposable instance is not tracked by the GC and will not collect the registration. - internal IDisposable Register(WeakReference target, GenericEventHandler handler) + /// A disposable that can be used to unregister the provided handler. + internal IDisposable Register(WeakReference target, GenericEventHandler handler, Delegate actualTarget) { lock (_lock) { @@ -122,6 +141,13 @@ internal IDisposable Register(WeakReference target, GenericEventHandler handler) void RemoveRegistration() { + // Force a capture of the delegate by setting it to null. + // This will ensure that keeping the IDisposable alive will + // keep the registered original delegate alive. This does ensure + // that if the original delegate was provided as a lambda, the + // instance will stay active as long as the disposable instance. + actualTarget = null!; + lock (_lock) { _handlers.Remove(key); @@ -154,8 +180,6 @@ public void Dispose() /// /// If either the or the are /// collected, raising the event will produce nothing. - /// - /// The returned disposable instance itself is not tracked by the GC and will not collect the registration. /// internal static IDisposable RegisterEvent(WeakEventCollection list, Delegate handler, EventRaiseHandler raise) { @@ -178,7 +202,7 @@ internal static IDisposable RegisterEvent(WeakEventCollection list, Delegate han } }; - return list.Register(wr, genericHandler); + return list.Register(wr, genericHandler, handler); } } } From 58714bbb10cd3fa797a79666951438fc918f5f4b Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Mon, 25 Nov 2024 15:25:39 -0500 Subject: [PATCH 11/14] chore: Update doc --- doc/articles/uno-development/Internal-WeakEventHelper.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/articles/uno-development/Internal-WeakEventHelper.md b/doc/articles/uno-development/Internal-WeakEventHelper.md index ff854ad38c73..577abab03d40 100644 --- a/doc/articles/uno-development/Internal-WeakEventHelper.md +++ b/doc/articles/uno-development/Internal-WeakEventHelper.md @@ -19,7 +19,8 @@ return disposable. If the provided handler is collected, the registration will be collected as well. The returned disposable is not tracked, which means that it will -not remove the registration when collected. +not remove the registration when collected, unless the provided handler is a lambda. In +this case, the lambda's lifetime is tied to the returned disposable. The WeakEventCollection automatically manages its internal registration list using GC events. From 0627818b9927a2c6ca971a7d448152f60a28dccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Laban?= Date: Tue, 26 Nov 2024 08:23:43 -0500 Subject: [PATCH 12/14] chore: Adjust for arraypool possible leak --- src/Uno.UWP/UI/Core/WeakEventHelper.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Uno.UWP/UI/Core/WeakEventHelper.cs b/src/Uno.UWP/UI/Core/WeakEventHelper.cs index d60eaff42feb..478d1f660eae 100644 --- a/src/Uno.UWP/UI/Core/WeakEventHelper.cs +++ b/src/Uno.UWP/UI/Core/WeakEventHelper.cs @@ -112,7 +112,13 @@ public void Invoke(object sender, object? args) for (int i = 0; i < count; i++) { - handlers[i].Handler(sender, args); + ref var handler = ref handlers[i]; + + handler.Handler(sender, args); + + // Clear the handle immediately, so we don't + // call ArrayPool.Return with clear. + handler = null; } ArrayPool.Shared.Return(handlers); From f2ee537dedeb77d2402eda6923324fea7188f0e2 Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Tue, 26 Nov 2024 16:27:11 -0500 Subject: [PATCH 13/14] chore: undo unused change --- src/Uno.UI/Uno.UI.netcoremobile.csproj | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Uno.UI/Uno.UI.netcoremobile.csproj b/src/Uno.UI/Uno.UI.netcoremobile.csproj index 3bb7854e34ce..69a19239075a 100644 --- a/src/Uno.UI/Uno.UI.netcoremobile.csproj +++ b/src/Uno.UI/Uno.UI.netcoremobile.csproj @@ -46,6 +46,14 @@ + + True + True + + + True + True + True True @@ -136,8 +144,5 @@ - - - From 63ddc19f455cef1cdec2bbf075ab844af6f2b6ca Mon Sep 17 00:00:00 2001 From: Jerome Laban Date: Tue, 26 Nov 2024 16:21:31 -0500 Subject: [PATCH 14/14] chore: Simplify for loop --- src/Uno.UWP/UI/Core/WeakEventHelper.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Uno.UWP/UI/Core/WeakEventHelper.cs b/src/Uno.UWP/UI/Core/WeakEventHelper.cs index 478d1f660eae..9803c412b2dc 100644 --- a/src/Uno.UWP/UI/Core/WeakEventHelper.cs +++ b/src/Uno.UWP/UI/Core/WeakEventHelper.cs @@ -75,12 +75,11 @@ private bool Trim() { lock (_lock) { - for (int i = 0; i < _handlers.Count; i++) + for (int i = _handlers.Count - 1; i >= 0; i--) { if (!_handlers[i].Target.IsAlive) { _handlers.RemoveAt(i); - i--; } }