From ad4de254773e5c6e4d6227ebbb65ecfa3291dd90 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 25 Jun 2026 22:59:25 -0700 Subject: [PATCH 1/2] perf(reconciler): skip redundant self-merge of element modifiers (P1) Reconciler.Update resolves an element's modifiers by accumulating any ModifiedElement wrapper layers, then merging the final inner element's own Modifiers. For the common case of an element that carries modifiers directly (no wrapper) -- every cell in a large keyed grid -- the accumulator is still referentially the element's own ElementModifiers, so that final merge was a self-merge x.Merge(x): it allocated a fresh, value-identical ElementModifiers plus its non-null Layout/Visual bucket sub-records (a Layout+Visual cell is parent + 2 buckets = 3 records, on each of the old and new sides => ~6) per changed cell, every render. On the StocksGrid workload (500 cells, ~50% mutation) that is ~3 KB/changed cell (~7 MB/render of pure garbage). Guard the final merge with !ReferenceEquals(accumulator, element.Modifiers): only merge when a wrapper layer actually contributed a distinct instance. When nothing wrapped the element, keep its own Modifiers reference as-is. Semantically identical (Merge(x,x) is value-equal to x); purely removes the allocation. ApplyModifiers behavior is unchanged. Scope note: this PR is P1 only. The originally-paired P2 (an ApplyModifiers fast-path that skips the post-update pass when modifiers compare equal) was dropped after review found Element.ModifiersEqual does not compare every field ApplyModifiers writes (RequestedTheme, Scale/Rotation/Translation/CenterPoint, inline-flow margin/padding/border, OnUnmountAction/OnUpdateAction hooks), so a structurally-equal compare can coexist with a changed transform/theme and the skip would leave the control stale. P2 will return as its own PR that first makes ModifiersEqual complete w.r.t. ApplyModifiers' guarded writes. Tests (tests/Reactor.Tests/ReconcilerModifierMergeTests.cs): - Merge(x,x) is value-equal to x but a distinct instance (the invariant the guard relies on). - A real wrapper+inner merge still combines correctly (inner wins, base fills gaps) -- the guard only skips the self case, never a real merge. - Revert->fail teeth: 50k Update calls on a direct-modifier leaf allocate ~0 B/call with the guard; reverting it allocates ~1.95 KB/call (cap 64 B). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Reactor/Core/Reconciler.Update.cs | 15 ++- .../ReconcilerModifierMergeTests.cs | 117 ++++++++++++++++++ 2 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 tests/Reactor.Tests/ReconcilerModifierMergeTests.cs diff --git a/src/Reactor/Core/Reconciler.Update.cs b/src/Reactor/Core/Reconciler.Update.cs index 8c7d3120e..3ed01c4e8 100644 --- a/src/Reactor/Core/Reconciler.Update.cs +++ b/src/Reactor/Core/Reconciler.Update.cs @@ -45,10 +45,19 @@ public sealed partial class Reconciler oldEl = oldMod.Inner; newEl = newMod.Inner; } - // Merge any modifiers from the final inner element - if (oldEl.Modifiers is not null) + // Merge any modifiers from the final inner element. + // When there were no ModifiedElement wrapper layers, oldModifiers/modifiers + // are still referentially the element's own Modifiers, so the Merge below + // would be a self-merge (x.Merge(x)) that allocates a fresh, value-identical + // ElementModifiers (plus its non-null Layout/Visual bucket sub-records) for + // nothing. For a large keyed grid that is ~6 needless allocations per changed + // cell every render (a Layout+Visual cell: parent + 2 buckets, on each of the + // old and new sides). Skip the self-merge when the accumulator is already the + // element's own modifiers; only merge when a wrapper layer contributed a + // distinct instance that must be combined. + if (oldEl.Modifiers is not null && !ReferenceEquals(oldModifiers, oldEl.Modifiers)) oldModifiers = oldModifiers is not null ? oldModifiers.Merge(oldEl.Modifiers) : oldEl.Modifiers; - if (newEl.Modifiers is not null) + if (newEl.Modifiers is not null && !ReferenceEquals(modifiers, newEl.Modifiers)) modifiers = modifiers is not null ? modifiers.Merge(newEl.Modifiers) : newEl.Modifiers; // Short-circuit: if old and new elements are structurally identical, diff --git a/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs b/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs new file mode 100644 index 000000000..2b4289176 --- /dev/null +++ b/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs @@ -0,0 +1,117 @@ +using System; +using Microsoft.UI.Reactor.Core; +using Microsoft.UI.Xaml; +using Xunit; + +namespace Microsoft.UI.Reactor.Tests; + +/// +/// Pins the modifier-resolution optimization in +/// (the redundant self-merge fix). When an element carries modifiers directly +/// (no ModifiedElement wrapper) — the common case for every cell in a +/// large grid — the resolved oldModifiers/modifiers are already the +/// element's own reference, so the old code ran +/// x.Merge(x), allocating six value-identical records per changed cell for +/// nothing. now guards that with +/// !ReferenceEquals(...). +/// +/// These tests are headless (no XAML Application). They avoid brush-typed +/// modifiers (which require a UI thread) and use value-type fields only. +/// +public class ReconcilerModifierMergeTests +{ + private static readonly Action NoOp = () => { }; + + // A leaf element that carries modifiers directly — mirrors the shape of a + // grid cell (TextBlockElement with Layout/Visual modifiers, no wrapper). + private sealed record DirectModifierLeaf(int Id) : Element; + + private static ElementModifiers CellModifiers() => new() + { + Layout = new LayoutModifiers { Padding = new Thickness(2, 1, 2, 1), Width = 8 }, + Visual = new VisualModifiers { Opacity = 0.5 }, + }; + + // ── Invariant the guard relies on: Merge(x, x) is value-equal to x ────── + + [Fact] + public void Merge_With_Self_Is_Value_Equal_To_Original() + { + var m = CellModifiers(); + + var merged = m.Merge(m); + + // Value-equal (record structural equality, including the Layout/Visual + // sub-records) — this is what makes skipping the self-merge safe. + Assert.Equal(m, merged); + Assert.True(Element.ModifiersEqual(m, merged)); + + // …but a *distinct* instance: Merge always allocates a fresh record + // (plus fresh Layout/Visual sub-records). That allocation is exactly + // what the reconciler now avoids for the no-wrapper case. + Assert.NotSame(m, merged); + Assert.NotSame(m.Layout, merged.Layout); + Assert.NotSame(m.Visual, merged.Visual); + } + + // ── The merge the guard must still perform stays correct (inner wins) ─── + + [Fact] + public void Merge_Prefers_Other_Fields_And_Fills_Gaps_From_Base() + { + // Simulates accumulated wrapper modifiers (base) merged with an inner + // element's own modifiers (other). `other` wins where set; `base` fills + // the gaps. If anyone "optimizes" Merge into a no-op, this fails — the + // ReferenceEquals guard only skips the *self* case, never a real merge. + var baseMods = new ElementModifiers + { + Layout = new LayoutModifiers { Padding = new Thickness(2), Width = 8 }, + Visual = new VisualModifiers { Opacity = 0.25 }, + }; + var other = new ElementModifiers + { + Layout = new LayoutModifiers { Width = 99 }, // overrides Width + Visual = new VisualModifiers { Rotation = 45f }, // adds Rotation + }; + + var merged = baseMods.Merge(other); + + Assert.Equal(99, merged.Width); // other wins + Assert.Equal(new Thickness(2), merged.Padding); // base fills gap + Assert.Equal(0.25, merged.Opacity); // base fills gap + Assert.Equal(45f, merged.Visual?.Rotation); // other adds + } + + // ── Revert→fail teeth: the no-wrapper resolution must not self-merge ──── + + [Fact] + public void Update_DirectModifiers_AvoidsRedundantSelfMergeAllocation() + { + var reconciler = new Reconciler(); + // Same instance for old and new: structurally identical → Update takes + // the shallow-equality skip path and returns without ever dereferencing + // the (null) control. The modifier resolution at the top of Update still + // runs, so this isolates exactly the self-merge allocation. Before the + // fix this allocated six records (ElementModifiers + Layout + Visual, per + // side); after the fix it allocates none. + var leaf = new DirectModifierLeaf(1) { Modifiers = CellModifiers() }; + + // Warm up the JIT + the resolution path. + for (int i = 0; i < 2_000; i++) + _ = reconciler.Update(leaf, leaf, control: null!, NoOp); + + const int iterations = 50_000; + long before = GC.GetAllocatedBytesForCurrentThread(); + for (int i = 0; i < iterations; i++) + _ = reconciler.Update(leaf, leaf, control: null!, NoOp); + long delta = GC.GetAllocatedBytesForCurrentThread() - before; + + // New code allocates ~0 B/call on this path. The reverted self-merge + // allocates well over 1 KB/call (six records). A 64 B/iteration cap + // cleanly separates the two while tolerating incidental bookkeeping. + Assert.True(delta <= 64L * iterations, + $"Update self-merged direct modifiers: {delta}B over {iterations} calls " + + $"({delta / (double)iterations:F1} B/call, cap 64). The redundant " + + $"x.Merge(x) appears to be back."); + } +} From 157ff0202befc8e65f355831c6f90a936ca798e8 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Fri, 26 Jun 2026 02:08:17 -0700 Subject: [PATCH 2/2] test(reconciler): pin wrapper-merge fall-through + lifecycle-callback preservation (P1 pr-review) Folds two test-coverage findings from the pr-review of #692 (both multi-model-confirmed), strengthening the modifier-merge teeth without touching production code: - Update_WrapperLayer_StillMergesInnerModifiers: proves the !ReferenceEquals guard skips ONLY the self-merge - when a ModifiedElement wrapper contributes a distinct modifier instance, Reconciler.Update''s fall-through still performs the real merge. Differential-allocation teeth verified to bite: simulating an over-broad guard makes this fail while the existing self-merge tooth still passes (unique coverage). - Merge_Preserves_Lifecycle_Callbacks: pins that ElementModifiers.Merge preserves OnMount/OnUnmount/OnUpdateAction (other wins, base fills gaps), guarding the same callback-drop class that bit a sibling change. Full Reactor.Tests green (9707 passed / 64 skipped / 0 failed). Test-only; no src/Reactor change, so the P1 perf delta on ad4de254 is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ReconcilerModifierMergeTests.cs | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs b/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs index 2b4289176..62f899ad2 100644 --- a/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs +++ b/tests/Reactor.Tests/ReconcilerModifierMergeTests.cs @@ -114,4 +114,107 @@ public void Update_DirectModifiers_AvoidsRedundantSelfMergeAllocation() $"({delta / (double)iterations:F1} B/call, cap 64). The redundant " + $"x.Merge(x) appears to be back."); } + + // ── Lifecycle callbacks survive a merge (regression guard) ───────────── + // ModifiersEqual deliberately ignores OnMount/OnUpdate (side-effect hooks, + // not render state), but Merge must PRESERVE them. A self-merge that + // silently dropped OnUpdateAction is exactly the class of bug that bit a + // sibling change — pin it so the self-merge skip stays provably safe for + // the callback-bearing fields, not just the Layout/Visual buckets. + [Fact] + public void Merge_Preserves_Lifecycle_Callbacks() + { + Action mount = _ => { }; + Action unmount = _ => { }; + Action update = _ => { }; + + var baseMods = new ElementModifiers + { + OnMountAction = mount, + OnUnmountAction = unmount, + OnUpdateAction = update, + }; + + // Self-merge (the no-wrapper case the reconciler now skips) is value- + // identical: every callback is still the very same delegate afterwards. + // This is what makes skipping the self-merge safe for callback fields. + var self = baseMods.Merge(baseMods); + Assert.Same(mount, self.OnMountAction); + Assert.Same(unmount, self.OnUnmountAction); + Assert.Same(update, self.OnUpdateAction); + + // A real merge with an `other` that leaves callbacks unset keeps the + // base callbacks (base fills the gap)… + var other = new ElementModifiers { Visual = new VisualModifiers { Opacity = 0.3 } }; + var filled = baseMods.Merge(other); + Assert.Same(mount, filled.OnMountAction); + Assert.Same(unmount, filled.OnUnmountAction); + Assert.Same(update, filled.OnUpdateAction); + + // …and `other` wins where it sets its own callback. + Action update2 = _ => { }; + var overriding = baseMods.Merge(new ElementModifiers { OnUpdateAction = update2 }); + Assert.Same(update2, overriding.OnUpdateAction); + Assert.Same(mount, overriding.OnMountAction); // base still fills the gap + } + + // ── The guard skips ONLY the self-merge: a real wrapper layer still merges ── + // Update_DirectModifiers_AvoidsRedundantSelfMergeAllocation pins that the + // no-wrapper case is skipped. This pins the *other* half through + // Reconciler.Update: when a ModifiedElement wrapper contributes a DISTINCT + // modifier instance, the !ReferenceEquals guard's fall-through MUST still run + // the real merge (wrapper ⊕ inner). The merged value-correctness is covered + // by Merge_Prefers_Other_Fields_And_Fills_Gaps_From_Base; here we prove the + // reconciler does not wrongly skip that merge. (End-to-end application of the + // resolved modifiers to a live control is covered by the selftest fixtures — + // ModifierEventFixtures / ControlUpdateFixtures — which need a UI thread.) + [Fact] + public void Update_WrapperLayer_StillMergesInnerModifiers() + { + var reconciler = new Reconciler(); + + // No-wrapper leaf: the resolved modifiers ARE the element's own instance, + // so the !ReferenceEquals guard skips the self-merge → ~0 B/call. + var leaf = new DirectModifierLeaf(1) { Modifiers = CellModifiers() }; + + // Wrapped leaf: the outer ModifiedElement contributes a distinct modifier + // instance (WrappedModifiers), so the accumulator is no longer reference- + // equal to the inner element's own Modifiers — the guard falls through and + // performs the real merge, which allocates. Same instance for old and new + // so Update still takes the shallow-equality skip and never dereferences + // the (null) control. + var inner = new DirectModifierLeaf(2) { Modifiers = CellModifiers() }; + var wrapped = new ModifiedElement( + inner, + new ElementModifiers { Visual = new VisualModifiers { Rotation = 30f } }); + + // Warm up the JIT + both resolution paths. + for (int i = 0; i < 2_000; i++) + { + _ = reconciler.Update(leaf, leaf, control: null!, NoOp); + _ = reconciler.Update(wrapped, wrapped, control: null!, NoOp); + } + + const int iterations = 50_000; + + long b0 = GC.GetAllocatedBytesForCurrentThread(); + for (int i = 0; i < iterations; i++) + _ = reconciler.Update(leaf, leaf, control: null!, NoOp); + long noWrapper = GC.GetAllocatedBytesForCurrentThread() - b0; + + long b1 = GC.GetAllocatedBytesForCurrentThread(); + for (int i = 0; i < iterations; i++) + _ = reconciler.Update(wrapped, wrapped, control: null!, NoOp); + long wrapper = GC.GetAllocatedBytesForCurrentThread() - b1; + + // The wrapper layer's merge allocates a fresh ElementModifiers (+ a Visual + // bucket) per side, per call — comfortably more than 200 B/call above the + // guarded no-wrapper path. If anyone broadens the guard so it also skips + // the wrapper merge, `wrapper` collapses toward `noWrapper` and the wrapper + // layer's modifiers would silently stop applying. This catches that. + Assert.True(wrapper > noWrapper + 200L * iterations, + $"Wrapper-layer merge appears to have been skipped: wrapper={wrapper} B, " + + $"noWrapper={noWrapper} B over {iterations} calls " + + $"(delta {(wrapper - noWrapper) / (double)iterations:F1} B/call, expected > 200)."); + } }