Skip to content

Commit 599360b

Browse files
fix(virtualization): proper ItemsRepeater recycling — stop leaking every realized container (#324)
* fix(virtualization): proper ItemsRepeater recycling — stop leaking every realized container ElementFactory<T> never reused args.Element from RecycleElement — every GetElement built a fresh Reactor Element tree and Mounted a fresh WinUI control. Per microsoft-ui-xaml-lift `ViewManager.cpp:865-869` and `ItemTemplateWrapper.cpp:120-136`, ItemsRepeater keeps every realized UIElement parented to itself forever and expects the IElementFactory to cycle them through GetElement/RecycleElement. The "fresh build every time" pattern produced one orphan in Repeater.Children per realize. Empirical: scrolling DataSystemDemo > VirtualList (Fixed Height, 100k items) leaked ~75 KB working set per realize and unbounded managed heap. Variable-height demo crashed with stowed exception 0xC000027B once the orphan count interacted with structural divergence between expanded and collapsed rows in RefreshRealizedItems. Fix: - ElementFactory.RecycleElement pushes args.Element onto a recycle stack and drops _mountedElements/_keyByControl tracking. It does NOT call UnmountChild — the WinUI tree stays alive for reuse. - ElementFactory.GetElement pops from the recycle stack first and calls Reconciler.Reconcile(oldElement, newElement, control, ...) to diff the existing tree against the new content. Allocates fresh only when the pool is empty. - _lastElementByControl tracks the last-bound Element per control so reuse has an oldElement to diff against. - _keyByControl reverse-lookup map (also new) lets RecycleElement drop the _mountedElements key entry in O(1) — without it, stale entries drove Reconcile against mismatched realized children when RefreshRealizedItems ran during a subsequent update, which is what was triggering the variable-height demo's 0xC000027B crash. FlexPanel: stop subscribing to XamlRoot.Changed per instance. Loaded/Unloaded do not fire reliably for ItemsRepeater-recycled containers (in this repro Unloaded fired for 12 of 2,774 containers), so the subscribe in OnLoaded leaked every FlexPanel through XamlRoot's multicast delegate list. Replaced with SyncPointScaleLazy() called from MeasureOverride — reads XamlRoot.RasterizationScale directly and updates YogaConfig.PointScaleFactor when it changes. No subscription, same DPI-tracking behavior. Result: at 14,506 realize/recycle cycles, FlexPanel constructor count stays at 50 (the working set), managedMB flat at 17, workingSetMB flat at 238. Previously these climbed 1:1 with realize count. Tests: Reactor.Tests 7648 passed / Reactor.SelfTests 714 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(virtualization): pin the ItemsRepeater recycling contract Adds three SelfTest fixtures that exercise ElementFactory<T> directly through its IElementFactory interface, capturing the invariants that the prior leak violated: - EFR_Factory_BoundedDistinctControls_AcrossManyRealizeCycles: 100 realize-then-recycle cycles return ≤5 distinct UIElement instances (pre-fix: 100, one per cycle). The headline regression gate. - EFR_Factory_RecycledControlIsReusedOnNextRealize: The control handed back via RecycleElement IS the control returned by the next GetElement (reference equality). When the recycle pool is empty, the next GetElement Mints fresh — different control. - EFR_Factory_BookkeepingBoundedAcrossCycles: After 50 realize+recycle pairs, internal dicts (_mountedElements, _keyByControl, _recyclePool, _lastElementByControl) stay bounded at ≤1 each. Pre-fix _mountedElements / _keyByControl would have grown to 50. Catches a second class of bug where the control gets reused but bookkeeping entries accumulate — the exact corruption that drove the variable-height demo's 0xC000027B crash via stale entries in RefreshRealizedItems. Fixtures drive a standalone ElementFactory<T> + Reconciler (no real LazyVStack / ItemsRepeater) so the IElementFactory contract is the only thing exercised. Standalone isolation matters: when an earlier draft shared the same data items with a live LazyVStack, the framework's pre-realized window + our cycles collided on string keys and corrupted state — informative but not what the test intends to measure. ElementFactory<T> exposes four `Debug*Count` accessors (gated by the existing InternalsVisibleTo on Reactor.AppTests.Host) so the bookkeeping fixture can read dict counts without reflection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix+test(virtualization): address PR #324 review feedback Three edge cases Copilot's review caught in the recycle path, plus SelfTest fixtures that pin each one as a regression gate. The original fix is solid for homogeneous-row demos like DataSystem; these are real defects only the broader fleet (heterogeneous rows, re-renders during scroll, navigation away from a LazyStack) would exercise. 1. ElementFactory.GetElement, heterogeneous-row replacement When Reconcile returns a non-null replacement (root element type changed mid-cycle), the popped `reused` control is still parented to the ItemsRepeater — re-introducing the very orphan-in-Children leak the recycle pool was meant to fix. Now we DetachFromParent the orphan and drop its _lastElementByControl tracking before returning the replacement. New fixture: EFR_Factory_ReplacementOnRootTypeChange_DropsOldControlTracking. 2. ElementFactory.RefreshRealizedItems, stale _lastElementByControl The refresh path updated _mountedElements[key] with the new Element but left _lastElementByControl[control] pointing at the pre-refresh Element. A later recycle-then-reuse of the same control then fed Reconcile a stale oldElement and diffed against the wrong tree shape (potentially skipping updates or walking the wrong children). Refresh now writes both dicts together. New fixture: EFR_Factory_RefreshRealizedItems_SyncsLastElementByControl. 3. Reconciler.UnmountRecursive, ItemsRepeater children Verified via microsoft-ui-xaml-lift/.../ItemsRepeater.idl:154 — ItemsRepeater projects to C# as a FrameworkElement, not Panel, even though its C++ implementation uses DeriveFromPanelHelper. So the existing Panel branch in UnmountRecursive did NOT descend into repeater children, leaving every row Component's UseEffect cleanups unrun when the LazyStack itself unmounted (navigation, conditional render). Long-lived timers / subscriptions / async loops inside row Components would leak forever post-navigation. Added an ItemsRepeater branch that walks via VisualTreeHelper (the public surface doesn't expose Children directly). New fixture: EFR_LazyStack_Unmount_CleansUpAllRecycledRowComponents. Not addressed in this PR (design-scope, documented for follow-up): Per-row Component identity. LazyStack's user keySelector is not propagated to row Element.Key, so reusing a control across two logical items preserves any inner Component's UseState/UseEffect state. This is the same behavior as WinUI RecyclingElementFactory and is the documented expectation, but Reactor should give users a clean way to opt into per-item state reset. Tracking separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 516aa2d commit 599360b

5 files changed

Lines changed: 530 additions & 64 deletions

File tree

src/Reactor/Core/ElementFactory.cs

Lines changed: 121 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,43 @@ public sealed partial class ElementFactory<T> : IElementFactory
3333
private readonly Dictionary<string, Element> _mountedElements =
3434
new(global::System.StringComparer.Ordinal);
3535

36+
// Reverse lookup: realized WinUI control → key. Lets RecycleElement drop
37+
// the matching _mountedElements entry in O(1) when ItemsRepeater hands a
38+
// container back. Without this, entries accumulate one per unique key as
39+
// the user scrolls (every realize adds; recycle never removes), and on
40+
// any subsequent re-render RefreshRealizedItems walks stale entries
41+
// whose row.Index now points at a different logical row's container —
42+
// running Reconcile against a mismatched UIElement tree.
43+
private readonly Dictionary<UIElement, string> _keyByControl = new();
44+
45+
// Recycle pool for proper WinUI ItemsRepeater integration. The framework
46+
// keeps every realized UIElement parented to the repeater forever and
47+
// expects the factory to cycle them — see ViewManager.cpp:865-869 in the
48+
// microsoft-ui-xaml-lift source: on realize, it skips Append if the
49+
// returned control is already parented to the repeater. So a recycled
50+
// container must come back out via GetElement to keep the working set
51+
// bounded; allocating fresh on every realize creates one orphan in
52+
// Children per call.
53+
private readonly Stack<UIElement> _recyclePool = new();
54+
55+
// Last Element bound to a given realized control. On reuse from the
56+
// recycle pool, this is the oldElement passed to Reconciler.Reconcile so
57+
// the existing WinUI tree gets diffed-in-place against the new content
58+
// rather than thrown away and re-mounted.
59+
private readonly Dictionary<UIElement, Element> _lastElementByControl = new();
60+
61+
// Test-only accessors for the regression fixture
62+
// ElementFactoryRecyclingFixtures.Factory_BookkeepingBoundedAcrossCycles.
63+
// Confirm that the four bookkeeping structures don't grow with the
64+
// number of realize/recycle cycles. Gated by InternalsVisibleTo on
65+
// Reactor.AppTests.Host (see Reactor.csproj).
66+
internal int DebugRecyclePoolCount => _recyclePool.Count;
67+
internal int DebugLastElementByControlCount => _lastElementByControl.Count;
68+
internal int DebugMountedElementsCount => _mountedElements.Count;
69+
internal int DebugKeyByControlCount => _keyByControl.Count;
70+
internal bool DebugTryGetLastElementByControl(UIElement control, out Element? element)
71+
=> _lastElementByControl.TryGetValue(control, out element!);
72+
3673
public ElementFactory(
3774
IReadOnlyList<T> items,
3875
Func<T, int, Element> viewBuilder,
@@ -126,6 +163,12 @@ internal void RefreshRealizedItems(Microsoft.UI.Xaml.Controls.ItemsRepeater repe
126163

127164
var newElement = _viewBuilder(_items[currentIndex], currentIndex);
128165
_mountedElements[key] = newElement;
166+
// Keep the per-control "last element" tracking in lockstep with
167+
// _mountedElements. Without this, a later RecycleElement→GetElement
168+
// round-trip for the same control would feed the pre-refresh
169+
// Element to Reconcile as oldElement and diff against a stale
170+
// tree shape. (PR #324 review)
171+
_lastElementByControl[child] = newElement;
129172

130173
_reconciler.Reconcile(oldElement, newElement, child, _requestRerender);
131174
}
@@ -160,51 +203,93 @@ public UIElement GetElement(ElementFactoryGetArgs args)
160203

161204
var item = _items[index];
162205
var element = _viewBuilder(item, index);
206+
207+
UIElement? control;
208+
if (_recyclePool.Count > 0)
209+
{
210+
// Reuse a previously-recycled container. The framework still has
211+
// it parented to the ItemsRepeater, so the ViewManager.cpp:866
212+
// Append-skip kicks in and the visual tree stays stable.
213+
var reused = _recyclePool.Pop();
214+
if (_lastElementByControl.TryGetValue(reused, out var oldElement))
215+
{
216+
var replacement = _reconciler.Reconcile(oldElement, element, reused, _requestRerender);
217+
if (replacement is not null && !ReferenceEquals(replacement, reused))
218+
{
219+
// Heterogeneous-row case: Reconcile decided the root
220+
// element type changed and built a fresh control.
221+
// `reused` is now unmounted but still parented to the
222+
// ItemsRepeater — detach so it doesn't sit there as
223+
// an orphan (the original leak shape we're fixing).
224+
// (PR #324 review)
225+
DetachFromParent(reused);
226+
_lastElementByControl.Remove(reused);
227+
control = replacement;
228+
}
229+
else
230+
{
231+
control = reused;
232+
}
233+
}
234+
else
235+
{
236+
// Defensive: pool entry without a tracked oldElement should
237+
// not happen — fall back to re-mounting on top of it.
238+
control = _reconciler.Mount(element, _requestRerender);
239+
}
240+
}
241+
else
242+
{
243+
control = _reconciler.Mount(element, _requestRerender);
244+
}
245+
163246
_mountedElements[key] = element;
164-
var control = _reconciler.Mount(element, _requestRerender);
247+
if (control is not null)
248+
{
249+
_keyByControl[control] = key;
250+
_lastElementByControl[control] = element;
251+
}
252+
165253
return control ?? new TextBlock { Text = "" };
166254
}
167255

256+
// Detach a UIElement from whatever container it's parented to. ItemsRepeater
257+
// is a Panel subclass so the standard Children.Remove path applies; we also
258+
// handle Border/ScrollViewer/ContentControl so this is safe to call on
259+
// arbitrary recycled subtrees.
260+
private static void DetachFromParent(UIElement control)
261+
{
262+
if (control is not FrameworkElement fe) return;
263+
switch (fe.Parent)
264+
{
265+
case Microsoft.UI.Xaml.Controls.Panel panel:
266+
panel.Children.Remove(fe);
267+
break;
268+
case Microsoft.UI.Xaml.Controls.Border border when ReferenceEquals(border.Child, fe):
269+
border.Child = null;
270+
break;
271+
case Microsoft.UI.Xaml.Controls.ContentControl cc when ReferenceEquals(cc.Content, fe):
272+
cc.Content = null;
273+
break;
274+
}
275+
}
276+
168277
public void RecycleElement(ElementFactoryRecycleArgs args)
169278
{
170279
if (args.Element is null) return;
171280

172-
// Clean up Reactor state (component contexts, effects).
173-
_reconciler.UnmountChild(args.Element);
174-
175-
// Pool interactive leaf controls for reuse. Layout containers (Panel, Border)
176-
// are NOT pooled here because ItemsRepeater may still reference the root element
177-
// during its layout pass and modifying children causes COMExceptions. Interactive
178-
// controls are safe to detach and pool because they are leaves with no children.
179-
if (_pool is not null)
180-
PoolInteractiveLeaves(args.Element);
181-
}
281+
// Drop the mounted-element tracking for this container so a later
282+
// RefreshRealizedItems can't run Reconcile against a stale Element
283+
// paired with a now-foreign realized child.
284+
if (_keyByControl.Remove(args.Element, out var stashedKey))
285+
_mountedElements.Remove(stashedKey);
182286

183-
/// <summary>
184-
/// Walk the recycled subtree and pool interactive leaf controls (Button, TextBox,
185-
/// ToggleSwitch). These are the most expensive controls to create and benefit most
186-
/// from pooling. Detaches each from its parent panel before returning to the pool.
187-
/// </summary>
188-
private void PoolInteractiveLeaves(UIElement root)
189-
{
190-
if (root is Microsoft.UI.Xaml.Controls.Panel panel)
191-
{
192-
// Walk children in reverse so removal doesn't shift indices
193-
for (int i = panel.Children.Count - 1; i >= 0; i--)
194-
PoolInteractiveLeaves(panel.Children[i]);
195-
}
196-
else if (root is Microsoft.UI.Xaml.Controls.Border border && border.Child is not null)
197-
{
198-
PoolInteractiveLeaves(border.Child);
199-
}
200-
else if (root is FrameworkElement fe && IsPoolableInteractive(fe))
201-
{
202-
_pool!.Return(fe);
203-
}
287+
// DON'T UnmountChild — the WinUI tree stays alive and is reused on
288+
// the next GetElement call via Reconciler.Reconcile. ItemsRepeater
289+
// keeps the element parented either way (see ViewManager.cpp), so
290+
// tearing down Reactor state here would just be discarded work.
291+
// The _lastElementByControl entry stays valid for the next realize.
292+
_recyclePool.Push(args.Element);
204293
}
205294

206-
private static bool IsPoolableInteractive(FrameworkElement fe) =>
207-
fe is Microsoft.UI.Xaml.Controls.Button
208-
or TextBox
209-
or Microsoft.UI.Xaml.Controls.ToggleSwitch;
210295
}

src/Reactor/Core/Reconciler.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,6 +1101,25 @@ private void UnmountRecursive(UIElement control)
11011101
foreach (var child in panel.Children)
11021102
UnmountRecursive(child);
11031103
}
1104+
else if (control is WinUI.ItemsRepeater repeater)
1105+
{
1106+
// ItemsRepeater projects to C# as a FrameworkElement (not a
1107+
// Panel — see microsoft-ui-xaml-lift/.../ItemsRepeater.idl), so
1108+
// the Panel branch above doesn't catch it even though the
1109+
// framework keeps both realized and recycled containers in
1110+
// its visual subtree. Without this branch, row Components'
1111+
// UseEffect cleanups would never run when the LazyStack is
1112+
// unmounted (e.g., on navigation), leaking any in-flight
1113+
// timers / subscriptions / async loops. We walk via
1114+
// VisualTreeHelper because the public ItemsRepeater surface
1115+
// doesn't expose a Children collection. (PR #324 review)
1116+
int count = Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChildrenCount(repeater);
1117+
for (int i = 0; i < count; i++)
1118+
{
1119+
if (Microsoft.UI.Xaml.Media.VisualTreeHelper.GetChild(repeater, i) is UIElement child)
1120+
UnmountRecursive(child);
1121+
}
1122+
}
11041123
else if (control is WinUI.Border border && border.Child is not null)
11051124
{
11061125
UnmountRecursive(border.Child);

src/Reactor/Yoga/FlexPanel.cs

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -31,50 +31,33 @@ public partial class FlexPanel : Panel
3131
private readonly YogaNode _rootNode;
3232
private readonly HashSet<UIElement> _syncCurrentChildren = new();
3333
private readonly List<UIElement> _syncToRemove = new();
34-
private XamlRoot? _subscribedXamlRoot;
3534

3635
public FlexPanel()
3736
{
3837
_rootNode = new YogaNode(_yogaConfig);
39-
Loaded += OnLoaded;
4038
Unloaded += OnUnloaded;
4139
}
4240

43-
private void OnLoaded(object sender, RoutedEventArgs e)
41+
/// <summary>
42+
/// Read the current rasterization scale from this panel's XamlRoot and
43+
/// update <see cref="YogaConfig.PointScaleFactor"/> if it changed. Called
44+
/// from <see cref="MeasureOverride"/> so the scale tracks the live system
45+
/// DPI without subscribing to <c>XamlRoot.Changed</c>. The subscription
46+
/// approach pinned every FlexPanel through XamlRoot's multicast delegate
47+
/// list — fatal for virtualized lists where ItemsRepeater's recycle path
48+
/// does not reliably fire Unloaded on every recycled container.
49+
/// </summary>
50+
private void SyncPointScaleLazy()
4451
{
45-
var current = XamlRoot;
46-
if (current is null || ReferenceEquals(current, _subscribedXamlRoot))
47-
return;
48-
49-
if (_subscribedXamlRoot is not null)
50-
_subscribedXamlRoot.Changed -= OnXamlRootChanged;
51-
_subscribedXamlRoot = current;
52-
_subscribedXamlRoot.Changed += OnXamlRootChanged;
53-
SyncPointScaleFromXamlRoot();
54-
}
55-
56-
private void OnXamlRootChanged(XamlRoot sender, XamlRootChangedEventArgs args)
57-
{
58-
SyncPointScaleFromXamlRoot();
59-
}
60-
61-
private void SyncPointScaleFromXamlRoot()
62-
{
63-
var scale = (float)(_subscribedXamlRoot?.RasterizationScale ?? 1.0);
52+
var scale = (float)(XamlRoot?.RasterizationScale ?? 1.0);
6453
if (scale <= 0) return;
6554
if (Math.Abs(_yogaConfig.PointScaleFactor - scale) < 0.0001f) return;
6655
_yogaConfig.PointScaleFactor = scale;
6756
_rootNode.MarkDirtyAndPropagate();
68-
InvalidateMeasure();
6957
}
7058

7159
private void OnUnloaded(object sender, RoutedEventArgs e)
7260
{
73-
if (_subscribedXamlRoot is not null)
74-
{
75-
_subscribedXamlRoot.Changed -= OnXamlRootChanged;
76-
_subscribedXamlRoot = null;
77-
}
7861
// Clear Yoga node cache when removed from the visual tree to avoid leaking references
7962
foreach (var node in _nodeCache.Values)
8063
_rootNode.RemoveChild(node);
@@ -311,6 +294,7 @@ private struct ChildLayout { public float X, Y, Width, Height; }
311294

312295
protected override Size MeasureOverride(Size availableSize)
313296
{
297+
SyncPointScaleLazy();
314298
_measuredThisPass.Clear();
315299
SyncYogaTree();
316300
SetRootConstraints(availableSize);

0 commit comments

Comments
 (0)