diff --git a/src/Reactor/Core/AccessibilityScanner.cs b/src/Reactor/Core/AccessibilityScanner.cs index a7242c51..11754c2f 100644 --- a/src/Reactor/Core/AccessibilityScanner.cs +++ b/src/Reactor/Core/AccessibilityScanner.cs @@ -117,6 +117,12 @@ public static partial class AccessibilityScanner // implementation and its dependency chain. private static IScanExtension? s_scanExtension; + // Reused per-thread scratch list for BuildContext's child extraction so a + // per-finding Where(...).ToList() is not allocated each call (perf #64). + // Cleared before and after use so it never pins Element references. + [ThreadStatic] + private static List? t_buildCtxChildren; + /// /// Registration hook for a subsystem-specific scanner extension (e.g. the /// Charting accessibility checker). Installed once when the subsystem @@ -175,7 +181,13 @@ private static void Walk(Element? el, ScanContext ctx, List find { if (el is null or EmptyElement) return; - ctx.Push(el); + // Compute children once and reuse for both sibling-text collection + // (inside Push) and the recursion below. GetChildren does a full + // switch dispatch and (for the single-child arms) allocates a fresh + // array literal on every call, so calling it twice per element + // doubles that cost (perf #62). + var children = GetChildren(el); + ctx.Push(el, children); // Per-element checks CheckIconButton(el, ctx, findings); @@ -196,7 +208,7 @@ private static void Walk(Element? el, ScanContext ctx, List find CollectLandmark(el, ctx); // Recurse into children - foreach (var child in GetChildren(el)) + foreach (var child in children) Walk(child, ctx, findings); ctx.Pop(); @@ -480,31 +492,43 @@ private static void CheckNoMainLandmark(ScanContext ctx, List fi /// A11Y_007: Non-sequential TabIndex values (gaps > 1). private static void CheckTabIndexGaps(ScanContext ctx, List findings) { - if (ctx.TabIndices.Count < 2) return; + int count = ctx.TabIndices.Count; + if (count < 2) return; - var sorted = ctx.TabIndices.OrderBy(t => t).ToList(); - for (int i = 1; i < sorted.Count; i++) + // Snapshot the set into a rented buffer and sort in place instead of + // OrderBy(...).ToList() (which allocates an iterator + a List) — perf #66. + var buffer = global::System.Buffers.ArrayPool.Shared.Rent(count); + try { - if (sorted[i] - sorted[i - 1] > 1) + ctx.TabIndices.CopyTo(buffer); + Array.Sort(buffer, 0, count); + for (int i = 1; i < count; i++) { - findings.Add(new A11yDiagnostic + if (buffer[i] - buffer[i - 1] > 1) { - Id = "A11Y_007", - Severity = "info", - Message = $"TabIndex gap: {sorted[i - 1]} → {sorted[i]}. Non-sequential values may confuse keyboard navigation order", - WcagCriterion = "2.1.1", - ElementType = "(tree-wide)", - Fix = new A11yFixSuggestion + findings.Add(new A11yDiagnostic { - Modifier = "TabIndex", - SuggestedValue = null, - CodeSnippet = "Renumber TabIndex values sequentially", - }, - Context = new A11yContext(), - }); - break; // Report only the first gap + Id = "A11Y_007", + Severity = "info", + Message = $"TabIndex gap: {buffer[i - 1]} → {buffer[i]}. Non-sequential values may confuse keyboard navigation order", + WcagCriterion = "2.1.1", + ElementType = "(tree-wide)", + Fix = new A11yFixSuggestion + { + Modifier = "TabIndex", + SuggestedValue = null, + CodeSnippet = "Renumber TabIndex values sequentially", + }, + Context = new A11yContext(), + }); + break; // Report only the first gap + } } } + finally + { + global::System.Buffers.ArrayPool.Shared.Return(buffer); + } } /// A11Y_008: .LabeledBy() references an AutomationId not found in the tree. @@ -584,18 +608,29 @@ private static bool IsAccessibilityHidden(Element el) => private static bool IsLikelyEmoji(string? text) { if (string.IsNullOrEmpty(text)) return false; + var span = text.AsSpan(); // Single character or very short string that's non-ASCII → likely emoji/icon - if (text.Length <= 2 && text.Any(c => c > 0x2000)) return true; - // Unicode symbol ranges commonly used for icons - return text.Length <= 4 && text.All(c => - c > 0x2000 || char.IsHighSurrogate(c) || char.IsLowSurrogate(c)); + if (span.Length <= 2) + { + foreach (var c in span) + if (c > 0x2000) return true; + } + // Unicode symbol ranges commonly used for icons: short string where + // every char is a high symbol or a surrogate half. + if (span.Length > 4) return false; + foreach (var c in span) + { + if (!(c > 0x2000 || char.IsHighSurrogate(c) || char.IsLowSurrogate(c))) + return false; + } + return true; } private static string Truncate(string text, int maxLen) => text.Length <= maxLen ? text : text[..(maxLen - 3)] + "..."; private static string[] GetSiblingTexts(ScanContext ctx) => - ctx.CurrentSiblingTexts.ToArray(); + ctx.SiblingTextsCached; // ════════════════════════════════════════════════════════════════ // Scan context (maintained during tree walk) @@ -615,6 +650,14 @@ private sealed class ScanContext : IScanContext // Current sibling context (texts of siblings in the same container) public readonly List CurrentSiblingTexts = new(); + // Cached array snapshot of CurrentSiblingTexts for the current frame. + // Multiple findings on the same element previously each allocated a + // fresh ToArray() copy; this caches one snapshot per element and + // invalidates it on Push (perf #63). The array is immutable once + // exposed, so sharing it across that element's findings is safe. + private string[]? _siblingTextsCache; + public string[] SiblingTextsCached => _siblingTextsCache ??= CurrentSiblingTexts.ToArray(); + public string? CurrentComponent => _stack.Count > 0 ? _stack.Peek().ComponentType : null; // ── IScanContext (issue #498): exposes the scanner's private helpers to @@ -623,7 +666,7 @@ private sealed class ScanContext : IScanContext bool IScanContext.HasAutomationName(Element el) => AccessibilityScanner.HasAutomationName(el); A11yContext IScanContext.BuildContext(Element el) => BuildContext(el, AccessibilityScanner.GetSiblingTexts(this)); - public void Push(Element el) + public void Push(Element el, IEnumerable children) { var frame = new ContextFrame(); @@ -651,15 +694,19 @@ public void Push(Element el) if (el is ComponentElement comp) frame.ComponentType = comp.ComponentType?.Name; - // Collect sibling texts from container children + // Collect sibling texts from the already-computed children + // (passed in by Walk so GetChildren runs once per element — #62). CurrentSiblingTexts.Clear(); - foreach (var child in GetChildren(el)) + foreach (var child in children) { if (child is TextBlockElement t) CurrentSiblingTexts.Add(t.Content); else if (child is ButtonElement b && !string.IsNullOrEmpty(b.Label)) CurrentSiblingTexts.Add(b.Label); } + // Invalidate the per-element sibling-text snapshot; it is + // recomputed lazily on first read for this frame (perf #63). + _siblingTextsCache = null; _stack.Push(frame); } @@ -674,20 +721,42 @@ public A11yContext BuildContext(Element el, string[] siblingTexts) { var frame = _stack.Count > 0 ? _stack.Peek() : new ContextFrame(); - // Extract child info - var children = GetChildren(el).Where(c => c is not null and not EmptyElement).ToList(); - string? childContent = null; + // Extract child info in a single pass over GetChildren(el), + // replacing the previous Where/Select/OfType LINQ chains (perf #64). + // A reused thread-static scratch list avoids the per-finding + // intermediate List allocation; the only allocation kept is the + // childTypes result array (pre-sized), which the LINQ path also built. + var children = t_buildCtxChildren ??= new List(); + children.Clear(); string? childText = null; + string? childContent = null; string[]? childTypes = null; - if (children.Count > 0) + // Wrap the scratch populate/use in try/finally so a mid-population + // exception can't leave element refs pinned on the thread-static list + // until the next BuildContext call on this thread (Copilot review). + try + { + foreach (var c in GetChildren(el)) + { + if (c is null or EmptyElement) continue; + children.Add(c); + if (childText is null && c is TextBlockElement tb) + childText = tb.Content; + } + + if (children.Count > 0) + { + childTypes = new string[children.Count]; + for (int i = 0; i < children.Count; i++) + childTypes[i] = children[i].GetType().Name; + if (children.Count == 1) + childContent = children[0].ToString(); + } + } + finally { - childTypes = children.Select(c => c!.GetType().Name).ToArray(); - var firstText = children.OfType().FirstOrDefault(); - if (firstText is not null) - childText = firstText.Content; - if (children.Count == 1) - childContent = children[0]!.ToString(); + children.Clear(); } // For ButtonElement, use ContentElement if present diff --git a/src/Reactor/Core/Context.cs b/src/Reactor/Core/Context.cs index d44c4924..70772980 100644 --- a/src/Reactor/Core/Context.cs +++ b/src/Reactor/Core/Context.cs @@ -8,6 +8,13 @@ namespace Microsoft.UI.Reactor.Core; public abstract class ContextBase { internal abstract object? DefaultValueBoxed { get; } + + /// + /// Compares this context's current value in against a + /// previously-recorded boxed value, without boxing the current value (perf #25). + /// Semantically equivalent to object.Equals(scope.Read(this), lastBoxed). + /// + internal abstract bool CurrentValueEquals(ContextScope scope, object? lastBoxed); } /// @@ -26,4 +33,33 @@ public Context(T defaultValue, [CallerMemberName] string? name = null) } internal override object? DefaultValueBoxed => DefaultValue; + + internal override bool CurrentValueEquals(ContextScope scope, object? lastBoxed) + { + var current = scope.Read(this); // strongly-typed read — no boxing of the current value + if (current is null) + return lastBoxed is null; + if (lastBoxed is null) + return false; + // For two non-null operands, object.Equals(current, lastBoxed) is exactly + // ReferenceEquals(current, lastBoxed) || current.Equals(lastBoxed) + // Reproduce that without boxing the freshly-read value (perf #25): + // * reference types — keep the ReferenceEquals fast-path: it covers the common + // "same instance, unchanged" context value AND the only case where a + // non-reflexive Equals override would otherwise diverge from object.Equals. + // The fall-back virtual Equals(object) preserves reference-equality change + // detection for a type implementing IEquatable WITHOUT an object.Equals + // override (a value-equality collapse there would skip a required rerender). + // * value types — the constrained call binds to the Equals(object) override, so + // primitives and Nullable compare without a box; a boxed value of a DIFFERENT + // type returns false rather than throwing, exactly as the prior boxed + // object.Equals did. ReferenceEquals is skipped — distinct boxes are never + // reference-equal, and testing it would needlessly box the current value. + // Routing value types through EqualityComparer.Default would instead dispatch to + // IEquatable, diverging from object.Equals for any struct whose IEquatable.Equals + // disagrees with its object.Equals override. + if (!typeof(T).IsValueType && ReferenceEquals(current, lastBoxed)) + return true; + return current.Equals(lastBoxed); + } } diff --git a/src/Reactor/Core/ContextScope.cs b/src/Reactor/Core/ContextScope.cs index 7666fc10..9d8dad4b 100644 --- a/src/Reactor/Core/ContextScope.cs +++ b/src/Reactor/Core/ContextScope.cs @@ -17,6 +17,16 @@ internal void Push(IReadOnlyDictionary values) _version++; } + /// + /// Single-pair push — avoids allocating a one-entry dictionary on the common + /// typed-context push path (perf #29). + /// + internal void Push(ContextBase context, object? value) + { + _stack.Add((context, value)); + _version++; + } + internal void Pop(int count) { _stack.RemoveRange(_stack.Count - count, count); diff --git a/src/Reactor/Core/Reconciler.KeyedItemsBinding.cs b/src/Reactor/Core/Reconciler.KeyedItemsBinding.cs index fad3e564..daa3cfa2 100644 --- a/src/Reactor/Core/Reconciler.KeyedItemsBinding.cs +++ b/src/Reactor/Core/Reconciler.KeyedItemsBinding.cs @@ -73,12 +73,20 @@ internal void BindKeyedItemsSource( ArgumentNullException.ThrowIfNull(buildItemView); ArgumentNullException.ThrowIfNull(requestRerender); - // Refresh the stashed view source on every call. The closure - // captures the *current* `items` + `buildItemView` references, so - // realizations + container refreshes after this point see the - // updated data. Cheap allocation; happens at most once per render. - var viewSource = new ClosureItemViewSource(items, buildItemView); - SetItemViewSource(control, viewSource); + // Refresh the stashed view source. The closure captures the *current* + // `items` + `buildItemView` references, so realizations + container + // refreshes after this point see the updated data. When neither + // reference changed since the last bind (parent re-rendered but this + // list's data + builder are stable) the previously stashed source is + // still exact — reuse it and skip both the allocation and the COM + // attached-DP stash write (perf #39). + ClosureItemViewSource? viewSource = + GetItemViewSource(control) as ClosureItemViewSource; + if (viewSource is null || !viewSource.Matches(items, buildItemView)) + { + viewSource = new ClosureItemViewSource(items, buildItemView); + SetItemViewSource(control, viewSource); + } switch (control) { @@ -581,5 +589,8 @@ public ClosureItemViewSource(IReadOnlyList items, Func _items.Count; public Element BuildItemView(int index) => _buildItemView(_items[index], index); + + public bool Matches(IReadOnlyList items, Func buildItemView) + => ReferenceEquals(_items, items) && ReferenceEquals(_buildItemView, buildItemView); } } diff --git a/src/Reactor/Core/Reconciler.Mount.cs b/src/Reactor/Core/Reconciler.Mount.cs index 2e621a28..7922143b 100644 --- a/src/Reactor/Core/Reconciler.Mount.cs +++ b/src/Reactor/Core/Reconciler.Mount.cs @@ -110,7 +110,10 @@ public sealed partial class Reconciler // Issue #345 — one-time debug warning for HStack/VStack collapsing to 0×0 inside a // Grid Auto track. Gated so Release builds with the flag off pay a single flag read. - if (global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.AlwaysOnInDebug || ReactorFeatureFlags.WarnLayoutFootguns) + // Inspect() is a no-op for non-GridElement, so pre-filter on type to skip the call + // (and its redundant internal flag re-check) for every non-Grid element (perf #40). + if ((global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.AlwaysOnInDebug || ReactorFeatureFlags.WarnLayoutFootguns) + && element is GridElement) global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.Inspect(element); // Apply inline modifiers after mounting @@ -625,13 +628,13 @@ private UIElement MountComponent(ComponentElement compElement, Action requestRer var node = new ComponentNode { Component = component, RenderedElement = null, Element = compElement, - PreviousProps = compElement.Props, + PreviousProps = compElement.Props, Control = wrapper, }; _componentNodes[wrapper] = node; // Pass the component's own wrapped rerender to children so that child state // changes propagate SelfTriggered up through all component ancestors. - var componentRerender = CreateComponentRerender(node, requestRerender); + var componentRerender = GetOrCreateComponentRerender(node, requestRerender); Element childElement; try @@ -658,13 +661,13 @@ private UIElement MountFuncComponent(FuncElement funcElement, Action requestRere var wrapper = new Border(); var node = new ComponentNode { - Context = ctx, RenderedElement = null, Element = funcElement, + Context = ctx, RenderedElement = null, Element = funcElement, Control = wrapper, }; _componentNodes[wrapper] = node; // Pass the component's own wrapped rerender to children so that child state // changes propagate SelfTriggered up through all component ancestors. - var componentRerender = CreateComponentRerender(node, requestRerender); + var componentRerender = GetOrCreateComponentRerender(node, requestRerender); Element childElement; try @@ -692,13 +695,13 @@ private UIElement MountMemoComponent(MemoElement memoElement, Action requestRere var node = new ComponentNode { Context = ctx, RenderedElement = null, Element = memoElement, - MemoDependencies = memoElement.Dependencies, + MemoDependencies = memoElement.Dependencies, Control = wrapper, }; _componentNodes[wrapper] = node; // Pass the component's own wrapped rerender to children so that child state // changes propagate SelfTriggered up through all component ancestors. - var componentRerender = CreateComponentRerender(node, requestRerender); + var componentRerender = GetOrCreateComponentRerender(node, requestRerender); Element childElement; try diff --git a/src/Reactor/Core/Reconciler.Update.cs b/src/Reactor/Core/Reconciler.Update.cs index e11cedd1..84f8539b 100644 --- a/src/Reactor/Core/Reconciler.Update.cs +++ b/src/Reactor/Core/Reconciler.Update.cs @@ -124,7 +124,8 @@ public sealed partial class Reconciler // an explicit size being dropped on an already-mounted stack. We are past the // structural-equality short-circuit above, so this only runs when something actually // changed; the emit-once dedup keyed on element identity prevents repeat spam. - if (global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.AlwaysOnInDebug || ReactorFeatureFlags.WarnLayoutFootguns) + if ((global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.AlwaysOnInDebug || ReactorFeatureFlags.WarnLayoutFootguns) + && newEl is GridElement) global::Microsoft.UI.Reactor.Diagnostics.LayoutFootgunDetector.Inspect(newEl); // Push context values onto scope before processing children @@ -185,6 +186,9 @@ public sealed partial class Reconciler // modifiers are null, pass an empty instance so ApplyModifiers can clear // stale values (same principle as the flex attached-property fix). var target = result ?? control; + // Coalesce the repeated `target is FrameworkElement` RCW type-tests below + // into a single cast (perf #14) — each `is` on a WinRT RCW crosses COM. + var targetFe = target as FrameworkElement; // Record the control for highlight overlay only when the element's own // WinUI properties were actually updated (not just children recursed). @@ -194,27 +198,27 @@ public sealed partial class Reconciler && _highlightModified is not null && (!Element.OwnPropsEqual(oldEl, newEl) || !Element.ModifiersEqual(oldModifiers, modifiers))) _highlightModified.Add(control); - if ((modifiers is not null || oldModifiers is not null) && target is FrameworkElement fe) - ApplyModifiers(fe, oldModifiers, modifiers ?? new ElementModifiers(), requestRerender); - if (target is FrameworkElement dragFe) - ApplyDragAttached(dragFe, newEl.GetAttached()); + if ((modifiers is not null || oldModifiers is not null) && targetFe is not null) + ApplyModifiers(targetFe, oldModifiers, modifiers, requestRerender); + if (targetFe is not null) + ApplyDragAttached(targetFe, newEl.GetAttached()); // Re-apply the caption-derived default after modifiers have run so a // label change ("+ 1" → "+ 2") updates UIA Name when the author never // set an explicit name. No-ops when the author did. - if (target is FrameworkElement captionFe) + if (targetFe is not null) UpdateDefaultAutomationName( - captionFe, + targetFe, ResolveCaptionForElement(oldEl), ResolveCaptionForElement(newEl)); // Apply theme-resource bindings (ThemeRef → resolved Brush from WinUI resources) - if (newEl.ThemeBindings is not null && target is FrameworkElement thFe) - ApplyThemeBindings(thFe, newEl.ThemeBindings); + if (newEl.ThemeBindings is not null && targetFe is not null) + ApplyThemeBindings(targetFe, newEl.ThemeBindings); // Apply per-control resource overrides (lightweight styling) - if ((newEl.ResourceOverrides is not null || oldEl.ResourceOverrides is not null) && target is FrameworkElement resFe) - ApplyResourceOverrides(resFe, oldEl.ResourceOverrides, newEl.ResourceOverrides); + if ((newEl.ResourceOverrides is not null || oldEl.ResourceOverrides is not null) && targetFe is not null) + ApplyResourceOverrides(targetFe, oldEl.ResourceOverrides, newEl.ResourceOverrides); // Apply transitions after update (re-applies when transition config changes) if (newEl.ImplicitTransitions is not null || newEl.ThemeTransitions is not null) @@ -1091,6 +1095,11 @@ internal void ReconcileChild(Element? oldChild, Element? newChild, /// // Internal so the V1-owned TemplatedListLifecycle can drive per-row content // reconciliation; also reused 1:1 by Reconciler.KeyedItemsBinding.cs. + // Reused snapshot buffer so the realized-container refresh doesn't allocate a + // List per call (perf #22; this runs ~once per list per frame). Borrowed for the + // duration of one refresh; a reentrant refresh (nested list) uses a fresh list. + private List? _realizedScratch = new(); + internal void RefreshRealizedContainers(WinUI.ListViewBase listViewBase, Internal.IItemViewSource viewSource, Action requestRerender) { var panel = listViewBase.ItemsPanelRoot; @@ -1099,36 +1108,54 @@ internal void RefreshRealizedContainers(WinUI.ListViewBase listViewBase, Interna // Snapshot first — Update may indirectly mount new controls and modifying // Children during enumeration throws (WinUI's UIElementCollection enforces // this). Counts are small (one viewport's worth) so the copy is cheap. - var realized = new List(panel.Children.Count); - foreach (var child in panel.Children) realized.Add(child); + var realized = _realizedScratch; + if (realized is null) + realized = new List(panel.Children.Count); + else + _realizedScratch = null; - foreach (var child in realized) + try { - // Cast to SelectorItem so both ListView (ListViewItem) and GridView - // (GridViewItem) containers are handled — both derive from SelectorItem - // and share the same ContentTemplateRoot pattern. - if (child is not Microsoft.UI.Xaml.Controls.Primitives.SelectorItem container) continue; - if (container.ContentTemplateRoot is not ContentControl cc) continue; + foreach (var child in panel.Children) realized.Add(child); - var index = listViewBase.IndexFromContainer(container); - if (index < 0 || index >= viewSource.ItemCount) continue; + foreach (var child in realized) + { + // Cast to SelectorItem so both ListView (ListViewItem) and GridView + // (GridViewItem) containers are handled — both derive from SelectorItem + // and share the same ContentTemplateRoot pattern. + if (child is not Microsoft.UI.Xaml.Controls.Primitives.SelectorItem container) continue; + if (container.ContentTemplateRoot is not ContentControl cc) continue; - var oldItemElement = GetElementTag(cc); - var newItemElement = viewSource.BuildItemView(index); + var index = listViewBase.IndexFromContainer(container); + if (index < 0 || index >= viewSource.ItemCount) continue; - if (oldItemElement is not null && cc.Content is UIElement existingCtrl && CanUpdate(oldItemElement, newItemElement)) - { - var replacement = Update(oldItemElement, newItemElement, existingCtrl, requestRerender); - if (replacement is not null && !ReferenceEquals(cc.Content, replacement)) - cc.Content = replacement; - } - else - { - if (cc.Content is UIElement oldCtrl) - Unmount(oldCtrl); - cc.Content = Mount(newItemElement, requestRerender); + var oldItemElement = GetElementTag(cc); + var newItemElement = viewSource.BuildItemView(index); + + if (oldItemElement is not null && cc.Content is UIElement existingCtrl && CanUpdate(oldItemElement, newItemElement)) + { + var replacement = Update(oldItemElement, newItemElement, existingCtrl, requestRerender); + if (replacement is not null && !ReferenceEquals(cc.Content, replacement)) + cc.Content = replacement; + } + else + { + if (cc.Content is UIElement oldCtrl) + Unmount(oldCtrl); + cc.Content = Mount(newItemElement, requestRerender); + } + SetElementTag(cc, newItemElement); } - SetElementTag(cc, newItemElement); + } + finally + { + realized.Clear(); + // perf #22 follow-up: don't pin an oversized backing array from a one-off + // large refresh — only retain the scratch at a modest capacity; otherwise + // let it be collected and re-allocate (sized to the viewport) next call. + const int realizedScratchRetainCap = 256; + if (realized.Capacity <= realizedScratchRetainCap) + _realizedScratch ??= realized; } } diff --git a/src/Reactor/Core/Reconciler.cs b/src/Reactor/Core/Reconciler.cs index fc2983b1..9386e921 100644 --- a/src/Reactor/Core/Reconciler.cs +++ b/src/Reactor/Core/Reconciler.cs @@ -83,6 +83,25 @@ public sealed partial class Reconciler : IDisposable typeof(Reconciler), new PropertyMetadata(null)); + // Effective theme (ActualTheme) captured the last time ApplyStyleToElement wrote + // a themed Style. Lets the apply guard (perf #23) skip the load-bearing + // null-then-set re-resolution when neither the Style ref nor the effective theme + // changed. ElementTheme.Default is never returned by ActualTheme, so it doubles + // as the "never applied" sentinel. + private static readonly DependencyProperty ReactorAppliedThemeProperty = + DependencyProperty.RegisterAttached( + "ReactorAppliedTheme", + typeof(Microsoft.UI.Xaml.ElementTheme), + typeof(Reconciler), + new PropertyMetadata(Microsoft.UI.Xaml.ElementTheme.Default)); + + // Reused scratch so the per-call cache-key build doesn't allocate a keys array + // + StringBuilder for every themed element update (perf #24). + [ThreadStatic] private static global::System.Text.StringBuilder? t_cacheKeySb; + [ThreadStatic] private static KeyValuePair[]? t_cacheKeyPairs; + private static readonly IComparer> s_themeBindingKeyComparer = + Comparer>.Create(static (a, b) => string.CompareOrdinal(a.Key, b.Key)); + /// /// Builds a deterministic cache key for a style based on its target type and /// the set of ThemeRef bindings. Keys are sorted by property name so that @@ -91,14 +110,34 @@ public sealed partial class Reconciler : IDisposable /// internal static string BuildCacheKey(string targetType, IReadOnlyDictionary bindings) { - // Format: "TargetType|Prop1=Key1|Prop2=Key2" with properties sorted by Ordinal - var sortedKeys = bindings.Keys.ToArray(); - Array.Sort(sortedKeys, StringComparer.Ordinal); - var sb = new global::System.Text.StringBuilder(targetType); - foreach (var key in sortedKeys) - { - sb.Append('|').Append(key).Append('=').Append(bindings[key].ResourceKey); - } + // Format: "TargetType|Prop1=Key1|Prop2=Key2" with properties sorted by Ordinal. + int count = bindings.Count; + if (count == 0) return targetType; + + var pairs = t_cacheKeyPairs; + if (pairs is null || pairs.Length < count) + pairs = t_cacheKeyPairs = new KeyValuePair[count < 4 ? 4 : count]; + int n = 0; + foreach (var kvp in bindings) + pairs[n++] = kvp; + Array.Sort(pairs, 0, count, s_themeBindingKeyComparer); + + var sb = t_cacheKeySb ??= new global::System.Text.StringBuilder(); + sb.Clear(); + sb.Append(targetType); + for (int i = 0; i < count; i++) + sb.Append('|').Append(pairs[i].Key).Append('=').Append(pairs[i].Value.ResourceKey); + + Array.Clear(pairs, 0, count); // drop refs so the retained scratch doesn't pin strings/ThemeRefs + // Retain both scratch buffers unconditionally (no size cap). They are sized to + // a single element's theme-binding count (pairs) and one key (sb), so the + // high-water is tiny and bounded by realistic per-element binding counts, and + // Array.Clear/sb.Clear already drop the referenced strings/ThemeRefs so nothing + // is pinned. A size cap was trialed but a multi-model perf cross-check showed + // discarding reallocates a KeyValuePair[] (2x a string[]) plus a fresh + // StringBuilder — i.e. WORSE than the pre-#24 baseline for an element that trips + // the cap every frame — whereas unconditional retain is strictly cheaper than + // that baseline (the dominant StringBuilder backing buffer is reused, not realloc'd). return sb.ToString(); } @@ -1199,6 +1238,15 @@ internal static void WireReferenceEdge( apply(ctrl, cell.Current); } + private static bool ContainsByRef( + List list, + Microsoft.UI.Reactor.Input.ElementRef item) + { + for (int i = 0; i < list.Count; i++) + if (ReferenceEquals(list[i], item)) return true; + return false; + } + internal static void WireReferenceListEdge( FrameworkElement ctrl, int slot, @@ -1211,13 +1259,15 @@ internal static void WireReferenceListEdge( edge.Clear = clearTarget; // retained so teardown can empty the target list (CR-002) edge.Handler ??= _ => edge.Recompute?.Invoke(ctrl); - var next = new List(); + // Dedupe input into `next` with a manual scan instead of LINQ Any(lambda), + // which allocated a capturing closure per cell (perf #27). + var next = new List(cells?.Count ?? 0); if (cells is not null) { foreach (var cell in cells) { if (cell is null) continue; - if (!next.Any(existing => ReferenceEquals(existing, cell))) + if (!ContainsByRef(next, cell)) next.Add(cell); } } @@ -1225,14 +1275,14 @@ internal static void WireReferenceListEdge( for (int i = edge.Cells.Count - 1; i >= 0; i--) { var existing = edge.Cells[i]; - if (next.Any(cell => ReferenceEquals(cell, existing))) continue; + if (ContainsByRef(next, existing)) continue; existing.CurrentChanged -= edge.Handler; edge.Cells.RemoveAt(i); } foreach (var cell in next) { - if (edge.Cells.Any(existing => ReferenceEquals(existing, cell))) continue; + if (ContainsByRef(edge.Cells, cell)) continue; edge.Cells.Add(cell); cell.CurrentChanged += edge.Handler; } @@ -1287,8 +1337,8 @@ internal static void TeardownReferenceEdges(FrameworkElement ctrl) /// internal IDisposable PushContextDisposable(Context context, T value) { - var dict = new Dictionary(1) { [context] = value }; - _contextScope.Push(dict); + // Single-pair push — no one-entry dictionary allocation (perf #29). + _contextScope.Push(context, value); return new PopOnDispose(_contextScope, 1); } @@ -1468,15 +1518,95 @@ public void Unmount(UIElement control, Reconciler reconciler) } } + // ── Self-triggered component tracking (perf #20) ─────────────────────────── + // Explicitly tracking the self-triggered nodes lets PopulateDirtyAncestorPath + // skip scanning every entry in _componentNodes each pass. The set is mutated + // from the rerender closure (which may run on a worker thread in headless + // hosts) as well as the UI-thread reconcile/unmount paths, so the flag write + // and the set membership change are kept atomic under _selfTriggeredLock — that + // way PopulateDirtyAncestorPath always observes a consistent (flag, set) view. + private readonly HashSet _selfTriggeredNodes = new(); + private readonly object _selfTriggeredLock = new(); + // Reused buffer so the dirty-path snapshot doesn't allocate per pass. + private readonly List _selfTriggeredScratch = new(); + + private void MarkSelfTriggered(ComponentNode node) + { + lock (_selfTriggeredLock) + { + // A stale cached rerender closure (perf #15) can fire after the node was + // unmounted; don't resurrect it into the tracking set or it would linger + // forever (the dirty-path pass only clears nodes it actually reconciles) and + // pin its captured component tree (correctness — review finding #20). + if (node.Unmounted) return; + node.SelfTriggered = true; + _selfTriggeredNodes.Add(node); + } + } + + private void ClearSelfTriggered(ComponentNode node, bool unmounting = false) + { + // Fast path: a volatile false read means no Mark has committed, so the node + // is not in the set — skip the lock entirely. This is the common case on the + // hot path (only the component that called setState is self-triggered). When + // unmounting we must still take the lock to plant the tombstone even if the + // node was never self-triggered. + if (!unmounting && !node.SelfTriggered) return; + lock (_selfTriggeredLock) + { + if (unmounting) + { + node.Unmounted = true; + // A cached rerender closure (perf #15) handed to a long-lived or cross-thread + // subscriber can outlive unmount and keep `node` alive; drop the wrapper-control + // reference so a leaked closure can't also pin the unmounted visual subtree. The + // dirty-path pass is the only reader of Control and never visits an unmounted node + // (it is removed from the set just below and MarkSelfTriggered refuses to re-add + // it), so this is observably a no-op — it only releases the subtree for GC. + node.Control = null; + } + node.SelfTriggered = false; + _selfTriggeredNodes.Remove(node); + } + } + + /// + /// Returns the component's wrapped rerender delegate, building it once and caching + /// it on the node (perf #15). A fresh closure is only created when the upstream + /// requestRerender identity changes, so steady-state re-renders reuse the delegate + /// instead of allocating one per component per frame. + /// + /// Internal (not private) so the caching contract can be unit-tested + /// directly via InternalsVisibleTo("Reactor.Tests"). + internal Action GetOrCreateComponentRerender(ComponentNode node, Action requestRerender) + { + if (node.CachedRerender is { } cached && ReferenceEquals(node.CachedRerenderSource, requestRerender)) + return cached; + var wrapped = CreateComponentRerender(this, node, requestRerender); + node.CachedRerender = wrapped; + node.CachedRerenderSource = requestRerender; + return wrapped; + } + private void PopulateDirtyAncestorPath() { - // Hot path — most renders have zero self-triggered nodes (the - // pass was triggered by a prop change higher up). Avoid the - // HashSet allocation entirely until we find one. + // Hot path — most renders have zero self-triggered nodes (the pass was + // triggered by a prop change higher up). Snapshot the tracked set (perf #20) + // under the lock into a reused buffer so the background-thread rerender path + // can't mutate it mid-walk, then walk the visual tree without holding the lock. + _selfTriggeredScratch.Clear(); + lock (_selfTriggeredLock) + { + foreach (var n in _selfTriggeredNodes) + _selfTriggeredScratch.Add(n); + } + HashSet? set = null; - foreach (var (control, node) in _componentNodes) + foreach (var node in _selfTriggeredScratch) { if (!node.SelfTriggered) continue; + var control = node.Control; + if (control is null) continue; set ??= _dirtyAncestorPath ?? new HashSet(); // Add the control itself first — Update on the wrapper // element that owns this control needs to bypass too so it @@ -1599,6 +1729,9 @@ internal bool TryAdoptRealizedReplacement(UIElement realized, UIElement replacem // inside ReconcileImperative; overwrite defensively regardless. _componentNodes.Remove(replacement); _componentNodes[realized] = freshNode; + // Keep the migrated node's cached control in sync so the self-triggered + // dirty-path pass (perf #20) resolves it to the still-parented wrapper. + freshNode.Control = realized; // Move the fresh visual subtree into the parented wrapper. Assigning // Border.Child detaches it from `replacementWrapper` first (and detaches the @@ -1624,7 +1757,7 @@ private void ReconcileComponent(Element oldEl, Element newEl, UIElement control, // ── Memo check: skip render if props/context unchanged and not self-triggered ── bool selfTriggered = node.SelfTriggered; - node.SelfTriggered = false; + ClearSelfTriggered(node); // Hot reload forces every component to re-run Render(); the new method // body lives only on the type, not in props/deps, so the memo gate @@ -1697,7 +1830,7 @@ private void ReconcileComponent(Element oldEl, Element newEl, UIElement control, // ── Render the component ── // Pass the component's own wrapped rerender to children so that child state // changes propagate SelfTriggered up through all component ancestors. - var componentRerender = CreateComponentRerender(node, requestRerender); + var componentRerender = GetOrCreateComponentRerender(node, requestRerender); // Only compute component name + timestamps when the Render keyword is // enabled, so the disabled path avoids the reflection and Stopwatch work. @@ -1821,7 +1954,7 @@ private void ReconcileComponent(Element oldEl, Element newEl, UIElement control, /// before invoking the parent requestRerender, so the memo check is bypassed. /// Captures the node directly to avoid accessing _componentNodes from background threads. /// - private static Action CreateComponentRerender(ComponentNode node, Action requestRerender) + private static Action CreateComponentRerender(Reconciler self, ComponentNode node, Action requestRerender) { return () => { @@ -1853,7 +1986,7 @@ private static Action CreateComponentRerender(ComponentNode node, Action request { uiDq.TryEnqueue(() => { - node.SelfTriggered = true; + self.MarkSelfTriggered(node); InvokeRerenderTracked(requestRerender); }); return; @@ -1861,7 +1994,7 @@ private static Action CreateComponentRerender(ComponentNode node, Action request // Either we're on the UI thread, or no UI DQ has been captured // (test/headless host) — run inline. - node.SelfTriggered = true; + self.MarkSelfTriggered(node); InvokeRerenderTracked(requestRerender); }; } @@ -1883,8 +2016,9 @@ private bool HasConsumedContextChanged(ComponentNode node) foreach (var ctxHook in renderCtx.ContextHooks) { - var currentValue = _contextScope.Read(ctxHook.Context); - if (!Equals(currentValue, ctxHook.LastValue)) + // Generic compare via Context — avoids boxing the current context + // value the non-generic Read + object.Equals path incurred (perf #25). + if (!ctxHook.Context.CurrentValueEquals(_contextScope, ctxHook.LastValue)) return true; } return false; @@ -1996,9 +2130,16 @@ internal void CleanupNavigationHostNode(UIElement control) private void UnmountRecursive(UIElement control) { + // Read the FrameworkElement RCW + attached Reactor element tag once. Every + // branch below otherwise re-crosses the COM boundary via GetElementTag + // (GetValue on ReactorAttached.StateProperty). The tag is stable through + // unmount (Clear*/TeardownReferenceEdges never null state.Element), so a + // single read is behavior-identical (perf #16). + var fe = control as FrameworkElement; + var tagEl = fe is not null ? GetElementTag(fe) : null; + // Capture connected animation snapshot while element is still in the visual tree - if (control is FrameworkElement caFe && GetElementTag(caFe) is Element caEl - && caEl.ConnectedAnimationKey is not null) + if (fe is not null && tagEl is { ConnectedAnimationKey: not null } caEl) { try { @@ -2009,7 +2150,7 @@ private void UnmountRecursive(UIElement control) } // Clean up animation state (mirrors UnmountAndCollect) - if (control is FrameworkElement animFe && GetElementTag(animFe) is Element animEl) + if (fe is not null && tagEl is Element animEl) { if (animEl.InteractionStates is not null) ClearInteractionStates(control); @@ -2023,14 +2164,14 @@ private void UnmountRecursive(UIElement control) // whether the element is tagged), and clear the producer ref when the element is // available. Unconditional so descriptor/binding referrers without a tag are also // cleaned up (CR-001 / CR-002). - if (control is FrameworkElement refFe) - CleanupReferenceStateForUnmount(refFe, GetElementTag(refFe)); + if (fe is not null) + CleanupReferenceStateForUnmount(fe, tagEl); // OnUnmountAction (.OnUnmount) — imperative teardown half of .OnMount. - if (control is FrameworkElement umFe && _onUnmountActions.TryGetValue(umFe, out var onUnmount)) + if (fe is not null && _onUnmountActions.TryGetValue(fe, out var onUnmount)) { - _onUnmountActions.Remove(umFe); - onUnmount(umFe); + _onUnmountActions.Remove(fe); + onUnmount(fe); } if (_componentNodes.TryGetValue(control, out var node)) @@ -2039,11 +2180,21 @@ private void UnmountRecursive(UIElement control) node.Component?.GetType().Name ?? node.Element?.GetType().Name ?? "unknown"); node.Component?.Context.RunCleanups(); node.Context?.RunCleanups(); + ClearSelfTriggered(node, unmounting: true); _componentNodes.Remove(control); } _errorBoundaryNodes.Remove(control); + // Re-read the attached element tag AFTER the user teardown callbacks above + // (.OnUnmount and the component/effect RunCleanups) ran. Those receive the live + // FrameworkElement and may detach/replace ReactorState via the public state APIs, + // so the V1/type unmount dispatch below must observe the post-cleanup tag — exactly + // as the pre-coalesce code did, which re-read GetElementTag at each of these sites. + // The earlier branches (connected-animation/animation-state/reference cleanup) run + // before any user callback and still share the single top-of-method read (perf #16). + var postCleanupTagEl = fe is not null ? GetElementTag(fe) : null; + // Spec 047 §14 Phase 1 (1.1) — V1 handler unmount dispatch. Standard // handlers return CollectSelf (terminate the traversal — children // already torn down by the handler; e.g. NavigationHostHandler.Unmount @@ -2052,7 +2203,7 @@ private void UnmountRecursive(UIElement control) // wrapped child whose control they returned (e.g., FlyoutElement returns // the Target's mounted control; the Target's children still need their // own unmount). - if (control is FrameworkElement v1Fe && GetElementTag(v1Fe) is Element v1TagEl + if (fe is not null && postCleanupTagEl is Element v1TagEl && _v1Handlers.TryGet(v1TagEl.GetType(), out var v1Entry) && v1Entry.HasUnmount) { var v1Disposition = v1Entry.Unmount(control, this); @@ -2061,8 +2212,8 @@ private void UnmountRecursive(UIElement control) } // Check registered type unmount handlers via the attached element - if (control is FrameworkElement fe && GetElementTag(fe) is Element tagEl - && _typeRegistry.TryGetValue(tagEl.GetType(), out var reg) && reg.HasUnmount) + if (fe is not null && postCleanupTagEl is Element regTagEl + && _typeRegistry.TryGetValue(regTagEl.GetType(), out var reg) && reg.HasUnmount) { reg.Unmount(control, this); return; @@ -2318,22 +2469,50 @@ internal void ReplaceChildWithExitTransition(IChildCollection children, int inde } } + // Reused scratch buffer so the common (non-reentrant) unmount doesn't allocate + // a List per call (perf #21). Borrowed for the duration of one UnmountAndPool; + // a reentrant unmount falls back to a fresh list. + private List? _unmountPoolScratch = new(); + internal void UnmountAndPool(UIElement control) { - var toPool = new List(); - UnmountAndCollect(control, toPool); + var toPool = _unmountPoolScratch; + if (toPool is null) + toPool = new List(); + else + _unmountPoolScratch = null; + + try + { + UnmountAndCollect(control, toPool); - // Pool top-down: parent's CleanElement calls Children.Clear() which - // detaches children, so by the time children are pooled they're parentless. - for (int i = 0; i < toPool.Count; i++) - _pool.Return(toPool[i]); + // Pool top-down: parent's CleanElement calls Children.Clear() which + // detaches children, so by the time children are pooled they're parentless. + for (int i = 0; i < toPool.Count; i++) + _pool.Return(toPool[i]); + } + finally + { + toPool.Clear(); + // perf #21 follow-up: mirror the _realizedScratch cap so a one-off unmount + // of a very large subtree doesn't pin an oversized backing array on the + // reconciler for the rest of its lifetime — only retain a modest scratch. + const int unmountScratchRetainCap = 256; + if (toPool.Capacity <= unmountScratchRetainCap) + _unmountPoolScratch ??= toPool; + } } private void UnmountAndCollect(UIElement control, List toPool) { + // Read the FrameworkElement RCW + attached element tag once (perf #17): + // every branch below otherwise repeats the GetElementTag COM read. The tag + // is stable through unmount, so a single read is behavior-identical. + var fe = control as FrameworkElement; + var tagEl = fe is not null ? GetElementTag(fe) : null; + // Capture connected animation snapshot while element is still in the visual tree - if (control is FrameworkElement caFe && GetElementTag(caFe) is Element caEl - && caEl.ConnectedAnimationKey is not null) + if (fe is not null && tagEl is { ConnectedAnimationKey: not null } caEl) { try { @@ -2344,7 +2523,7 @@ private void UnmountAndCollect(UIElement control, List toPool) } // Clean up animation state - if (control is FrameworkElement animFe && GetElementTag(animFe) is Element animEl) + if (fe is not null && tagEl is Element animEl) { if (animEl.InteractionStates is not null) ClearInteractionStates(control); @@ -2354,14 +2533,14 @@ private void UnmountAndCollect(UIElement control, List toPool) ClearScrollAnimation(control, animEl.ScrollAnimation); } - if (control is FrameworkElement refFe) - CleanupReferenceStateForUnmount(refFe, GetElementTag(refFe)); + if (fe is not null) + CleanupReferenceStateForUnmount(fe, tagEl); // OnUnmountAction (.OnUnmount) — imperative teardown half of .OnMount. - if (control is FrameworkElement umFe && _onUnmountActions.TryGetValue(umFe, out var onUnmount)) + if (fe is not null && _onUnmountActions.TryGetValue(fe, out var onUnmount)) { - _onUnmountActions.Remove(umFe); - onUnmount(umFe); + _onUnmountActions.Remove(fe); + onUnmount(fe); } // Run cleanup logic (component teardown, etc.) @@ -2371,9 +2550,17 @@ private void UnmountAndCollect(UIElement control, List toPool) node.Component?.GetType().Name ?? node.Element?.GetType().Name ?? "unknown"); node.Component?.Context.RunCleanups(); node.Context?.RunCleanups(); + ClearSelfTriggered(node, unmounting: true); _componentNodes.Remove(control); } + // Re-read the tag after the user teardown callbacks (.OnUnmount + RunCleanups), + // which may detach/replace ReactorState via the public state APIs — the V1/type + // dispatch below must observe the post-cleanup tag, matching the pre-coalesce code + // that re-read GetElementTag at each of these sites (perf #17). Earlier branches + // run before any user callback and keep sharing the single top-of-method read. + var postCleanupTagEl = fe is not null ? GetElementTag(fe) : null; + // Spec 047 §14 Phase 1 (1.1) — V1 handler unmount on the // UnmountAndCollect path. Standard handlers return CollectSelf // (mirrors the legacy registry behavior: pool the control, no @@ -2382,7 +2569,7 @@ private void UnmountAndCollect(UIElement control, List toPool) // to the default child traversal (ContinueDefaultTraversal — // wrapped child owns the control identity, default recursion // reaches the wrapped element's own unmount). - if (control is FrameworkElement v1Fe && GetElementTag(v1Fe) is Element v1TagEl + if (fe is not null && postCleanupTagEl is Element v1TagEl && _v1Handlers.TryGet(v1TagEl.GetType(), out var v1Entry) && v1Entry.HasUnmount) { var v1Disposition = v1Entry.Unmount(control, this); @@ -2399,8 +2586,8 @@ private void UnmountAndCollect(UIElement control, List toPool) } } - if (control is FrameworkElement fe && GetElementTag(fe) is Element tagEl - && _typeRegistry.TryGetValue(tagEl.GetType(), out var reg) && reg.HasUnmount) + if (fe is not null && postCleanupTagEl is Element regTagEl + && _typeRegistry.TryGetValue(regTagEl.GetType(), out var reg) && reg.HasUnmount) { reg.Unmount(control, this); // Collect this control for pooling, but do NOT recurse into children — @@ -2550,7 +2737,7 @@ internal bool TryHotReloadMigrateComponent(Element oldEl, Element newEl, UIEleme // changed the hook shape, reconciles the child element against the // preserved wrapper, and updates node.Element / node.PreviousProps. node.Component = newComponent; - node.SelfTriggered = true; + MarkSelfTriggered(node); ReconcileComponent(oldEl, newEl, existingControl, requestRerender); Diagnostics.ReactorEventSource.Log.HotReloadStateMigrated(newType.FullName ?? newType.Name); @@ -2690,6 +2877,12 @@ public static void UpdateDefaultAutomationName(FrameworkElement fe, string? oldC (oldCaption is null || !string.Equals(current, oldCaption, StringComparison.Ordinal)); if (authorOverride) return; var trimmed = newCaption.Length > 100 ? newCaption.Substring(0, 100) : newCaption; + // Skip the SetName COM write when the resolved name is already current + // (perf #18). This is the common steady-state case — a captioned cell whose + // caption text didn't change across re-renders. The GetName read above can't + // be skipped (it's needed to honor an author-set override + re-derive a + // name that was externally cleared), but a no-op write can. + if (string.Equals(current, trimmed, StringComparison.Ordinal)) return; Microsoft.UI.Xaml.Automation.AutomationProperties.SetName(fe, trimmed); } @@ -3331,6 +3524,9 @@ private static void TransitionToState(UIElement uie, InteractionStateTracker tra /// Applies stagger delays to an element's Visual's implicit animations. /// Called after layout/property animations are set up on children. /// + internal static readonly string[] s_staggerAnimationKeys = + { "Offset", "Opacity", "Scale", "RotationAngle", "Size", "CenterPoint" }; + internal static void ApplyStaggerDelays(UIElement parent, StaggerConfig config) { if (parent is not WinUI.Panel panel) return; @@ -3342,7 +3538,7 @@ internal static void ApplyStaggerDelays(UIElement parent, StaggerConfig config) if (visual.ImplicitAnimations is null) continue; var delay = TimeSpan.FromTicks(config.Delay.Ticks * i); - foreach (var key in new[] { "Offset", "Opacity", "Scale", "RotationAngle", "Size", "CenterPoint" }) + foreach (var key in s_staggerAnimationKeys) { try { @@ -3583,8 +3779,16 @@ internal void ApplyModifiers(FrameworkElement fe, ElementModifiers m, Action req private static void ApplyDragAttached(FrameworkElement fe, DragAttached? attached) => DragAttachedProperties.SetIsDragEnabled(fe, attached?.IsEnabled); - internal void ApplyModifiers(FrameworkElement fe, ElementModifiers? oldM, ElementModifiers m, Action requestRerender) + private static readonly ElementModifiers s_emptyModifiers = new(); + + internal void ApplyModifiers(FrameworkElement fe, ElementModifiers? oldM, ElementModifiers? m, Action requestRerender) { + // A null `m` means "clear all" — substitute a shared empty instance so the + // clear-modifiers update path doesn't allocate a throwaway ElementModifiers + // per call (perf #19). ApplyModifiers only ever reads from `m`, so sharing + // a singleton is safe. + m ??= s_emptyModifiers; + // Guard each property: only call into WinUI when the value actually changed. // Each WinUI property set is a managed→native interop call, so avoiding // unnecessary sets is critical for large element counts. @@ -4721,6 +4925,12 @@ internal static void ClearThemeBindings(FrameworkElement fe) if (ReferenceEquals(fe.Style, ownedStyle)) fe.ClearValue(FrameworkElement.StyleProperty); fe.ClearValue(ReactorAppliedThemeStyleProperty); + // Clear the companion effective-theme marker too (written together with the + // style marker in ApplyStyleToElement) so the pair stays consistent across a + // pool/reuse cycle (#23 / AS1). Harmless either way — the perf-#23 skip guard is + // gated on the style marker above, so a lingering theme marker could at most + // force an extra (correct) null-then-set, never skip a needed re-resolution. + fe.ClearValue(ReactorAppliedThemeProperty); } /// @@ -4731,6 +4941,22 @@ internal static void ClearThemeBindings(FrameworkElement fe) /// private static void ApplyStyleToElement(FrameworkElement fe, Style cachedStyle) { + // The null-then-set dance forces WinUI to re-resolve {ThemeResource} setters + // against the element's current effective theme (which can change via a parent + // RequestedTheme override even when the same cached Style ref is re-applied). + // That re-resolution only matters when either the applied Style changed or the + // effective theme changed since we last wrote it. When BOTH are unchanged the + // clear+set is a guaranteed no-op, so skip the two COM DP writes + visual-state + // re-eval (perf #23) — the dominant case on a steady-state data re-render. + var currentTheme = fe.ActualTheme; + if (ReferenceEquals(fe.Style, cachedStyle) + && ReferenceEquals(fe.GetValue(ReactorAppliedThemeStyleProperty), cachedStyle) + && fe.GetValue(ReactorAppliedThemeProperty) is Microsoft.UI.Xaml.ElementTheme appliedTheme + && appliedTheme == currentTheme) + { + return; + } + // Clearing first forces WinUI to process the subsequent set as a // genuine change, re-resolving {ThemeResource} values. Without this, // re-applying the same cached Style reference is a no-op. @@ -4742,6 +4968,7 @@ private static void ApplyStyleToElement(FrameworkElement fe, Style cachedStyle) // #522). Always overwrite so an A → B ThemeBindings transition // updates the marker to the new Style. fe.SetValue(ReactorAppliedThemeStyleProperty, cachedStyle); + fe.SetValue(ReactorAppliedThemeProperty, currentTheme); } private static string? GetStyleTargetType(FrameworkElement fe) => fe switch @@ -4798,13 +5025,27 @@ public static void ApplyResourceOverrides( // Track which keys Reactor has set on this element var managed = _managedResourceKeys.GetOrCreateValue(fe); - // Remove old keys that are no longer present in the new overrides - if (oldOverrides is not null) - { - var newKeys = newOverrides?.AllKeys.ToHashSet() ?? new HashSet(); - foreach (var key in managed.ToArray()) + // Remove managed keys that are no longer present in the new overrides. Membership + // is tested directly against the new Literals/ThemeRefs dictionaries (O(1) + // ContainsKey) instead of materializing AllKeys.ToHashSet(), and removals are + // deferred into a lazily-allocated list so the common no-removal case doesn't + // allocate managed.ToArray() (perf #26). Note: removal is driven by `managed` + // (the keys actually applied to this control) vs `newOverrides`, NOT by an + // old-vs-new ref comparison — the shallow-skip fast path calls this with + // old===new yet still needs stale managed keys reconciled away. + if (oldOverrides is not null && managed.Count > 0) + { + List? toRemove = null; + foreach (var key in managed) + { + bool stillPresent = newOverrides is not null + && (newOverrides.Literals.ContainsKey(key) || newOverrides.ThemeRefs.ContainsKey(key)); + if (!stillPresent) + (toRemove ??= new List()).Add(key); + } + if (toRemove is not null) { - if (!newKeys.Contains(key)) + foreach (var key in toRemove) { fe.Resources.Remove(key); managed.Remove(key); @@ -4995,6 +5236,22 @@ internal class ComponentNode /// Accessed from background threads (UseState callbacks) — use volatile field. private volatile bool _selfTriggered; public bool SelfTriggered { get => _selfTriggered; set => _selfTriggered = value; } + /// The component's Border wrapper — the key this node is registered under + /// in _componentNodes. Cached so the self-triggered dirty-path pass (perf #20) + /// can resolve the control without scanning the dictionary; kept in sync on re-key. + public UIElement? Control { get; set; } + /// Cached wrapped rerender delegate (perf #15). Rebuilt only when the upstream + /// requestRerender identity changes, so steady-state re-renders of a stateful row/cell + /// don't allocate a fresh closure every frame. + public Action? CachedRerender { get; set; } + /// The requestRerender the cached delegate was built from. + public Action? CachedRerenderSource { get; set; } + /// Tombstone set when the node is unmounted. A cached rerender closure + /// (perf #15) can outlive the node and fire after teardown; this flag lets + /// refuse to resurrect an unmounted node into + /// _selfTriggeredNodes (which would never be cleared again and would pin + /// the node's captured component tree). Mutated only under _selfTriggeredLock. + public bool Unmounted { get; set; } } /// diff --git a/tests/Reactor.Tests/ReconcilerInteropPerfTests.cs b/tests/Reactor.Tests/ReconcilerInteropPerfTests.cs new file mode 100644 index 00000000..a308f9a0 --- /dev/null +++ b/tests/Reactor.Tests/ReconcilerInteropPerfTests.cs @@ -0,0 +1,442 @@ +using System.Collections.Generic; +using System.Linq; +using Microsoft.UI.Reactor.Core; +using static Microsoft.UI.Reactor.Factories; +using Xunit; + +namespace Microsoft.UI.Reactor.Tests; + +/// +/// Behavior-parity unit tests for the reconcile-path COM-interop + allocation +/// perf fixes (issue: "perf: coalesce COM-interop reads + allocations in +/// reconcile path"). Each optimization replaced a per-element allocation or a +/// repeated COM read with a reused/coalesced equivalent; these tests pin the +/// observable behavior the optimizations must preserve: +/// +/// * #24 BuildCacheKey reuses a thread-static scratch buffer + StringBuilder +/// across calls — verify no stale entries leak and the format is stable. +/// * #25 Context<T>.CurrentValueEquals reproduces the previous +/// object.Equals semantics without boxing the current typed value. +/// * #29 ContextScope.Push(context, value) single-pair overload matches the +/// dictionary-based push it replaced. +/// * #62-#67 the refactored accessibility scanner walk emits identical +/// findings (output parity / re-scan determinism). +/// +public class ReconcilerInteropPerfTests +{ + // ════════════════════════════════════════════════════════════════ + // #24 — BuildCacheKey thread-static scratch reuse + // ════════════════════════════════════════════════════════════════ + + [Fact] + public void BuildCacheKey_LargeThenSmall_NoStaleTailLeaks() + { + var many = new Dictionary + { + { "Foreground", new ThemeRef("Fg") }, + { "Background", new ThemeRef("Bg") }, + { "BorderBrush", new ThemeRef("Border") }, + }; + // Ordinal sort: Background < BorderBrush < Foreground. + Assert.Equal( + "Button|Background=Bg|BorderBrush=Border|Foreground=Fg", + Reconciler.BuildCacheKey("Button", many)); + + // A subsequent smaller binding set reuses the same (larger) scratch + // buffer — the key must not pick up leftovers from the prior call. + var few = new Dictionary { { "Background", new ThemeRef("Bg") } }; + Assert.Equal("Button|Background=Bg", Reconciler.BuildCacheKey("Button", few)); + + // And an empty set must collapse to just the target type. + Assert.Equal("Button", Reconciler.BuildCacheKey("Button", new Dictionary())); + } + + [Fact] + public void BuildCacheKey_RepeatedCalls_AreDeterministic() + { + var bindings = new Dictionary + { + { "Foreground", new ThemeRef("TextPrimary") }, + { "Background", new ThemeRef("AccentBrush") }, + }; + + var expected = "Grid|Background=AccentBrush|Foreground=TextPrimary"; + for (int i = 0; i < 100; i++) + Assert.Equal(expected, Reconciler.BuildCacheKey("Grid", bindings)); + } + + // ════════════════════════════════════════════════════════════════ + // #25 — Context.CurrentValueEquals (no boxing of the current value) + // ════════════════════════════════════════════════════════════════ + + private static readonly Context CountCtx = new(0); + private static readonly Context ThemeCtx = new("light"); + private static readonly Context SessionCtx = new(defaultValue: null); + + [Fact] + public void CurrentValueEquals_ValueType_EqualAndDifferent() + { + var scope = new ContextScope(); + scope.Push(new Dictionary { [CountCtx] = 7 }); + + Assert.True(CountCtx.CurrentValueEquals(scope, 7)); // unchanged + Assert.False(CountCtx.CurrentValueEquals(scope, 8)); // changed + Assert.False(CountCtx.CurrentValueEquals(scope, null)); // current 7 vs null + + scope.Pop(1); + } + + [Fact] + public void CurrentValueEquals_Default_ReadsDefaultValue() + { + var scope = new ContextScope(); + // No push → reads the context default (0). Matches object.Equals(0, 0). + Assert.True(CountCtx.CurrentValueEquals(scope, 0)); + Assert.False(CountCtx.CurrentValueEquals(scope, 5)); + } + + [Fact] + public void CurrentValueEquals_ReferenceType_NullHandling() + { + var scope = new ContextScope(); + + // Default session value is null → current null. + Assert.True(SessionCtx.CurrentValueEquals(scope, null)); // null == null + Assert.False(SessionCtx.CurrentValueEquals(scope, "abc")); // null vs "abc" + + scope.Push(new Dictionary { [SessionCtx] = "abc" }); + Assert.True(SessionCtx.CurrentValueEquals(scope, "abc")); // unchanged + Assert.False(SessionCtx.CurrentValueEquals(scope, null)); // "abc" vs null + scope.Pop(1); + } + + // ════════════════════════════════════════════════════════════════ + // #29 — ContextScope.Push(context, value) single-pair overload + // ════════════════════════════════════════════════════════════════ + + [Fact] + public void SinglePairPush_ReadsValue_ThenPopRestoresDefault() + { + var scope = new ContextScope(); + scope.Push(ThemeCtx, "dark"); + Assert.Equal("dark", scope.Read(ThemeCtx)); + + scope.Pop(1); + Assert.Equal("light", scope.Read(ThemeCtx)); // default + } + + [Fact] + public void SinglePairPush_Nested_InnerShadowsOuter() + { + var scope = new ContextScope(); + scope.Push(ThemeCtx, "dark"); + scope.Push(ThemeCtx, "high-contrast"); + + Assert.Equal("high-contrast", scope.Read(ThemeCtx)); + scope.Pop(1); + Assert.Equal("dark", scope.Read(ThemeCtx)); + scope.Pop(1); + Assert.Equal("light", scope.Read(ThemeCtx)); + } + + [Fact] + public void SinglePairPush_MatchesDictionaryPush() + { + var single = new ContextScope(); + single.Push(ThemeCtx, "dark"); + + var dict = new ContextScope(); + dict.Push(new Dictionary { [ThemeCtx] = "dark" }); + + Assert.Equal(dict.Read(ThemeCtx), single.Read(ThemeCtx)); + } + + // ════════════════════════════════════════════════════════════════ + // #62-#67 — accessibility scanner output parity + // ════════════════════════════════════════════════════════════════ + + private static Element BuildScanTree() => VStack( + Button(TextBlock("🔍"), null), // A11Y_001: icon-only button + Image("ms-appx:///Assets/photo.png"), // A11Y_002: image without name + Button("A").TabIndex(1), + Button("B").TabIndex(7)); // A11Y_007: tab-index gap 1 → 7 + + [Fact] + public void Scanner_ReScan_ProducesIdenticalFindings() + { + var first = AccessibilityScanner.Scan(BuildScanTree()); + var second = AccessibilityScanner.Scan(BuildScanTree()); + + // The refactored GetChildren-once walk, sibling-array cache and + // single-pass BuildContext must emit byte-identical findings. + var firstKeys = first.Select(f => $"{f.Id}|{f.Message}").OrderBy(s => s, StringComparer.Ordinal); + var secondKeys = second.Select(f => $"{f.Id}|{f.Message}").OrderBy(s => s, StringComparer.Ordinal); + Assert.Equal(firstKeys, secondKeys); + + // Spot-check the expected findings are actually present. + Assert.Contains(first, f => f.Id == "A11Y_001"); + Assert.Contains(first, f => f.Id == "A11Y_002"); + Assert.Contains(first, f => f.Id == "A11Y_007"); + } + + [Fact] + public void Scanner_EmitsFindings_InStableWalkOrder() + { + // #62-66 refactored the GetChildren-once walk, sibling cache and single-pass + // BuildContext. Scanner_ReScan_ProducesIdenticalFindings sorts before comparing, + // so it cannot catch an emission-ORDER regression. Pin the natural walk order: + // per-element findings emit depth-first during Walk (icon button, then image), + // and the tree-wide tab-index-gap check emits afterward in the post-walk phase. + var ids = AccessibilityScanner.Scan(BuildScanTree()) + .Select(f => f.Id) + .Where(id => id is "A11Y_001" or "A11Y_002" or "A11Y_007") + .ToArray(); + + Assert.Equal(new[] { "A11Y_001", "A11Y_002", "A11Y_007" }, ids); + } + + [Fact] + public void Scanner_IconButton_StillCarriesChildTypeContext() + { + // Exercises the single-pass BuildContext child extraction (#64). + var findings = AccessibilityScanner.Scan(VStack(Button(TextBlock("🔍"), null))); + + var iconFinding = findings.FirstOrDefault(f => f.Id == "A11Y_001"); + Assert.NotNull(iconFinding); + Assert.NotNull(iconFinding!.Context.ChildTypes); + Assert.Contains("TextBlockElement", iconFinding.Context.ChildTypes!); + } + + [Fact] + public void Scanner_SequentialTabIndex_NoGapFinding() + { + // Exercises the Array.Sort-over-rented-buffer tab-index scan (#66). + var findings = AccessibilityScanner.Scan(VStack( + Button("A").TabIndex(1), + Button("B").TabIndex(2), + Button("C").TabIndex(3))); + + Assert.DoesNotContain(findings, f => f.Id == "A11Y_007"); + } + + [Fact] + public void Scanner_EmitsExactExpectedMessages() + { + // Pin the literal finding messages (not just IDs / re-scan determinism) so a + // regression in the refactored walk that altered every scan identically would + // still be caught (#62-#66 output parity). + var findings = AccessibilityScanner.Scan(BuildScanTree()); + + Assert.Contains(findings, f => + f.Id == "A11Y_001" && + f.Message == "Icon-only Button has no accessible name — screen readers cannot describe this control"); + Assert.Contains(findings, f => + f.Id == "A11Y_002" && + f.Message == "Image has no accessible name and is not hidden from assistive technology"); + Assert.Contains(findings, f => + f.Id == "A11Y_007" && + f.Message == "TabIndex gap: 1 → 7. Non-sequential values may confuse keyboard navigation order"); + } + + // ════════════════════════════════════════════════════════════════ + // #25 (H1) — reference types implementing IEquatable WITHOUT an + // object.Equals override keep the prior reference-equality change + // detection. A value-equality collapse here would skip a required + // context rerender, so the hybrid compare must NOT route ref types + // through EqualityComparer.Default. + // ════════════════════════════════════════════════════════════════ + + private sealed class RefId : global::System.IEquatable + { + private readonly int _v; + public RefId(int v) => _v = v; + // IEquatable value equality, but intentionally NO object.Equals/GetHashCode + // override — mirrors a hand-rolled ref type used as a context value. + public bool Equals(RefId? other) => other is not null && other._v == _v; + } + + private static readonly Context RefIdCtx = new(new RefId(0)); + + [Fact] + public void CurrentValueEquals_ReferenceIEquatable_UsesReferenceEquality() + { + var a = new RefId(1); + var b = new RefId(1); // IEquatable-equal to a, but a distinct instance + + var scope = new ContextScope(); + scope.Push(new Dictionary { [RefIdCtx] = b }); + + // The old object.Equals path was the virtual object.Equals (reference equality + // for this type), so a freshly-built-but-equal instance counts as CHANGED. + Assert.False(RefIdCtx.CurrentValueEquals(scope, a)); + // The same reference is unchanged. + Assert.True(RefIdCtx.CurrentValueEquals(scope, b)); + + scope.Pop(1); + } + + // A reference type whose object.Equals is NON-reflexive (returns false even for the + // same instance). object.Equals(x, x) short-circuits via ReferenceEquals and returns + // true, so the context compare must keep that fast-path, NOT call x.Equals(x). + private sealed class NonReflexive + { + public override bool Equals(object? obj) => false; + public override int GetHashCode() => 0; + } + + private static readonly Context NonReflexiveCtx = new(new NonReflexive()); + + [Fact] + public void CurrentValueEquals_ReferenceType_SameInstance_UsesReferenceEqualityFastPath() + { + var x = new NonReflexive(); + var scope = new ContextScope(); + scope.Push(new Dictionary { [NonReflexiveCtx] = x }); + + // Same instance recorded and current: object.Equals(x, x) == true via the + // ReferenceEquals short-circuit even though x.Equals(x) == false. Match it exactly. + Assert.Equal(object.Equals(x, x), NonReflexiveCtx.CurrentValueEquals(scope, x)); + Assert.True(NonReflexiveCtx.CurrentValueEquals(scope, x)); + + scope.Pop(1); + } + + [Fact] + public void CurrentValueEquals_ValueType_TypeMismatch_ReturnsFalseWithoutThrowing() + { + // A same-slot context-type swap can leave a boxed value of a DIFFERENT type as + // the recorded LastValue. The old object.Equals returned false there (it never + // threw); current.Equals(lastBoxed) — the Equals(object) override — does the same + // (int.Equals(object) returns false for a non-int), not an InvalidCastException. + var scope = new ContextScope(); + scope.Push(new Dictionary { [CountCtx] = 7 }); + + Assert.False(CountCtx.CurrentValueEquals(scope, "not-an-int")); + + scope.Pop(1); + } + + // A struct whose IEquatable.Equals INTENTIONALLY disagrees with its object.Equals + // override: object.Equals compares Value AND Tag; IEquatable ignores Tag. The context + // change-detection must follow object.Equals (the documented pre-#25 semantics), NOT + // IEquatable — otherwise EqualityComparer.Default would mask a real Tag change and + // skip a required rerender. + private readonly struct SplitEq : global::System.IEquatable + { + public readonly int Value; + public readonly int Tag; + public SplitEq(int value, int tag) { Value = value; Tag = tag; } + + public bool Equals(SplitEq other) => Value == other.Value; // ignores Tag + public override bool Equals(object? obj) => obj is SplitEq o && o.Value == Value && o.Tag == Tag; + public override int GetHashCode() => Value; + } + + private static readonly Context SplitEqCtx = new(default); + + [Fact] + public void CurrentValueEquals_ValueIEquatable_UsesObjectEqualsNotIEquatable() + { + // Current value = (Value 5, Tag 1). + var scope = new ContextScope(); + scope.Push(new Dictionary { [SplitEqCtx] = new SplitEq(5, 1) }); + + // Same Value, DIFFERENT Tag: object.Equals => CHANGED. EqualityComparer.Default + // (IEquatable) would wrongly say UNCHANGED. Assert we match object.Equals exactly. + Assert.Equal( + object.Equals(new SplitEq(5, 1), (object)new SplitEq(5, 2)), + SplitEqCtx.CurrentValueEquals(scope, new SplitEq(5, 2))); + Assert.False(SplitEqCtx.CurrentValueEquals(scope, new SplitEq(5, 2))); + + // Fully equal (Value AND Tag) is unchanged. + Assert.True(SplitEqCtx.CurrentValueEquals(scope, new SplitEq(5, 1))); + + scope.Pop(1); + } + + private static readonly Context NullableCountCtx = new(defaultValue: null); + + [Fact] + public void CurrentValueEquals_NullableValueType_BoxedUnderlying_MatchesExactly() + { + // Guards the boxed-Nullable edge: a non-null int? boxes to a boxed underlying + // int, and Nullable.Equals(object) (the constrained-call target) unwraps and + // compares it, so an UNCHANGED nullable context value is not spuriously treated as + // changed. + var scope = new ContextScope(); + scope.Push(new Dictionary { [NullableCountCtx] = 5 }); + + Assert.True(NullableCountCtx.CurrentValueEquals(scope, 5)); // unchanged (boxed int 5) + Assert.False(NullableCountCtx.CurrentValueEquals(scope, 6)); // changed + Assert.False(NullableCountCtx.CurrentValueEquals(scope, null)); // 5 vs null + scope.Pop(1); + + // Default (null) current vs a null record is unchanged. + Assert.True(NullableCountCtx.CurrentValueEquals(scope, null)); + } + + // ════════════════════════════════════════════════════════════════ + // #15 — component rerender delegate caching + // (and #20 — unmount tombstone refuses a late self-trigger) + // ════════════════════════════════════════════════════════════════ + + [Fact] + public void GetOrCreateComponentRerender_ReusesDelegate_UntilSourceIdentityChanges() + { + var reconciler = new Reconciler(); + var node = new Reconciler.ComponentNode(); + + global::System.Action source = () => { }; + var first = reconciler.GetOrCreateComponentRerender(node, source); + var second = reconciler.GetOrCreateComponentRerender(node, source); + + // Same upstream requestRerender ⇒ same wrapped delegate (no per-frame closure alloc). + Assert.Same(first, second); + Assert.Same(source, node.CachedRerenderSource); + + // A new upstream identity ⇒ a freshly built wrapper, and the source is updated. + global::System.Action source2 = () => { }; + var third = reconciler.GetOrCreateComponentRerender(node, source2); + Assert.NotSame(first, third); + Assert.Same(source2, node.CachedRerenderSource); + } + + [Fact] + public void CachedRerender_WhenInvoked_MarksSelfTriggered_AndBubblesToSource() + { + var reconciler = new Reconciler(); + var node = new Reconciler.ComponentNode(); + + int sourceCalls = 0; + global::System.Action source = () => sourceCalls++; + var wrapped = reconciler.GetOrCreateComponentRerender(node, source); + Assert.NotNull(wrapped); + + Assert.False(node.SelfTriggered); + wrapped(); // headless: no UI dispatcher captured ⇒ runs inline + + Assert.True(node.SelfTriggered); + Assert.Equal(1, sourceCalls); + } + + [Fact] + public void CachedRerender_AfterUnmountTombstone_DoesNotReSelfTrigger() + { + // #20: a cached closure can outlive the node and fire after teardown. The + // Unmounted tombstone must stop it re-adding the node to the self-triggered set + // (which the dirty-path pass would otherwise never clear again, pinning the tree). + var reconciler = new Reconciler(); + var node = new Reconciler.ComponentNode(); + + int sourceCalls = 0; + global::System.Action source = () => sourceCalls++; + var wrapped = reconciler.GetOrCreateComponentRerender(node, source); + Assert.NotNull(wrapped); + + node.Unmounted = true; // mirrors ClearSelfTriggered(node, unmounting: true) + wrapped(); + + Assert.False(node.SelfTriggered); // refused by the tombstone guard + Assert.Equal(1, sourceCalls); // the upstream callback itself still runs (unchanged) + } +}