perf: restore diff skip-path + cut DSL/element per-render allocations#665
Merged
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
3 tasks
azchohfi
added a commit
that referenced
this pull request
Jun 25, 2026
… (DIFF skip-path) The Element-layer diff skip-path (PR #665) skips a cell's update only when its event-handler delegates are reference-stable across renders, so the cached UseState/UseReducer setters (#43/#44) and UseCallback's deps-unchanged return (#46) are load-bearing for DIFF correctness, not just allocation. Add a consolidated regression guard that drives a realistic combined hook set (UseState + UseReducer + Redux dispatch + UseCallback) through 50 steady-state renders — passing a fresh handler lambda each render, as a real component does — and asserts every returned delegate is the same instance captured on render 0. Includes a precision check that a changed callback dep DOES yield a new instance so the skip-path correctly re-applies the handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Performance-focused update to Reactor’s Element/DSL layer to restore reconciler skip-path effectiveness (especially for interactive cells) and reduce per-render allocations in hot UI workloads (e.g., stress perf grids).
Changes:
- Refactors modifier equality to allow skip-path for reference-stable event handlers and fixes previously-uncompared modifier slots.
- Reduces allocations via attached-value single-entry dictionary, per-thread shared SolidColorBrush caching, and several DSL array/iteration fast paths.
- Adds/updates unit tests to lock down skip-path behavior and DSL correctness.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/PerfDiffSkipPathTests.cs | New regression tests for skip-path + allocation-cut scenarios across elements/modifiers/DSL/brush parsing. |
| tests/Reactor.Tests/DeclarativeModifierTests.cs | Updates event-handler equality expectations to match the new reference-equality skip-path rules. |
| src/Reactor/Core/Element.cs | Core changes to attached storage, shallow equality/setters comparison, and modifiers equality (layout/visual hoist + callback reference checks). |
| src/Reactor/Elements/ElementExtensions.cs | Routes many fluent modifiers through layout/visual buckets; adds ApplyStyle delegate caching; switches string brush modifiers to shared brush parse path. |
| src/Reactor/Elements/Dsl.cs | DSL micro-optimizations (UniformGrid/InterspersedGrid/ForEach/FilterChildren) to reduce per-render allocations. |
| src/Reactor/Elements/BrushHelper.cs | Introduces per-thread SolidColorBrush cache and shared parse path while keeping public Parse returning a fresh brush. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
azchohfi
added a commit
that referenced
this pull request
Jun 25, 2026
…ability Adds UseCallback_And_Setter_Satisfy_ReferenceEquals_Contract_DiffSkipPath, the literal ReferenceEquals form of the DIFF skip-path predicate (PR #665): deps unchanged => ReferenceEquals(prev,next)==true (cell skippable); deps changed => ==false (cell must re-apply). Complements the Assert.Same/NotSame coverage with the exact predicate the Element-layer skip path evaluates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
azchohfi
added a commit
that referenced
this pull request
Jun 25, 2026
…h sharing Review-pass fixes on PR #665 (pr-review skill + gpt-5.4 multi-model cross-check): H1 (correctness, HIGH) - ModifiersEqual let an element with a stable .OnTapped take the skip path without comparing OnUnmountAction. The skip path bypasses ApplyModifiers, which is what re-registers the latest teardown in the reconciler's _onUnmountActions on every Update; a changed .OnUnmount closure was therefore stranded and the stale first-render teardown would fire at unmount. Fixed by comparing OnUnmountAction by ReferenceEquals (mirrors the handler/.Ref slots). Zero grid-workload cost (cells have null teardown both renders). The plain-leaf no-handler case was a pre-existing latent bug on main; this also closes it (strictly more correct). Regression test added. H2 (multi-model, HIGH) - reverted #168 shared-brush sharing. main deliberately creates a fresh SolidColorBrush per call because a brush is a thread-affine DependencyObject with mutable Color/Opacity and cannot be safely shared across controls; an imperative .Set(tb => mutate tb.Background) would otherwise leak to every peer cell and poison the cache. Removed ParseShared/GetBrush/_brushCache; Background/Foreground/WithBorder(string) now use the fresh-brush Parse. Kept the safe ParseColor string->Color cache (pre-existing on main) and the #165/#157 ModifyVisual bucket-merge alloc win. Test-coverage (MEDIUM) - added a 27-slot ModifierCallbacksEqual sweep proving every event/gesture/drag-drop handler slot participates by reference (guards against a forgotten or mis-paired slot). Validation: src/Reactor Release x64 0 warn / 0 err (AOT-as-errors gate); tests/Reactor.Tests 9713 pass / 0 fail / 64 skip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two scoped cache-polish fixes from the dual review (Copilot + pr-review skill):
FIX A (Copilot, BrushHelper.cs:16): construct _colorCache with
StringComparer.OrdinalIgnoreCase so colors differing only in casing
("Red"/"red", "#FF0000"/"#ff0000") dedupe to one entry instead of creating
duplicates + re-running the factory per distinct casing. Safe: ParseColor
lowercases named colors before the switch and ParseHex is case-insensitive,
so case-folded keying yields identical results. ParseColor switch unchanged.
FIX B (pr-review, ElementExtensions.cs StyleApplier): bound _styleApplierCache
with a 256-entry cap as defense-in-depth against a pathological data-driven
caller passing unbounded distinct style names. A lock-free TryGetValue
fast-path preserves the #174 zero-alloc steady state (Count is only touched
on a miss); past the cap it falls back to the pre-#174 per-call delegate, so
correctness is unchanged and the cache stays bounded.
Test (BrushHelperTests): ParseColor_IsCaseInsensitive (value equality across
casing) + ParseColor_Cache_Dedupes_Across_Casing (allocation teeth: 500
distinct casings of one name collapse to a single entry => ~0 alloc).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 27, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #664
What & why
Flagship diff/memory PR for the Element/DSL layer. The data-grid stress workload (
StressPerf.ReactorOptimized, a StocksGrid) spends most of its time in DIFF/reconcile because the steady-state skip-path (ShallowEquals->CanSkipUpdate) is defeated for interactive grid cells, and the DSL/fluent layer allocates a cascade of short-lived objects per cell per frame. This restores the skip-path and cuts the per-cell allocations.Scope is limited to 4 source files (
Core/Element.cs,Elements/ElementExtensions.cs,Elements/Dsl.cs,Elements/BrushHelper.cs) plus tests, to avoid conflicts with sibling perf PRs.Changes
Skip-path (diff):
Element.cs) - replace the "any handler present => never skip" rule inModifiersEqualwithModifierCallbacksEqual, a per-slotReferenceEqualsover the 21 routed-input handler slots (size-changed; pointer ×8; tapped/double-tapped/right-tapped/holding; key-down/up + preview-key-down/up; character-received; got/lost-focus; access-key-display-requested). The gesture (Pan/Pinch/Rotate/LongPress) and drag-drop (DragSource/DropTarget) slots are deliberately excluded — main's diff never compared them, so a per-render gesture closure stays skip-eligible exactly as before; comparing them would force Update mid-interaction and re-arm an in-flight gesture (long-press double-fire). Excluding them preserves observable behavior exactly and costs nothing on the grid (cells use only routed handlers).OnUnmountActionis also compared by reference (re-registered every non-skip Update, fires at unmount). Interactive cells with reference-stable handlers now skip; capturing closures still force Update. Also closes a latent hole where most slots were ignored entirely. (See safety note below.)Element.cs) -SettersEqual<T>is safe-conservative: returns true only for the same array instance (a memoized/unchanged element) or both-empty. A.Set(x => ...)is an apply-time imperative write that must re-run on every update, so identity-equality must not be mistaken for "write unnecessary" - any element with setters stays on the Update path. (Reverted from an earlier element-wise-identity version after the pr-review flagged it as unsafe.)Element.cs) - hoistLayout/Visualsub-records once against shared_emptyLayout/_emptyVisualsentinels; compare via record equality. Fixes latent skip bugs (RequestedTheme/logical-insets and Scale/Rotation/Translation/CenterPoint were not compared).Per-render allocation cuts (memory):
Element.cs) -SetAttacheduses an immutable single-entrySingleAttachedDictionaryfor the common one-attached-value case (one.Gridper cell), avoiding aDictionary<Type,object>alloc per cell per frame.ElementExtensions.cs) -ModifyLayout/ModifyVisualbucket-merge entry points. ~36 pure-layout / pure-visual fluent modifiers (Margin/Padding/Width/Height/Min*/Max*/Align/Opacity/Scale/Rotation/CenterPoint/CornerRadius/Background/Foreground/WithBorder/Translation) merge their delta straight into the Layout/Visual slot instead of allocating a throwaway parentElementModifiers(and its bucket shim) per chained call. Proven equivalent toElementModifiers.Mergeand asserted by*_Chain_Equals_HandBuilttests. This is the dominant per-cell memory win.BrushHelper.cs) -ParseColorcaches the string->Colorparse (an immutable value), so hot fluent chains don't re-parse the color string. The cache uses anOrdinalIgnoreCasecomparer so colors differing only in casing ("Red"/"red","#FF0000"/"#ff0000") dedupe to one entry (safe:ParseColoralready lowercases named colors andParseHexis case-insensitive, so values are identical). The publicParse(string)returns a fresh, caller-ownedSolidColorBrushper call - unchanged vs main. An earlier revision added an internal shared per-thread brush cache (ParseShared/GetBrush) routed throughBackground/Foreground/WithBorder(string); that brush-sharing was reverted during review (see below) because aSolidColorBrushis a thread-affineDependencyObjectand main deliberately creates a fresh one per call. The color-parse cache is retained; the brush allocation is not eliminated (the fluent-cascade win comes from Sets the minimum supported version to 10.0.17763 #165/ci: bump fast-xml-parser and react-native-windows in /tests/startup_perf/BlankRNW #157).ElementExtensions.cs) -ApplyStylecaches its OnMount delegate per style name (memory-only;OnMountActionis excluded fromModifiersEqual, so this is not a skip-path change). The cache is bounded by a 256-entry cap (lock-freeTryGetValuefast-path preserves the zero-alloc steady state; over-cap falls back to the pre-Design proposal: FitParent — container-sized rebuild for charts and other size-baked elements #174 per-call delegate) as defense-in-depth against a pathological data-driven caller.Dsl.cs) -ForEachIReadOnlyListfast-path;UniformGridpre-sized Star-track loop (positions into its own array);InterspersedGridexact-sized arrays (readsitemswithout mutating);FilterChildrentwo-pass count+fill on the expansion path (the no-expansion fast path returns the caller's array unchanged, identical to main); shared statics_oneStar. fix(reconciler): templated ListView/GridView stale-closure refresh + feat(charting): rich Element labels with Canvas anchors #160 already satisfied (type-switch-first).FLAGSHIP-1 safety note
A pure presence-only handler compare is unsafe in this layer: modifier handlers dispatch through the reconciler's per-element
ModifierEventHandlerState.Current*fields, refreshed only on the non-skipUpdatepath. The skip path doesn't refresh them, so presence-only would strand a stale captured closure on a skipped cell. Per-slot reference-equality is the safe equivalent - when delegate identity is unchanged the staleCurrent*is the new delegate (identical dispatch). The full grid win comes from making cell handlers reference-stable (coordinated in sibling hooks/DataGrid sessions) so this reference-equality skip fires; no dispatch-side change is needed here. The predicate is scoped to the 21 routed-input slots only — gesture/drag-drop slots dispatch through separate state and were never diffed by main, so they stay out of the skip predicate to preserve behavior (see Pass 3).Review pass (this draft)
Ran the repo
pr-reviewskill (8 dimensions + a gpt-5.4 multi-model cross-check) and requested a GitHub Copilot review across multiple passes. Findings addressed:Pass 1:
UniformGridwrote positioned.Grid(...)cells back into the array returned byFilterChildren, whose fast path aliases the caller'sitemsarray - corrupting a caller-supplied array. Fixed: UniformGrid positions into its own array; added a no-mutation regression test. The otherFilterChildrenconsumers only read the result.ForEachtests asserted only counts. Fixed: now assert rendered Content/index sequence + empty-input; added headlessParseColorcoverage.Pass 2 (fresh pr-review on the committed branch):
ModifiersEquallet a stable-.OnTappedelement take the skip path without comparingOnUnmountAction. The skip path bypassesApplyModifiers, which is what re-registers the latest teardown (_onUnmountActions) every Update, so a changed.OnUnmountclosure was stranded and the stale first-render teardown would fire at unmount. Fixed: compareOnUnmountActionbyReferenceEquals(mirrors the handler/.Refslots). Zero grid-workload cost (cells have a null teardown both renders); also closes a pre-existing latent hole in the no-handler case. Regression test added.SolidColorBrushcache (ParseShared/GetBrush) aliased one mutable thread-affineDependencyObjectbrush across controls - an imperative.Set(tb => mutate tb.Background)would leak to every peer and poison the cache; main explicitly documents that brushes cannot be safely shared. Fixed: reverted to fresh-brushParse(matching main); kept the safeParseColorstring->Colorcache and the Sets the minimum supported version to 10.0.17763 #165/ci: bump fast-xml-parser and react-native-windows in /tests/startup_perf/BlankRNW #157 fluent-cascade win.ModifierCallbacksEqualsweep proving every routed-input handler slot participates by reference, with a count-lockAssert.Equal(21, …)guarding a forgotten or mis-paired slot, plus a companion behavior-preservation test asserting the gesture/drag-drop slots are intentionally not compared (match main).ModifyLayout/ModifyVisualfield-by-field equivalence toElementModifiers.Merge.Pass 3 (gesture-regression fix):
GestureTestsregression surfaced because an earlierModifierCallbacksEqualcompared all 27 callback slots, including the 6 gesture/drag-drop slots. Since main's diff never compared those, the extra comparison forced an Update mid-gesture and re-registered a long-press handler between its Began and Ended phases, double-firing the released callback. Fixed: narrowedModifierCallbacksEqualto the 21 routed-input slots and excluded gesture/drag-drop, restoring main's observable behavior exactly. Added a behavior-preservation guard test + the count-lock above. The 6 excluded slots give zero StocksGrid benefit (cells dispatch via routed pointer/tap handlers, not gestures); a future opt-in gesture skip-path is tracked in follow-up Reconciler skip-path omits gesture/drag slots → stale gesture/drag closure after a skipped update #721.Pass 4 (cache polish):
BrushHelper._colorCachekeyed case-sensitively, duplicating entries across casing. Fixed:OrdinalIgnoreCasecomparer (Fix A above) — value-identical, dedupes; teeth test added.ElementExtensions._styleApplierCachewas unbounded. Fixed: 256-entry cap (Fix B above) — defense-in-depth, steady-state zero-alloc fast-path preserved.Validation
dotnet build src\Reactor\Reactor.csproj -c Release -p:Platform=x64-> 0 warnings, 0 errors (AOT/trim-as-errors gate green).dotnet test tests/Reactor.Tests(x64) -> 9751 passed, 0 failed, 64 skipped, incl.PerfDiffSkipPathTests.cs(FLAGSHIP-1/-2, the OnUnmount skip-path regression, the 21-slot routed-handler sweep + the gesture/drag-drop exclusion guard, fix(tests): serialize PersistedStateCache tests to fix flake #159, perf(reconciler): unblock fast-path for Setters; fix CommandHost ambient chord-tooltip leak #155, Sets the minimum supported version to 10.0.17763 #165/ci: bump fast-xml-parser and react-native-windows in /tests/startup_perf/BlankRNW #157, Design proposal: FitParent — container-sized rebuild for charts and other size-baked elements #174, Ensure the compiled ReactorApplication.xaml gets embedded in the .PRI #168 ParseColor, Dsl docs(charting): document D3Pie params to silence CS1573 #170-173) andBrushHelperTests(case-insensitive color-cache dedupe).Deferred / out of scope
#167(ThemeBindings dictionary) - touches the Reconciler, outside this PR's 4-file scope; tracked with #156/#169 in follow-up #683.#158/#161-164(record Equals/GetHashCode override, cached structural hashes, CheckBox callback inlining) - deferred.#156/#169- investigated, no hot-path benefit. A brush non-aliasing selftest is tracked in #682; a future opt-in gesture/drag skip-path in #721.Perf validation runs via
/perfon this PR (the sibling CI workflow is now merged to main).