fix(AdornerLayer): re-entrancy-safe lease for zOrderMap snapshot pool#5
Merged
oysteinkrog merged 2 commits intoMay 16, 2026
Merged
Conversation
Commit 7831813 ("wpf-perf(big-win T4): pool AdornerLayer._zOrderMap value snapshot") shipped a per-instance object[] snapshot buffer shared between MeasureOverride and ArrangeOverride to eliminate ~170 MB of per-pass DictionaryEntry[] allocations during MotionCatalyst take-open. Defect: Adorner.Measure / Adorner.Arrange callouts can re-enter the same AdornerLayer's MeasureOverride/ArrangeOverride via a nested layout pass. A naïve shared field lets the inner call's CopyTo overwrite the outer pass's snapshot, and its terminal Array.Clear nulls the slots the outer is still iterating — the outer then reads a null reference and the layout throws, leaving MotionCatalyst with a completely blank canvas on take-open. Fix: lease pattern. Each call captures the current field value into a local, immediately nulls the field (so any re-entrant call allocates its own buffer rather than aliasing), iterates on the local, and at end of pass restores its buffer to the field — keeping whichever buffer (own or the one a nested call left behind) is larger. Steady state on the non-re-entrant path remains zero-allocation: the field holds the grown buffer, every subsequent call leases-clears- copies-iterates-clears-restores in place. Re-entrant calls pay one object[] allocation per nesting level, matching the worst case of the pre-7831813a baseline. Validated end-to-end via MCP UI screenshots on MotionCatalyst: - HEAD before fix: take-open shows fully black canvas - HEAD + this fix: identical to vanilla upstream/release/10.0 (Carl Hansen golf swing, Frame 0/1240, both video viewports rendered, Pressure & Stance heatmap, Launch Monitor, all data boxes populated, playback toggles cleanly) All 358 perf commits in PRs #1-#4 preserved.
…eptions PR review feedback (gpt-5.5-pro): the lease pattern in commit 18eaca2 restored _zOrderValuesSnapshotBuffer only on normal exit. If an Adorner.Measure / Adorner.Arrange callout (or GetDesiredTransform, the property assignments, etc.) throws, the layer's pool field stays null and subsequent layout passes allocate again, regressing the perf win. The original NRE root cause (re-entrant CopyTo + terminal Array.Clear nulling outer-pass slots) was already fixed by the lease itself — the outer pass owns its local buffer and is immune to nested clobber. This patch adds defensive resource restoration for the exception path: the iteration runs under try, and Array.Clear + restore happen in finally. No behavior change on the happy path. Defense-in-depth for the unhappy path so the pool field invariant survives layout callouts that throw. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
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
1 participant
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.
Summary
Fixes a re-entrancy regression introduced by
7831813a(T4: poolAdornerLayer._zOrderMapvalue snapshot). Adorner.Measure / Adorner.Arrange callouts re-enter the sameAdornerLayer'sMeasureOverride/ArrangeOverridevia nested layout passes; with a naïve per-instance shared buffer, the inner call'sCopyTooverwrites the outer's snapshot and the inner's terminalArray.Clearnulls slots the outer call is still iterating — NullReferenceException → blank canvas.Fix
Lease pattern: take the shared buffer into a local at entry, null the field so a nested call sees no pool, restore on exit only if no nested call already stored a larger one. Steady-state stays zero-alloc (single buffer reused); only re-entrant nested levels allocate a private temporary.
Applied to both
MeasureOverrideandArrangeOverrideinsrc/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Documents/AdornerLayer.cs.Validation
Built the patched
PresentationFramework.dll, dropped it into theif.77nuget cache, ran the full MotionCatalyst build pipeline (InjectIfWpfAssembliesdeploys it toBUILD/x64_Debug/), launched MC via MCP UI broker against a hive that auto-reopens Carl Hansen Take 5, captured a screenshot:Test plan