Repository navigation
Fix dynamic ItemsHost reconciliation for generated control wrappers - #1363
kusutoriクスとり (kusutori) wants to merge 9 commits into
Conversation
Alexandre Zollinger Chohfi (azchohfi)
left a comment
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks for this. Pulling the two ItemsHost copies into one shared helper, reconciling incrementally with a stable observable sink, and giving generated slots real teardown are all good changes. Putting teardown on each property entry also looks like the right layer: WithUnmount would replace an author's own hook, and NamedSlot isn't available while Items holds the ItemsHost.
I'm leaving this as a comment for now, not an approval. I didn't find anything blocking, but a few things are worth addressing first (details inline):
- Teardown order. Items and generated slots are removed from the tree before
UnmountChildruns. The engine expects unmount first, while the control is still parented (for connected-animation snapshots, among other things). This is the main one. - Hot Reload. The
CanUpdatepre-check bypassesReconcileV1Child's Hot Reload state migration. - Performance for raw-item lists. Lists with no Element items lost their unchanged-list early return and now pay an O(n²) match plus allocations on every render.
ItemEqualssemantics. A match now keeps the old instance in the sink. Worth documenting, or changing.- Test gaps. Always visiting Element items, unkeyed/type-change/pooled items, raw items on a native
ItemCollection, theHasUnmountrooting, andCanAssignItemsrejection all lack a test that would fail if the code were reverted.
Not tied to a changed line:
plugins/reactor/skills/reactor-wrapping/SKILL.md(the[WrapElementSlot]row) still describes slots as mount + update only. It should mention the new generated detach/unmount teardown and that the control's own default is left alone when no slot element is declared.- The
ItemsHostmount/update branches inV1HandlerAdapter(hand-coded handlers) aren't exercised by any test:DescriptorHandler.ChildrenhidesItemsHost, so the new fixture only goes through the descriptor path. There's no in-repo hand-coded handler that exposesItemsHosttoday, so this is low priority, but a minimal registered handler fixture would cover the extra rooting inBind.
Things I checked that looked fine: both reactor.api.txt copies are byte-identical; the new SettingsControls reference is limited to the test hosts and its version is pinned centrally; generated code uses only public API; the generator changes are netstandard2.0-safe; the new runtime code is AOT/trim-safe (no reflection; GeneratedWinRTExposedExternalType covers the observable collection); and the generated guides match their templates.
Perf: before vs after (
|
| Harness | Renders/s | Reconcile ms | Diff ms | Memory MB |
|---|---|---|---|---|
| Stocks grid, 50% mutation | 5.06 → 5.44 | 36.7 → 34.0 | 32.2 → 30.2 | 277.8 → 274.1 |
| Stocks grid, skip floor (0%) | 10.32 → 10.44 | 11.2 → 11.6 | 10.2 → 10.7 | 253.6 → 254.0 |
| Keyed list | 15.72 → 15.46 | 6.1 → 7.6 | 6.0 → 7.5 | 196.3 → 186.6 |
| Flex | 3.37 → 3.42 | 47.1 → 47.8 | 42.1 → 43.2 | 305.4 → 307.0 |
| DataGrid | 7.31 → 7.27 | 44.0 → 44.2 | 43.9 → 44.1 | 388.9 → 389.0 |
All of these are within run-to-run noise; the keyed-list reconcile numbers swung 6–10 ms within each side, and this PR doesn't touch that path. The reconciler micro-suite (17 benchmarks, 4 paired rounds) also shows no timing change; every paired range includes zero. One deterministic allocation delta did show up:
- ToggleSwitch mount + unmount: +48 B per op (4,439 → 4,487). The likely cause is
DescriptorHandler.Unmount, which now runsforeachover_descriptor.Properties. That property is typedIReadOnlyList<…>, so each unmount of every descriptor-backed control boxes an enumerator. An indexedforloop fixes it. Caching theProperties.Any(p => p.HasUnmount)result once per descriptor would also take that scan off the mount path.
2. Targeted ItemsHost benchmark (ComboBox, 1 warm-up + 8 paired rounds, 2,000 updates per round)
Each update re-renders the ComboBox element and calls UpdateChild.
| Scenario | main | PR | Change | Alloc / update |
|---|---|---|---|---|
100 element items, same array, only the host's Margin changes |
4.8 µs | 58.8 µs | 12.3× slower | 1.4 KB → 5.6 KB |
100 element items, new but equal array (typical Render()) |
15.1 µs | 81.3 µs | 5.4× slower | 0.2 KB → 4.5 KB |
| 100 element items, one item changed | 19,972 µs | 41.0 µs | ~490× faster | 42.5 KB → 4.5 KB |
| 100 strings, new but equal array | 4.0 µs | 24.9 µs | 6.2× slower | 0.2 KB → 4.4 KB |
| 1,000 strings, new but equal array | 25.5 µs | 308.9 µs | 12.1× slower | 0.2 KB → 41.3 KB |
| 100 strings, one string changed | 757.8 µs | 37.6 µs | 20× faster | 6.7 KB → 4.5 KB |
Paired per-round ranges excluded zero in every row.
Where the unchanged-list cost comes from. ItemsHostReconciliation.Bind now runs in full on every update. Each run:
- allocates a
Dictionary, abool[], a newListand oneItemrecord per entry; - matches raw items with a linear scan of
previous, which is O(n²); - reads the native
ItemCollectionacross WinRT for every slot.
The old early return (same reference, or element-wise equal) skipped all of that.
Suggestions. None of these is blocking: the absolute cost is about 55 µs per parent render for a 100-item ComboBox.
- Raw (non-Element) items: restore an early return when the list is the same reference or element-wise equal. No component state lives under raw items, so the "always descend" rule doesn't apply.
- Element items: keep descending, but skip the collection-diff pass and avoid rebuilding
nextwhen every child matched in place. - Raw-item matching: try
j == ifirst before the linear scan, so an in-order list stays O(n). - Teardown: use an indexed loop in
DescriptorHandler.Unmountto remove the 48 B per unmount.
Benchmark source (drop into tests/perf_bench/PerfBench.ControlModel/Benches/ and add the six types to BenchCatalog.All)
Note: ListView(...) goes through the hand-coded ListViewHandler, not ItemsHost, so the benchmark uses ComboBox for both the Element and raw-string cases. Class names still say ListView, but the bodies use ComboBox.
// Ad-hoc PR #1363 perf probe — NOT committed. Exercises the ItemsHost update path
// (ListView = Element items, ComboBox = raw string items). Each iteration re-renders
// the host with a fresh parent element so the handler's Update path always runs.
using Microsoft.UI.Reactor;
using Microsoft.UI.Reactor.Core;
using Microsoft.UI.Xaml;
using PerfBench.ControlModel.Variants;
using static Microsoft.UI.Reactor.Factories;
namespace PerfBench.ControlModel.Benches;
public abstract class ItemsHostBenchBase : IBench
{
public abstract string Id { get; }
public abstract string Name { get; }
protected abstract Element Build(int iteration);
public void RunOne(BenchVariant variant, BenchContext ctx)
{
if (ctx.Scratch is not Fixture f)
{
var el = Build(0);
var ui = ctx.Reconciler.Mount(el, NoOp)!;
ctx.Parent.Children.Add(ui);
ctx.Scratch = new Fixture { Element = el, Control = ui };
return;
}
var next = Build(ctx.Iteration + 1);
ctx.Reconciler.UpdateChild(f.Element, next, f.Control, NoOp);
f.Element = next;
}
private sealed class Fixture
{
public Element Element = null!;
public UIElement Control = null!;
}
protected static Element[] TextItems(int n, int changedIndex = -1)
{
var a = new Element[n];
for (int i = 0; i < n; i++)
a[i] = TextBlock(i == changedIndex ? "changed" : "item " + i);
return a;
}
protected static string[] StringItems(int n, int changedIndex = -1)
{
var a = new string[n];
for (int i = 0; i < n; i++)
a[i] = i == changedIndex ? "changed" : "item " + i;
return a;
}
private static readonly Action NoOp = static () => { };
}
/// <summary>ComboBox ItemElements, 100 Element items, same array every render; only a host property
/// (Margin) toggles, so the host updates but its list is untouched.</summary>
public sealed class IH1_ListViewSameArray : ItemsHostBenchBase
{
public override string Id => "IH1";
public override string Name => "ComboBoxEl100_SameItemsArray";
private static readonly Element[] Items = TextItems(100);
protected override Element Build(int iteration) => ComboBox(Items, default, null).Margin(iteration & 1);
}
/// <summary>ComboBox ItemElements, 100 Element items, fresh-but-equal element instances every render (typical Render()).</summary>
public sealed class IH2_ListViewEqualFresh : ItemsHostBenchBase
{
public override string Id => "IH2";
public override string Name => "ComboBoxEl100_EqualFreshItems";
private static readonly Element[] A = TextItems(100);
private static readonly Element[] B = TextItems(100);
protected override Element Build(int iteration) => ComboBox((iteration & 1) == 0 ? A : B, default, null);
}
/// <summary>ComboBox ItemElements, 100 Element items, one item's text toggles every render.</summary>
public sealed class IH3_ListViewOneChanged : ItemsHostBenchBase
{
public override string Id => "IH3";
public override string Name => "ComboBoxEl100_OneItemChanged";
private static readonly Element[] A = TextItems(100);
private static readonly Element[] B = TextItems(100, changedIndex: 50);
protected override Element Build(int iteration) => ComboBox((iteration & 1) == 0 ? A : B, default, null);
}
/// <summary>ComboBox, 100 string items, fresh-but-equal arrays every render.</summary>
public sealed class IH4_ComboBoxEqualStrings100 : ItemsHostBenchBase
{
public override string Id => "IH4";
public override string Name => "ComboBox100_EqualStrings";
private static readonly string[] A = StringItems(100);
private static readonly string[] B = StringItems(100);
protected override Element Build(int iteration) => ComboBox((iteration & 1) == 0 ? A : B);
}
/// <summary>ComboBox, 1000 string items, fresh-but-equal arrays every render (raw-item matching scale).</summary>
public sealed class IH5_ComboBoxEqualStrings1000 : ItemsHostBenchBase
{
public override string Id => "IH5";
public override string Name => "ComboBox1000_EqualStrings";
private static readonly string[] A = StringItems(1000);
private static readonly string[] B = StringItems(1000);
protected override Element Build(int iteration) => ComboBox((iteration & 1) == 0 ? A : B);
}
/// <summary>ComboBox, 100 string items, one string toggles every render.</summary>
public sealed class IH6_ComboBoxOneChanged : ItemsHostBenchBase
{
public override string Id => "IH6";
public override string Name => "ComboBox100_OneStringChanged";
private static readonly string[] A = StringItems(100);
private static readonly string[] B = StringItems(100, changedIndex: 50);
protected override Element Build(int iteration) => ComboBox((iteration & 1) == 0 ? A : B);
}Run: PerfBench.ControlModel.exe --test IH1 IH2 IH3 IH4 IH5 IH6 --variant Reactor --reps 1 --iterations 2000 --headless --out <file>, alternating main and PR processes per round.
|
Following up on the review with perf numbers and an explanation of the CI failures. Performance:
|
| Harness | Renders/sec | Avg reconcile (ms) | Avg memory (MB) | Allocated per render |
|---|---|---|---|---|
| StocksGrid, 50% updates | 5.06 → 5.44 (+7.5%) | 36.7 → 34.0 (−7.3%) | 277.8 → 274.1 (−1.3%) | −2.2% |
| StocksGrid, low-load floor | 10.32 → 10.44 (+1.2%) | 11.2 → 11.6 (+3.6%) | 253.6 → 254.0 (+0.1%) | −0.3% |
| Keyed list (10 runs) | 15.74 → 15.74 (0.0%) | 5.63 → 5.68 (+0.9%) | 188.4 → 189.7 (+0.7%) | +0.2% |
| Flex layout | 3.37 → 3.42 (+1.4%) | 47.1 → 47.9 (+1.5%) | 305.4 → 307.1 (+0.6%) | +1.4% |
| Data grid | 7.31 → 7.27 (−0.5%) | 44.0 → 44.2 (+0.4%) | 388.9 → 389.0 (0.0%) | −0.3% |
Values are medians (main → PR). Every difference is within this machine's run-to-run spread, so I don't see a macro-level regression.
Control-model micro-benchmarks (PerfBench.ControlModel, five runs per side): time per operation moved in both directions, within noise. No benchmark slowed consistently. Allocations are byte-identical except for one deterministic change. Each benchmark that mounts and unmounts elements with callbacks now allocates +48 bytes per operation:
| Benchmark | Bytes/op, main → PR |
|---|---|
| Mount a leaf with one callback | 4,439 → 4,487 |
| Mount a leaf with three callbacks | 10,897 → 10,945 |
| Event-handler state allocation | 4,215 → 4,263 |
| Mount a leaf with no callbacks | 1,141 → 1,141 (unchanged) |
The callback-free case is unchanged, and only elements with a tag reach the handler's unmount path. That points to the new foreach (var prop in _descriptor.Properties) in DescriptorHandler.Unmount. Properties is typed as IReadOnlyList<…>, so the loop likely boxes the List<T> enumerator on every unmount. Iterating by index, or caching a per-descriptor "has property teardown" flag computed once, would remove the allocation. The Properties.Any(...) check in Mount could use the same flag. It's small, but it's paid on every unmount of every descriptor-backed control.
Why CI is failing
There are three independent causes. A fourth job fails only as a result of one of them.
1. Selftests (1/2), Packaged Selftests (MSIX) (1/2), Coverage (selftest-1): PropertyGrid_Target_Switching, Switching_ShapeShows
This is a real regression from the PR, and I reproduced it locally. main is green on the same fixture.
ItemsHostReconciliation.Unbind now calls GetCollection(control).Clear() when the host unmounts. The previous unmount path only tore down UIElement children and never cleared the sink. For a ComboBox with a selection, clearing Items raises SelectionChanged with SelectedIndex == -1. The control is still attached to its element at that point, so the element's OnSelectedIndexChanged runs during teardown.
The PropertyGrid enum editor (TypeRegistry.ResolveEnum) handles that callback with Enum.Parse(enumType, names[index]). Index −1 would throw there, so the most likely result is that the switch from the Material target (which has an enum ComboBox) to the Shape target never renders its rows, and "Position" is not found.
Verification: I confirmed that the Clear() call causes the failure. Removing that line made all five checks pass locally; with the line restored, Switching_ShapeShows fails. I didn't trace the exception itself, so the names[-1] step above is inferred from the code.
This affects users too. Any ComboBox, or other selection control backed by ItemsHost, that unmounts with a selection will now invoke its change callback with −1. Possible fixes:
- Don't clear native sinks on terminal unmount, which matches the previous behavior.
- Detach the element from the control's state before clearing, so the change handler has nothing to dispatch to.
- Clear the sink under the existing echo suppression (
WriteSuppressed).
A regression test that unmounts a ComboBox with a selection and asserts the callback is not invoked would cover it.
2. AOT Selftests (2/2): new ItemsHost_Observable_Keyed_Lifecycle fixture
not ok 779 ItemsHost_Observable_Keyed_Lifecycle_CRASH - InvalidOperationException: SettingsExpander was not mounted.
The fixture passes under JIT, both locally (all 15 checks) and in the regular Selftests lane. Under NativeAOT the SettingsExpander never appears in the tree. No render error precedes it in the log. This is the first selftest fixture that uses a Windows Community Toolkit control, and the new docs say the observable list works "including indirect Native AOT collection boundaries", which is exactly the scenario failing here. I haven't confirmed the root cause.
Before deciding how to handle this, it's worth confirming whether SettingsExpander can render at all under AOT. If it can, the PR's claim needs a fix. If it's a toolkit/AOT limitation unrelated to this PR, add the fixture to the runner's NativeAOT skip list (GetAotSkipPatterns in SelfTestRunner.cs) and soften the AOT wording in the docs.
3. Docs build: InlineSnippetLedgerTests.Inline_Csharp_Examples_Are_Snippet_Backed_Or_Ledgered
The new inline example in docs/_pipeline/templates/control-reconciler-protocol.md.dt (the Children = new ItemsHost<MyElement, MyControl>(...) { SetCollection = ... } block) isn't compiled by anything. The gate requires every inline C# block to be snippet-backed or explicitly allowed. Either move it into a doc app and reference it with ```csharp snippet="<topic>/<id>", or add it to AllowedInlineExamples in tests/Reactor.DocPipeline.Tests/InlineSnippetLedgerTests.cs. Moving it to a snippet is preferable, since it also gets compiled.
4. Merged coverage
This fails only because the selftest coverage lane above produced no numbers. It should clear once the PropertyGrid regression is fixed.
|
Thanks Alexandre Zollinger Chohfi (@azchohfi) for the detailed review and benchmark probes. I went through each review point and pushed the fixes in c8a3c92 and the follow-up performance optimization in 92ea6ce7. Fixes
Core performanceUsing your six probes, original
Values are medians of per-process means. The paired timing intervals confirm the unchanged-string improvements and the remaining unchanged-Element regressions versus main. A separate same-runner A/B against the corrected pre-optimization commit ( Validation and remaining limitations
There is still room to optimize, especially the unchanged-Element path, while keeping the lifecycle/state fixes intact. Please point out anything I missed or any concerns with these changes; I am happy to revise them further. |
Alexandre Zollinger Chohfi (azchohfi)
left a comment
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks kusutoriクスとり (@kusutori), this is a big improvement. CI is green on 92ea6ce, and the PropertyGrid, AOT and docs-snippet failures from last round are all fixed. You addressed all of the earlier inline comments; we'll resolve those threads.
Perf, rerun on our side
We reran the same six ItemsHost probes against three versions: main (aed6870), the PR before your fixes (e74a652), and the PR now (92ea6ce). Each side ran 8 interleaved rounds of 2,000 updates on one machine; values are medians per update.
| Scenario | main | PR before | PR now | B/op (main → now) |
|---|---|---|---|---|
| 100 Elements, same array | 5.2 µs | 96.9 µs | 35.0 µs | 1,369 → 1,381 |
| 100 Elements, new but equal | 21.8 µs | 63.6 µs | 35.1 µs | 198 → 200 |
| 100 Elements, one changed | 16,690 µs | 40.1 µs | 43.0 µs | 42,471 → 239 |
| 100 strings, new but equal | 2.4 µs | 25.6 µs | 4.9 µs | 180 → 181 |
| 1,000 strings, new but equal | 16.4 µs | 378.6 µs | 14.6 µs | 194 → 202 |
| 100 strings, one changed | 340.5 µs | 61.2 µs | 18.3 µs | 6,656 → 246 |
This matches your numbers: the extra allocations are gone, and the string paths are back to roughly main's level. The remaining gap is unchanged Element lists; see the inline note.
Before merge
- Bench count: please keep the ItemsHost benchmarks and update the perf harness's expected count from 17 to 23 (details inline on
AllBenches.cs). We're tracking the CI gap that let this through in #1373. - Guide: the teardown-order sentence in the guide is reversed.
- Tests: add a test for the callback/gesture refresh on skipped items.
- Callbacks during unmount: generated plain-event trampolines still get a callback during terminal unmount.
Not tied to a changed line
CHANGELOG.mdhas no entry. Please addSetCollectionand the three-callbackImperativeBridgedoverload under Added, and incrementalItemsHostreconciliation plus generated slot teardown under Fixed.plugins/reactor/skills/reactor-wrapping/SKILL.md(theItemsinference row, around line 77) still only says "aparams object[] itemsslot". One sentence would help agents: generatedItemsreconcile incrementally,.WithKey(...)preserves item state across reorders, and writableIList<object>sinks get an observable list on mount.
The rest are inline. Happy to help with any of these.
|
Thanks Alexandre Zollinger Chohfi (@azchohfi) for the detailed review! I've addressed the follow-up feedback in 4f5cb8e and added the suggested same-source fast path in 6320d4a. Changes:
PerformanceThis is before/after the latest optimization (
Displayed values are medians across matched rounds; Δ is the mean paired percentage change. IH1 improves substantially without changing allocations. All five macro timing/allocation intervals include zero. IH3 and IH6 are slightly slower in this run, It might just be measurement error Validation: full Also, a correction to my earlier but haven't reproduced them again or confirmed the proposed cause. The state was already native-attached. If I establish a reproducible problem, I'll open a separate issue. There is still room to improve performance, particularly for fresh sources. Please feel free to point out anything else that needs adjusting. |
Alexandre Zollinger Chohfi (azchohfi)
left a comment
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks kusutoriクスとり (@kusutori), this round addressed essentially everything from the last review, with tests that fail when the code is broken. The same-array gate in particular closes the biggest perf gap. I'm resolving the previous threads.
Perf: main vs last round vs now
We reran the six ItemsHost probes against main (aed6870), 92ea6ce and 6320d4a. Each side ran 8 interleaved rounds of 2,000 updates on one machine; values are medians per update.
| Scenario | main | 92ea6ce | 6320d4a | B/op (main → now) |
|---|---|---|---|---|
| 100 Elements, same array | 5.0 µs | 23.9 µs | 5.2 µs | 1,369 → 1,380 |
| 100 Elements, new but equal | 16.5 µs | 26.8 µs | 28.4 µs | 198 → 199 |
| 100 Elements, one changed | 15,468 µs | 48.6 µs | 50.9 µs | 42,471 → 239 |
| 100 strings, new but equal | 3.1 µs | 4.7 µs | 4.7 µs | 180 → 181 |
| 1,000 strings, new but equal | 20.8 µs | 14.6 µs | 14.5 µs | 194 → 202 |
| 100 strings, one changed | 369 µs | 16.2 µs | 16.0 µs | 3,376 → 213 |
This agrees with your before/after numbers. The same-array case is now on par with main. The "new but equal" Element case (about +12 µs for 100 items) is what remains, as you noted, and that's fine to leave for a follow-up.
Before merge
- The new
../aot-support.mdlink should fail the strict MkDocs build (inline). - The PR now conflicts with
maininCHANGELOG.md(that's the only conflicting file). Please rebase or mergemainand keep both sets of[Unreleased]entries.
The remaining comments are smaller: one context-propagation gap for nested item hosts (fine as a follow-up issue), doc accuracy and placement, and two tests that need a positive control or an extra path. CI hasn't run on these commits yet; we'll approve the workflows.
| code-only Native AOT apps may also need explicit provider registration (see | ||
| [AOT support](../aot-support.md)). |
There was a problem hiding this comment.
This relative link points outside the MkDocs docs_dir (mkdocs.yml sets docs_dir: docs/guide). A .md link that doesn't resolve triggers links.not_found: warn, and both ci.yml and docs.yml run mkdocs build --strict, so this should fail the docs build. (The existing ../ links in win2d-canvas.md point at directories, which MkDocs ignores, so they're not a precedent.) Other guide pages link outside the guide with an absolute URL, for example:
[AOT support](https://github.com/microsoft/microsoft-ui-reactor/blob/main/docs/aot-support.md)
Then recompile this topic.
| private static bool HasChangedItemContext(ItemsHostState state, Reconciler reconciler) | ||
| { | ||
| if (!reconciler.HasActiveContextValues) return false; | ||
| // ItemsControl and third-party hosts need not expose their items as | ||
| // ordinary content children. Query the tracked roots, not the host's | ||
| // visual subtree, so unexpanded/unrealized items are covered as well. | ||
| foreach (var item in state.Items) | ||
| if (item is { Source: Element, Value: UIElement child } | ||
| && reconciler.ShouldDeclineSkip(child)) | ||
| return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Nice gate overall. Each of the dirty, forced-render and theme guards has a fixture that fails without it.
One gap: ShouldDeclineSkip(child) walks the item's subtree through HasConsumedContextChangedInSubtree → ForEachReactorChildControl. That walk only visits Panel, Border, ScrollViewer, ContentControl.Content and similar. It never reads ReactorState.ItemsHostState.Items, so it doesn't look inside a nested ItemsHost (an item that is itself a generated ListBox or SettingsExpander) or an Expander.Header. With an unchanged outer array, a UseContext consumer inside one of those keeps the stale value. main skipped same-reference sources unconditionally, so this is an incomplete guard rather than a regression.
Since item state now lives on ReactorState, HasConsumedContextChangedInSubtree could also visit TryGetReactorState(control)?.ItemsHostState?.Items (values that are UIElement with an Element source). A fixture with an outer same-array host, an inner generated ListBox item and a context-consuming row would lock it in. Fine as a follow-up issue if you prefer.
| ### Code-only apps wrapping toolkit controls | ||
|
|
||
| An observable `ItemsHost` list has generated WinRT exposure, but that only covers | ||
| the collection ABI, not the control library's XAML templates. A code-only AOT app | ||
| may need to statically root and register the toolkit's generated metadata provider | ||
| before mounting wrapped controls. For example, with WCT SettingsControls: | ||
|
|
||
| ```csharp | ||
| ReactorApp.RegisterControlAssembly( | ||
| new CommunityToolkit.WinUI.Controls.SettingsControlsRns. | ||
| CommunityToolkit_WinUI_Controls_SettingsControls_XamlTypeInfo.XamlMetaDataProvider()); | ||
| ``` | ||
|
|
||
| The concrete provider name depends on the library. The SettingsExpander selftest | ||
| uses this registration explicitly; passing it is not proof of automatic metadata | ||
| discovery under trimming. | ||
|
|
There was a problem hiding this comment.
This ### subsection landed between the ## What does *not* work under AOT (today) heading and that section's existing intro ("These subsystems compile cleanly…") and table. The PropertyGrid, DataGrid, UseObservable and devtools rows now read as part of the toolkit subsection. Could you move it below the table, or give the existing intro and table their own ### heading?
| /// call engine-internal helpers for bindings without owned child lifetime, | ||
| /// such as rebuilding a RichTextBlock's blocks. |
There was a problem hiding this comment.
RichTextBlock isn't a good example of "without owned child lifetime". RebuildRichTextBlocks / UpdateRichTextBlocks mount and keep inline UI Element children (#480), and their teardown relies on the engine's UnmountInlineUIChildren special case. Could you drop the example, or use a binding that really has no Element children? The same example appears in the guide's overload table (control-reconciler-protocol.md.dt:401).
| .PARAMETER MicroIterations | ||
| Inner iterations per micro repetition (default 1000) over which each bench's | ||
| mean ns/op and allocated bytes/op are averaged. The suite runs 17 benches | ||
| mean ns/op and allocated bytes/op are averaged. The suite runs 23 benches |
There was a problem hiding this comment.
Thanks for updating the count. The description next to it still says "the spec-047 M1–M14 set plus 3 supplementary", which adds up to 17. Same in PerfLib.ps1:87-90, :755 and the PR-comment text at :776 ("spec-047 M1–M13 + 3 supplementary"). Could you add "+ IH1–IH6 ItemsHost benches" in those four places so the /perf output and docs match the 23?
| The generator normally emits `ReactorApp.RegisterControlAssembly` in the element's | ||
| static constructor, so a control library's |
There was a problem hiding this comment.
Small accuracy point: the generator emits ReactorApp.TryRegisterControlAssembly(typeof(X).Assembly) (WrapperGenerator.cs ~1759), not RegisterControlAssembly. The Try form quietly returns false when the provider has been trimmed, which is exactly the case the next paragraph describes. Naming the Try call and the symptom (a 0xC000027B crash when the control is first realized) would tell an agent when explicit registration is needed.
|
|
||
| ### Fixed | ||
|
|
||
| - Flat `ItemsHost` children reconcile incrementally, unchanged source snapshots can skip the item walk when state/context/theme/forced-render guards allow it, and generated Element slots receive terminal teardown while still parented (specs 047/058). |
There was a problem hiding this comment.
Two small things on the entries here and at line 31:
mainalready skipped same-reference sources unconditionally, so "unchanged source snapshots can skip the item walk" reads like a new optimisation. The user-visible fix is that a same-instance items array no longer swallows context, theme or forced-render updates to Element items. Worth rewording along those lines.- Both entries cite specs 047/058, but neither spec mentions
SetCollection, the three-callbackImperativeBridged, terminal slot teardown or incremental ItemsHost. Spec 058 still describes the old generated shape. Either add a short follow-up note to those specs, or link the guide section instead.
| list.SelectedIndex = 0; | ||
| H.Check("ItemsHost_PlainEvent_SelectionWorks", calls == 1); | ||
| calls = 0; | ||
| reconciler.UnmountChild(list); | ||
| await Harness.Render(); | ||
| H.Check("ItemsHost_PlainEvent_NoTerminalCallback", calls == 0); |
There was a problem hiding this comment.
This test would also pass if the terminal Clear() never raised SelectionChanged, because nothing shows that it does. The only thing stopping the callback is state.Element = null in Unbind, and that only matters if the clear raises the event. Could you add a positive control? For example, on a second mounted instance whose tag is still live: select an item, call list.Items.Clear() directly and assert calls == 1. Also assert list.Items.Count == 0 after unmount, as the ComboBox sibling test does.
| unmount: static (ctx, c) => | ||
| { | ||
| if (Reconciler.GetElementTag(c) is not ExpanderElement { HeaderTemplate: not null }) return; | ||
| if (c.Header is global::Microsoft.UI.Xaml.UIElement child) | ||
| ctx.Reconciler.UnmountChild(child); | ||
| c.Header = null; |
There was a problem hiding this comment.
No test runs this descriptor-side unmount. The built-in factory registers ExpanderHandler (Dsl.cs RegDecorator<ExpanderElement, ExpanderHandler>), so ItemsHost_Expander_HeaderCleanup only exercises ExpanderHandler.Unmount. Deleting this block would keep everything green. A variant of that fixture that registers ExpanderElement.Descriptor in an isolated registry, like the other descriptor-isolated selftests, would cover it. This matters because the ImperativeBridged XML doc now points to this as the reference pattern.
|
Thanks for the rerun and the detailed follow-up! Merged
Validation: Release host build with zero warnings/errors; all 70 targeted ItemsHost checks, 15 snippet-ledger tests, strict MkDocs build, and the 380 + 104 benchmark-helper assertions passed. Omitting tag clearing and descriptor header teardown caused the corresponding tests to fail; both mutations were restored and the targeted build/tests passed again. I have left the nested ItemsHost/header context-propagation gap for a separate follow-up, as suggested. No further performance changes were made in this round, and I did not rerun the full performance matrix or Native AOT suite. |
This branch has not been deployed
2 participants
Summary
Fix flat
ItemsHostreconciliation used by generated WCTSettingsExpanderwrappers. Dynamic cards now notify the repeater without rebuilding existing native children, preserving component state, focus, input drafts and animation players.ItemsHost.SetCollection; generate it only for a public writableIList<object>property. At mount, replace a non-notifying managed sink with a stable observable collection, while retaining native/read-only sinks and existing notifying collections.[WrapElementSlot]without replacing the descriptor author'sOnUnmountor clearing undeclared constructor defaults. Retain the existing two-callbackImperativeBridgedoverload.Linked issue / spec
Fixes #1362
Test plan
Local validation on Windows x64 (framework SDK 10.0.400; consuming app SDK 11.0.100-rc.1.26425.128):
dotnet test tests/Reactor.Tests --no-build -c Release -p:Platform=x64 --filter-class '*WrapperGeneratorTests*': 46 passed. The new writable-Items assertion fails when the generator DLL is replaced with the official preview.16 generator, and passes after restoring the fix.dotnet run --project tests/Reactor.AppTests.Host --no-build -c Release -p:Platform=x64 -- --self-test --filter ItemsHost_Observable: 15 assertions passed, including actual repeater realization after insertion/removal, keyed identity/state retention, child-owned state, enabled-state updates, Move/no Reset, exactly-once cleanup and Content/HeaderIcon detach.control-reconciler-protocolandextending-reactor-controls: passed (existing screenshots retained).dotnet restore Reactor.slnx: passed.dotnet build Reactor.slnx --no-restore -c Release -p:SkipSignaturesGen=true -m:1 -nodereuse:false: passed, 0 errors, including the packaged host that links the fixture. Two WIN2D0001 warnings report AnyCPU references in Reactor.Devtools / Chat.UI; no warning suppressions added.Risk / breaking changes
Two additive public APIs:
ItemsHost.SetCollectionand the three-callbackImperativeBridgedoverload. Existing signatures are retained. New generated code needs the matching runtime, both shipped in the core package.Flat-item behavior intentionally changes from wholesale rebuild to incremental reconciliation: Element identity is keyed or positional;
ItemEqualsapplies to raw values, not as a gate that skips Element descendants. The new collection setter must not target concrete nativeItemCollectionor read-only properties.Generated secondary slots now receive terminal teardown before descriptor
OnUnmount; custom descriptor callbacks must not unmount the same generated slot again. Verification covers x64 and loose package identity, not ARM64 or signed MSIX installation/upgrade.