Skip to content

Perf: Lazily construct ShortcutControl's edit dialog - #50920

Closed
Morten Nielsen (dotMorten) wants to merge 1 commit into
microsoft:mainfrom
dotMorten:fix/shortcutcontrol-lazy-dialog-perf
Closed

Morten Nielsen (dotMorten) wants to merge 1 commit into
microsoft:mainfrom
dotMorten:fix/shortcutcontrol-lazy-dialog-perf

Conversation

@dotMorten

@dotMorten Morten Nielsen (dotMorten) commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The Settings UI has noticeable slowdowns when navigating between pages. Profiling with a WinUI 3 ETW-based perf tool (capturing full navigation through all Settings pages) identified ShortcutControl — a control instantiated once per module's activation-shortcut row, and therefore present on nearly every Settings page — as the single largest source of parsing/allocation cost and GC-pause spikes.

Root cause

ShortcutControl unconditionally built its entire "Edit shortcut" dialog UI (a ShortcutDialogContentControl wrapped in a ContentDialog) at construction time via an eager field initializer and constructor-time allocation:

private ShortcutDialogContentControl c = new ShortcutDialogContentControl();
...
shortcutDialog = new ContentDialog { Content = c, ... };

That dialog content is only ever read from OpenDialogButton_Click and the dialog's own lifecycle callbacks (Opened/PrimaryButtonClick/Closing) — i.e. it's only needed if/when the user actually clicks "Edit" on that specific shortcut row. Every other page load paid the full cost of parsing ShortcutDialogContentControl.xbf and allocating the dialog graph for nothing.

Because ShortcutControl appears multiple times on several pages (e.g. CropAndLock, FancyZones), this eager work stacked up as a "death by a thousand cuts" — each individual instance wasn't dramatic, but combined across a page (or across a full navigation session) it showed up as large blocking GC suspension pauses (up to 355ms) and was, by far, the top XAML-parsing cost in the app.

Fix

  • Made ShortcutDialogContentControl c and ContentDialog shortcutDialog lazily constructed via a new EnsureDialogCreated() method, called only from OpenDialogButton_Click right before the dialog is shown.
  • Removed the eager field initializer and the eager constructor-time ContentDialog construction/event wiring.
  • Removed a redundant c.Keys = ... assignment in the HotkeySettings setter (the dialog already refreshes c.Keys fresh every time it's opened).
  • Added an isLoaded tracking field and null-guarded the dialog-specific event wiring in ShortcutControl_Loaded/ShortcutControl_Unloaded, since virtualized list containers can cycle Loaded/Unloaded multiple times while the underlying ShortcutControl (and its lazily-created dialog) persists — this ensures the dialog's events still get wired correctly whether it's created before or after the control is loaded.

No behavioral changes to the dialog itself — same fields, same buttons, same Save/Cancel/Reset/Clear/key-capture behavior. Verified via UI automation + screenshot that the "Edit shortcut" dialog still opens and functions identically after the change.

Performance: before vs. after

Measured with an identical 24-page Settings navigation script, captured via ETW trace analysis before and after the fix:

Metric Before After Change
ShortcutDialogContentControl.xbf parsing (top cost, 20 instances) 553.2ms (~27.6ms/instance) 0ms — no longer parsed Eliminated
ShortcutControl.xbf parsing (unavoidable row control) 40.0ms / 20 instances (~2.0ms each) 65.1ms / 39 instances (~1.67ms each) ~17% cheaper per instance, despite more instances
Top overall parsing resource 553ms (the dialog content) 65ms (the row control itself) #1 cost removed entirely
Worst single GC blocking-suspension pause 355ms 106ms ~70% shorter worst-case UI freeze
CropAndLockPage / FancyZonesPage SettingsCard self-time spikes (380–520ms, GC-suspension-aligned) Present Not reproduced Consistent with reduced allocation pressure

Caveat: both captures hit the trace tool's evidence-size cap (not fully exhaustive), and raw wall-clock/idle-time numbers between the two runs aren't directly comparable due to shared-machine variance during capture. However, the parsing-resource breakdown (tied to number/cost of distinct XAML parses, not wall time) and peak GC-suspension durations are reliable, reproducible indicators — and both show the same result: the single biggest parsing cost in the entire Settings app is fully eliminated, and worst-case UI-thread stalls caused by GC are cut by roughly 70%.

Testing

  • Rebuilt PowerToys.Settings.csproj (Debug/x64) — 0 errors.
  • Re-ran the identical full-navigation perf capture after the fix and compared results (see table above).
  • Manually verified via UI automation + screenshot: navigated to a page with a shortcut row, clicked "Edit", confirmed the dialog opens with correct current keys, and that Save/Cancel/Reset/Clear/key-capture all still work as expected.

…UI slowdowns

ShortcutControl (used once per module's activation-shortcut row, appearing on
nearly every Settings page) unconditionally built a full
ShortcutDialogContentControl + wrapping ContentDialog at construction time,
even though that UI is only needed when the user clicks "Edit" to open the
shortcut-editing dialog. This eagerly parsed ShortcutDialogContentControl.xbf
and allocated a full dialog object graph for every shortcut row on every
page load.

This change defers construction of the dialog content control and the
ContentDialog to a new EnsureDialogCreated() method, called only from
OpenDialogButton_Click. Loaded/Unloaded event wiring for the dialog is now
null-guarded so it still works correctly across virtualized
Loaded/Unloaded cycles, with or without the dialog created yet.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions github-actions Bot added Product-Settings The standalone PowerToys Settings application Ready for review labels Sep 30, 2026
@github-actions

Copy link
Copy Markdown

🧭 PR intake

Visual evidence: Not needed — The changed files do not indicate a visible UI change. No visual evidence is expected.

Recommendation

Link the issue this PR fixes using a closing keyword such as Closes #123.

✅ Ready for review

This PR passed the automated intake checks and is ready for maintainer review.

Automated PR intake; PowerToys maintainers make final decisions.

@niels9001

Copy link
Copy Markdown
Collaborator

Thanks for the detailed profiling and performance evidence here. This implements the same core optimization as the earlier #50532: defer construction of ShortcutDialogContentControl and its ContentDialog until the editor is first opened.

We’re keeping #50532 open as the canonical implementation because it also covers the surrounding lifecycle behavior that becomes important once construction is deferred, including:

  • applying the latest AllowDisable description when the editor is created;
  • preserving editor-owned handlers such as Learn More across Loaded/Unloaded cycles;
  • restoring HotkeySettings.PropertyChanged subscriptions and conflict state after reload;
  • hiding/deactivating an existing dialog and detaching from the exact subscribed window on unload/dispose;
  • routing Reset/Clear replacement settings through the property setter; and
  • automated WinUI lifecycle/state coverage for first open, reopen, unload/reload, and save/cancel/reset/clear paths.

Morten's ETW investigation and before/after measurements are valuable evidence for the optimization, and we should carry that context into #50532. Closing this one as a duplicate implementation in favor of #50532.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Product-Settings The standalone PowerToys Settings application Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants