Merge pull request #8433 from AvaloniaUI/fixes/8389-datagrid-detach

Improve performance of style class selector subscriptions

src/Avalonia.Base/Controls/Classes.cs

@@ -14,6 +14,8 @@ namespace Avalonia.Controls
     /// </remarks>
     public class Classes : AvaloniaList<string>, IPseudoClasses
     {
+        private List<IClassesChangedListener>? _listeners;
+
         /// <summary>
         /// Initializes a new instance of the <see cref="Classes"/> class.
         /// </summary>
@@ -39,6 +41,11 @@ public Classes(params string[] items)
         {            
         }
 
+        /// <summary>
+        /// Gets the number of listeners subscribed to this collection for unit testing purposes.
+        /// </summary>
+        internal int ListenerCount => _listeners?.Count ?? 0;
+
         /// <summary>
         /// Parses a classes string.
         /// </summary>
@@ -62,6 +69,7 @@ public override void Add(string name)
             if (!Contains(name))
             {
                 base.Add(name);
+                NotifyChanged();
             }
         }
 
@@ -89,6 +97,7 @@ public override void AddRange(IEnumerable<string> names)
             }
 
             base.AddRange(c);
+            NotifyChanged();
         }
 
         /// <summary>
@@ -103,6 +112,8 @@ public override void Clear()
                     RemoveAt(i);
                 }
             }
+
+            NotifyChanged();
         }
 
         /// <summary>
@@ -122,6 +133,7 @@ public override void Insert(int index, string name)
             if (!Contains(name))
             {
                 base.Insert(index, name);
+                NotifyChanged();
             }
         }
 
@@ -154,6 +166,7 @@ public override void InsertRange(int index, IEnumerable<string> names)
             if (toInsert != null)
             {
                 base.InsertRange(index, toInsert);
+                NotifyChanged();
             }
         }
 
@@ -169,7 +182,14 @@ public override void InsertRange(int index, IEnumerable<string> names)
         public override bool Remove(string name)
         {
             ThrowIfPseudoclass(name, "removed");
-            return base.Remove(name);
+
+            if (base.Remove(name))
+            {
+                NotifyChanged();
+                return true;
+            }
+
+            return false;
         }
 
         /// <summary>
@@ -197,6 +217,7 @@ public override void RemoveAll(IEnumerable<string> names)
             if (toRemove != null)
             {
                 base.RemoveAll(toRemove);
+                NotifyChanged();
             }
         }
 
@@ -214,6 +235,7 @@ public override void RemoveAt(int index)
             var name = this[index];
             ThrowIfPseudoclass(name, "removed");
             base.RemoveAt(index);
+            NotifyChanged();
         }
 
         /// <summary>
@@ -224,6 +246,7 @@ public override void RemoveAt(int index)
         public override void RemoveRange(int index, int count)
         {
             base.RemoveRange(index, count);
+            NotifyChanged();
         }
 
         /// <summary>
@@ -255,6 +278,7 @@ public void Replace(IList<string> source)
             }
 
             base.AddRange(source);
+            NotifyChanged();
         }
 
         /// <inheritdoc/>
@@ -263,13 +287,38 @@ void IPseudoClasses.Add(string name)
             if (!Contains(name))
             {
                 base.Add(name);
+                NotifyChanged();
             }
         }
 
         /// <inheritdoc/>
         bool IPseudoClasses.Remove(string name)
         {
-            return base.Remove(name);
+            if (base.Remove(name))
+            {
+                NotifyChanged();
+                return true;
+            }
+
+            return false;
+        }
+
+        internal void AddListener(IClassesChangedListener listener)
+        {
+            (_listeners ??= new()).Add(listener);
+        }
+
+        internal void RemoveListener(IClassesChangedListener listener)
+        {
+            _listeners?.Remove(listener);
+        }
+
+        private void NotifyChanged()
+        {
+            if (_listeners is null)
+                return;
+            foreach (var listener in _listeners)
+                listener.Changed();
         }
 
         private void ThrowIfPseudoclass(string name, string operation)

src/Avalonia.Base/Controls/IClassesChangedListener.cs

@@ -0,0 +1,14 @@
+namespace Avalonia.Controls
+{
+    /// <summary>
+    /// Internal interface for listening to changes in <see cref="Classes"/> in a more
+    /// performant manner than subscribing to CollectionChanged.
+    /// </summary>
+    internal interface IClassesChangedListener
+    {
+        /// <summary>
+        /// Notifies the listener that the <see cref="Classes"/> collection has changed.
+        /// </summary>
+        void Changed();
+    }
+}

src/Avalonia.Base/Styling/Activators/StyleClassActivator.cs

@@ -1,6 +1,7 @@
 using System.Collections.Generic;
 using System.Collections.Specialized;
 using Avalonia.Collections;
+using Avalonia.Controls;
 
 #nullable enable
 
@@ -10,21 +11,17 @@ namespace Avalonia.Styling.Activators
     /// An <see cref="IStyleActivator"/> which is active when a set of classes match those on a
     /// control.
     /// </summary>
-    internal sealed class StyleClassActivator : StyleActivatorBase
+    internal sealed class StyleClassActivator : StyleActivatorBase, IClassesChangedListener
     {
         private readonly IList<string> _match;
-        private readonly IAvaloniaReadOnlyList<string> _classes;
-        private NotifyCollectionChangedEventHandler? _classesChangedHandler;
+        private readonly Classes _classes;
 
-        public StyleClassActivator(IAvaloniaReadOnlyList<string> classes, IList<string> match)
+        public StyleClassActivator(Classes classes, IList<string> match)
         {
             _classes = classes;
             _match = match;
         }
 
-        private NotifyCollectionChangedEventHandler ClassesChangedHandler =>
-            _classesChangedHandler ??= ClassesChanged;
-
         public static bool AreClassesMatching(IReadOnlyList<string> classes, IList<string> toMatch)
         {
             int remainingMatches = toMatch.Count;
@@ -55,23 +52,20 @@ public static bool AreClassesMatching(IReadOnlyList<string> classes, IList<strin
             return remainingMatches == 0;
         }
 
-        protected override void Initialize()
+        void IClassesChangedListener.Changed()
         {
             PublishNext(IsMatching());
-            _classes.CollectionChanged += ClassesChangedHandler;
         }
 
-        protected override void Deinitialize()
+        protected override void Initialize()
         {
-            _classes.CollectionChanged -= ClassesChangedHandler;
+            PublishNext(IsMatching());
+            _classes.AddListener(this);
         }
 
-        private void ClassesChanged(object? sender, NotifyCollectionChangedEventArgs e)
+        protected override void Deinitialize()
         {
-            if (e.Action != NotifyCollectionChangedAction.Move)
-            {
-                PublishNext(IsMatching());
-            }
+            _classes.RemoveListener(this);
         }
 
         private bool IsMatching() => AreClassesMatching(_classes, _match);

src/Avalonia.Base/Styling/TypeNameAndClassSelector.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Text;
+using Avalonia.Controls;
 using Avalonia.Styling.Activators;
 
 #nullable enable
@@ -125,7 +126,7 @@ protected override SelectorMatch Evaluate(IStyleable control, IStyle? parent, bo
             {
                 if (subscribe)
                 {
-                    var observable = new StyleClassActivator(control.Classes, _classes.Value);
+                    var observable = new StyleClassActivator((Classes)control.Classes, _classes.Value);
 
                     return new SelectorMatch(observable);
                 }

tests/Avalonia.Base.UnitTests/Styling/SelectorTests_Template.cs

@@ -142,14 +142,13 @@ public void Nested_Selector_Is_Unsubscribed()
             var border = (Border)target.Object.VisualChildren.Single();
             var selector = default(Selector).OfType(templatedControl.Object.GetType()).Class("foo").Template().OfType<Border>();
             var activator = selector.Match(border).Activator;
-            var inccDebug = (INotifyCollectionChangedDebug)styleable.Object.Classes;
 
             using (activator.Subscribe(_ => { }))
             {
-                Assert.Single(inccDebug.GetCollectionChangedSubscribers());
+                Assert.Equal(1, ((Classes)styleable.Object.Classes).ListenerCount);
             }
 
-            Assert.Null(inccDebug.GetCollectionChangedSubscribers());
+            Assert.Equal(0, ((Classes)styleable.Object.Classes).ListenerCount);
         }
 
         private void BuildVisualTree<T>(Mock<T> templatedControl) where T : class, IVisual

tests/Avalonia.Benchmarks/Styling/Style_ClassSelector.cs

@@ -0,0 +1,85 @@
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using Avalonia.Controls;
+using Avalonia.Styling;
+using BenchmarkDotNet.Attributes;
+
+#nullable enable
+
+namespace Avalonia.Benchmarks.Styling
+{
+    [MemoryDiagnoser]
+    public class Style_ClassSelector
+    {
+        private Style _style = null!;
+
+        public Style_ClassSelector()
+        {
+            RuntimeHelpers.RunClassConstructor(typeof(TestClass).TypeHandle);
+        }
+
+        [GlobalSetup]
+        public void Setup()
+        {
+            _style = new Style(x => x.OfType<TestClass>().Class("foo"))
+            {
+                Setters = { new Setter(TestClass.StringProperty, "foo") }
+            };
+        }
+
+        [Benchmark(OperationsPerInvoke = 50)]
+        public void Apply()
+        {
+            var target = new TestClass();
+
+            target.BeginBatchUpdate();
+
+            for (var i = 0; i < 50; ++i)
+                _style.TryAttach(target, null);
+
+            target.EndBatchUpdate();
+        }
+
+        [Benchmark(OperationsPerInvoke = 50)]
+        public void Apply_Toggle()
+        {
+            var target = new TestClass();
+
+            target.BeginBatchUpdate();
+
+            for (var i = 0; i < 50; ++i)
+                _style.TryAttach(target, null);
+
+            target.EndBatchUpdate();
+
+            target.Classes.Add("foo");
+            target.Classes.Remove("foo");
+        }
+
+        [Benchmark(OperationsPerInvoke = 50)]
+        public void Apply_Detach()
+        {
+            var target = new TestClass();
+
+            target.BeginBatchUpdate();
+
+            for (var i = 0; i < 50; ++i)
+                _style.TryAttach(target, null);
+
+            target.EndBatchUpdate();
+
+            target.DetachStyles();
+        }
+
+        private class TestClass : Control
+        {
+            public static readonly StyledProperty<string?> StringProperty =
+                AvaloniaProperty.Register<TestClass, string?>("String");
+            public void DetachStyles() => InvalidateStyles();
+        }
+
+        private class TestClass2 : Control
+        {
+        }
+    }
+}

tests/Avalonia.LeakTests/ControlTests.cs

@@ -314,7 +314,7 @@ public void TextBox_Class_Listeners_Are_Freed()
 
                 // The TextBox should have subscriptions to its Classes collection from the
                 // default theme.
-                Assert.NotEmpty(((INotifyCollectionChangedDebug)textBox.Classes).GetCollectionChangedSubscribers());
+                Assert.NotEqual(0, textBox.Classes.ListenerCount);
 
                 // Clear the content and ensure the TextBox is removed.
                 window.Content = null;