Skip to content

Commit c8139b9

Browse files
Centralize feature prompts by target release (dotnet#14767)
Centralize feature prompt documents Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 27022e2a-9eb8-4f58-9bfa-e79ca0d6c559
1 parent d28310f commit c8139b9

9 files changed

Lines changed: 965 additions & 0 deletions
Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
# Work Order: HighPrecisionTimer Rework (Animation Timing)
2+
3+
**Branch:** `Net11/Integration-2` (KlausLoeffelmann/winforms)
4+
**Files:**
5+
- `src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Animation/HighPrecisionTimer.cs`
6+
- `src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Animation/HighPrecisionTimerTick.cs`
7+
- `src/System.Windows.Forms.Primitives/tests/UnitTests/System/Windows/Forms/Animation/HighPrecisionTimerTests.cs`
8+
- Consumer: `src/System.Windows.Forms/System/Windows/Forms/Rendering/Animation/AnimationManager.cs`
9+
10+
This work order is self-contained; it results from a code review of the current implementation.
11+
Read the current sources first, verify each finding against the code as it stands (the branch may have
12+
moved), then implement.
13+
14+
**Keep as-is (explicitly not up for redesign):** the registration model — per-registration
15+
`SynchronizationContext` capture, `InFlight` CAS-based frame coalescing with a `DroppedFrames` counter
16+
surfaced in `HighPrecisionTimerTick`, and the id-based `TimerRegistration` disposable struct
17+
(double-dispose safe, `default` safe). The *clock/pacing core* is what gets replaced.
18+
19+
---
20+
21+
## Finding 1 (critical): fixed-cadence pacer + spin causes a sawtooth burning ~50% of a core
22+
23+
Current design: `PeriodicTimer` at 14 ms (60 Hz path), then `SpinToTarget` spins to the 16.667 ms
24+
frame target with `SpinOnce(sleep1Threshold: -1)` (never sleeps).
25+
26+
`PeriodicTimer` fires on its own **fixed** cadence (14, 28, 42, 56, …) and does not re-phase per
27+
`WaitForNextTickAsync` call, while frame targets are 16.67, 33.33, 50, 66.67, … The phase slips
28+
2.67 ms per frame, so spin duration grows every frame — wake 28 → spin 5.3 ms; wake 42 → spin 8 ms;
29+
wake 56 → spin 10.7 ms — until phases wrap and the sawtooth restarts. Average spin ≈ half a frame
30+
≈ 8 ms of every 16.67 ms ⇒ ~50% of one core, continuously, for as long as **any** animation is
31+
registered. Infinite cycles (e.g. a pulsing focus indicator) make this permanent. The 30 Hz path
32+
(30 ms tick vs 33.33 ms frame) has the identical slip.
33+
34+
**Required fix (architecture, not constant-tuning):** absolute schedule. Due times are
35+
`epoch + frameIndex * period` against a single clock; wait on a mechanism that can hit them
36+
(Finding 3), with at most a sub-millisecond residual spin. Overshoot must be amortized against the
37+
absolute schedule (no `lastTick + period` relative scheduling — that accumulates and runs slow).
38+
39+
## Finding 2: integer-millisecond arithmetic throughout
40+
41+
`stopwatch.ElapsedMilliseconds` (truncated `long`) feeds `lastTickTimestamp`, `elapsed`, the spin
42+
target, drift detection, and the `Timestamp`/`Elapsed` values delivered to consumers — ±1 ms
43+
quantization (6% of a 16.667 ms budget) plus systematic truncation bias. Use `ElapsedTicks` /
44+
`Elapsed.TotalMilliseconds` (double) end to end; `HighPrecisionTimerTick` fields stay `TimeSpan`,
45+
constructed from ticks.
46+
47+
## Finding 3: replace `timeBeginPeriod(1)` with a high-resolution waitable timer
48+
49+
The code gates on `windows10.0.17134` — which `timeBeginPeriod` (ancient) does not need, but which is
50+
exactly the build (1803) that introduced `CreateWaitableTimerExW` with
51+
`CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`. Use it:
52+
53+
- absolute due times (negative-relative or absolute FILETIME) with sub-ms accuracy,
54+
- no process-wide timer-resolution raise (current code holds 1 ms resolution for the entire lifetime
55+
of any animation — a documented power/battery anti-pattern, and post-Win11 the effective resolution
56+
changes for occluded windows, silently shifting the timing floor),
57+
- composes directly with Finding 1's absolute schedule and eliminates the spin loop almost entirely.
58+
59+
Fallback below 17134: keep a coarse path (30 Hz, plain waits, no `timeBeginPeriod`) — document that
60+
sub-frame precision is not attempted there. Remove `TimeBeginPeriod`/`TimeEndPeriod` P/Invokes if no
61+
longer referenced.
62+
63+
## Finding 4: `Register`/`Unregister` vs `StopTimer` race strands registrations
64+
65+
`Unregister` does `TryRemove``IsEmpty?``StopTimer()` without coordinating with `Register`.
66+
Interleaving: A removes the last entry and observes empty; B adds a registration and `EnsureRunning`
67+
sees `s_loopTask != null` (still running) and returns; A stops the timer. Result: live registration,
68+
dead timer — animation frozen until an unrelated `Register` restarts the loop.
69+
**Fix:** perform the emptiness check + stop decision under `s_lock` together with loop-state
70+
transitions, or introduce a generation counter that `StopTimer` validates before actually stopping.
71+
72+
## Finding 5: per-frame, per-registration closure allocations on the hot path
73+
74+
`SyncContext.Post(_ => _ = InvokeCallbackAsync(registration, tick, cancellationToken), null)`
75+
allocates closure + delegate per registration per frame (60 Hz × N renderers of steady GC pressure).
76+
**Fix:** one cached `static SendOrPostCallback`; pass state via a per-registration state object —
77+
`InFlight` guarantees exclusivity, so tick data can be written into a reusable per-registration slot
78+
before posting. Target: zero allocations per frame in steady state.
79+
80+
## Finding 6: drift `Debug.Assert` is an assert storm
81+
82+
Drift >20% for 10 frames is normal under a debugger, breakpoints, or CI load. Once tripped,
83+
`consecutiveDriftFrames` keeps incrementing, so the assert fires **every subsequent frame**.
84+
**Fix:** replace with tracing/EventSource counters (drift, dropped frames, spin time). If any assert
85+
remains, reset the counter after firing once.
86+
87+
## Finding 7: lifecycle edges
88+
89+
- `StopTimer` never observes `s_loopTask`; a stop/start pair can transiently run two loops, and a
90+
stopping loop can dispatch one final frame with a canceled token. Decide and document: either join
91+
the old loop (bounded) or make late dispatch provably benign.
92+
- Post-unregister ticks can still be in flight toward a disposed consumer; `AnimationManager` /
93+
`AnimatedControlRenderer` must tolerate late callbacks — add a test.
94+
- `Reset()` (test hook) mutates state without locking — document the serialization requirement or
95+
lock it.
96+
- Dead code: `Registration.Id` is never read.
97+
- `InvokeCallbackAsync` swallows non-OCE exceptions with `Debug.Fail` and keeps invoking the same
98+
callback forever; consider auto-unregistering a registration after N consecutive faults.
99+
100+
## Finding 8: single-SyncContext funnel in `AnimationManager` (design note)
101+
102+
`HighPrecisionTimer` correctly captures a `SynchronizationContext` **per registration**, but
103+
`AnimationManager` is a process-wide singleton with a single registration, so every animation in a
104+
multi-message-loop application marshals to the first UI thread — and dies with it. Minimum: document
105+
the constraint. Better: make the manager per-UI-thread (e.g. `[ThreadStatic]` instance keyed off the
106+
message-loop thread), preserving one timer-registration-per-UI-thread. Also note the duplicate
107+
timeline: the manager keeps its own `Stopwatch` instead of deriving progress from
108+
`HighPrecisionTimerTick.Timestamp/Elapsed`; animation progress should use the tick's timeline so frame
109+
coalescing (`DroppedFrames`) is accounted for consistently.
110+
111+
## Finding 9: 60 Hz is a settled ceiling; the period is a pacer-owned runtime value for *downshift*
112+
113+
**Decision (do not relitigate):** the timer targets a hard 60 Hz ceiling (30 Hz fallback). Do not
114+
add refresh-rate matching or raise the cadence. Rationale, for the record:
115+
116+
- Under DWM, windowed GDI/GDI+ apps do not tear (composition is tear-free from the redirection
117+
surface); the only artifact of an unsynced 60 Hz timer is judder, which is imperceptible for this
118+
content class (focus pulses, hover fades, toggle transitions — not motion/scrolling).
119+
- GDI+ raster cost scales linearly with rate; driving N animated controls at 120–144 Hz multiplies
120+
compute for no perceptible gain (e.g. batched MVVM-driven updates across many controls).
121+
- A process-wide timer cannot refresh-match on mixed-rate multi-monitor setups ("the" refresh rate
122+
is ill-defined), and `DwmFlush`-style vblank pacing blocks per frame, binds to one monitor, and
123+
misbehaves under RDP — unfit for a process-wide UI timer by construction.
124+
125+
**Required now (structural):** the frame period must be a runtime value owned by the pacer
126+
(queryable/settable internally), not compile-time constants woven through the loop. The 30 Hz
127+
fallback already makes the period variable; the forward-looking motivation is **downshifting**, not
128+
matching: future power-driven reductions (30 Hz or full pause for occluded/minimized windows — where
129+
Windows 11 timer coalescing already alters the effective cadence — and battery-saver scenarios)
130+
must be addable without another rework.
131+
132+
---
133+
134+
## Acceptance criteria
135+
136+
1. Steady-state CPU of the timer loop with one registered infinite animation: **< 2% of one core**
137+
(measure; the current implementation is the ~50% baseline per Finding 1).
138+
2. No `timeBeginPeriod` while animations run (verify via `powercfg /energy` or timer-resolution
139+
query on a Win10 1803+ box).
140+
3. Frame delivery: mean interval within ±0.5 ms of target over a 10 s run on an idle machine at
141+
60 Hz; no monotonic slow drift (absolute-schedule check: 600th frame due time within one frame of
142+
`epoch + 600 × period`).
143+
4. Zero per-frame heap allocations in steady state (verify with an allocation-tracking test or
144+
`GC.GetAllocatedBytesForCurrentThread` bracketing).
145+
5. Race test for Finding 4: concurrent register/unregister stress leaves no registration without a
146+
running loop.
147+
6. Existing `HighPrecisionTimerTests` updated/extended accordingly; late-callback tolerance test for
148+
consumers (Finding 7).
149+
7. `HighPrecisionTimerTick` surface unchanged (internal consumers depend on it); all other churn is
150+
internal to the pacer.
151+
152+
## Constraints
153+
154+
- Everything stays `internal`; no public API review implications.
155+
- Follow repo conventions (`LibraryImport`, nullable enabled, existing XML-doc voice).
156+
- Windows-only paths gated with `[SupportedOSPlatform]` / `OperatingSystem.IsWindowsVersionAtLeast`
157+
as currently practiced in the file.
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
# Work Order: SystemVisualSettings (Implementation)
2+
3+
**Branch:** `Net11/Integration-2` (KlausLoeffelmann/winforms)
4+
**Scope:** Code changes only. The GitHub proposal issue is handled by a separate work order
5+
(`ApiReview-IssueUpdates.md`, Section 3) and run only after this implementation stands.
6+
**Supersedes:** `Application.GetWindowsAccentColor` and the standalone accessibility text-size change
7+
event currently on the branch (see item 6).
8+
9+
---
10+
11+
## Background
12+
13+
Windows delivers visual/accessibility setting changes through four channels — `WM_SETTINGCHANGE`,
14+
`WM_DWMCOLORIZATIONCOLORCHANGED`, `WM_THEMECHANGED`, `WM_SYSCOLORCHANGE` — and today each consumer
15+
normalizes that zoo individually. This work order introduces one typed, read-only snapshot plus one
16+
unified change notification, with a **leak-free consumption path for controls** (virtual cascade, no
17+
static-event subscription — the `SystemEvents.UserPreferenceChanged` leak class must be structurally
18+
impossible for the default path).
19+
20+
Deliberate non-goals, enforced by the type system: **no settable properties.** Perceptual adjustments
21+
(contrast, color filtering, text scale, focus prominence, motion) are user-owned via Windows
22+
accessibility settings; app-side theming is the business of control vendor partners. Renderers derive
23+
output from this snapshot combined with `EffectiveVisualStylesMode`.
24+
25+
---
26+
27+
## 1. New types (`System.Windows.Forms`)
28+
29+
```csharp
30+
public sealed class SystemVisualSettings
31+
{
32+
public Color AccentColor { get; }
33+
public float TextScaleFactor { get; } // 1.0–2.25, Windows a11y text scale
34+
public bool HighContrastEnabled { get; }
35+
public bool ClientAreaAnimationEnabled { get; } // SPI_GETCLIENTAREAANIMATION
36+
public bool KeyboardCuesVisible { get; } // SPI_GETKEYBOARDCUES (system default)
37+
public Size FocusBorderMetrics { get; } // SPI_GETFOCUSBORDERWIDTH/HEIGHT, pixels
38+
}
39+
40+
[Flags]
41+
public enum SystemVisualSettingsCategories
42+
{
43+
None = 0,
44+
AccentColor = 1 << 0,
45+
TextScale = 1 << 1,
46+
HighContrast = 1 << 2,
47+
Animations = 1 << 3,
48+
KeyboardCues = 1 << 4,
49+
FocusMetrics = 1 << 5
50+
}
51+
52+
public class SystemVisualSettingsChangedEventArgs : EventArgs
53+
{
54+
public SystemVisualSettings OldSettings { get; }
55+
public SystemVisualSettings NewSettings { get; }
56+
public SystemVisualSettingsCategories Changed { get; }
57+
}
58+
```
59+
60+
Immutable snapshot semantics — values must not shift under an event handler comparing old vs new.
61+
XML remarks per the design notes: `HighContrastEnabled` cross-references the effective-mode clamp;
62+
`KeyboardCuesVisible` documents the system-default vs per-window (`WM_UPDATEUISTATE`) distinction;
63+
`FocusBorderMetrics` is documented as the baseline input for border/focus prominence in renderers
64+
(scaled for DPI and `TextScaleFactor`), replacing fixed constants; the class remarks state the
65+
no-app-side-overrides principle explicitly.
66+
67+
## 2. `Application` surface
68+
69+
```csharp
70+
public static SystemVisualSettings SystemVisualSettings { get; }
71+
public static event EventHandler<SystemVisualSettingsChangedEventArgs>? SystemVisualSettingsChanged;
72+
```
73+
74+
Event XML remarks must state: (a) raised once per settings transition, normalized across the four
75+
underlying messages; (b) handlers should early-out via `e.Changed`; (c) **audience is app-lifetime
76+
consumers** (theming engines, services); components with shorter lifetime than the application must
77+
unsubscribe; **controls and forms should use the `Control`-level virtual/instance event instead,
78+
which requires no unsubscription** (see item 3). This positioning is the leak fix — make the
79+
leak-free path the documented default.
80+
81+
## 3. `Control`-level consumption (the leak-free path)
82+
83+
```csharp
84+
protected virtual void OnSystemVisualSettingsChanged(SystemVisualSettingsChangedEventArgs e);
85+
public event EventHandler<SystemVisualSettingsChangedEventArgs>? SystemVisualSettingsChanged;
86+
```
87+
88+
- Model the cascade on the existing `OnSystemColorsChanged` pattern in `Control.cs`: virtual
89+
dispatch parent→children, instance event raised from within the virtual. No subscription to any
90+
static event exists anywhere in this path — lifetime coupling is structural.
91+
- Keep the cascade pattern-identical to `OnVisualStylesModeChanged` / `OnParentVisualStylesModeChanged`
92+
(see the VisualStylesMode impact work order). A `HighContrast` category change resolves as an
93+
effective-visual-styles-mode change for affected controls and must route through that machinery's
94+
early-out/dispatch — no duplicate HC handling in this cascade.
95+
- **Remove/replace the existing Form-level replicated text-size event**: it generalizes into this
96+
cascade and disappears as a special case. Migrate in-box usages.
97+
- Staleness rule (document in XML remarks, mirroring `OnSystemColorsChanged` folklore — this time
98+
written down): the cascade only reaches parented controls; a control created but not yet parented
99+
misses transitions and must re-query `Application.SystemVisualSettings` on handle creation /
100+
`OnParentChanged`.
101+
102+
## 4. Message plumbing and normalization
103+
104+
Central internal tracker (e.g. `SystemVisualSettingsTracker`) holding the current snapshot:
105+
106+
- Every **top-level** window already receives the four raw messages; handle them in the existing
107+
top-level `WndProc` paths.
108+
- On receipt: the window asks the tracker to re-query. The **first** arriver computes the diff
109+
against the current snapshot, atomically swaps it (`Interlocked` reference swap), raises the static
110+
`Application` event **once**, and cascades into its own tree. Subsequent top-levels re-query, see
111+
no diff, raise nothing at Application level, but **still cascade into their own trees** using the
112+
already-computed args.
113+
- Threading falls out for free: each tree is notified on the thread owning its top-level — no
114+
marshaling, correct for multi-message-loop applications. Do not centralize onto one thread.
115+
- Coalesce message storms: a single user action can produce several of the four messages; debounce
116+
within a message-pump iteration (re-query once per burst per top-level, not per message).
117+
118+
## 5. In-box consumption cleanup
119+
120+
- Audit in-box `Microsoft.Win32.SystemEvents` subscriptions (`UserPreferenceChanged` et al.) in
121+
controls/renderers; migrate those covered by the new categories to the cascade. Document any that
122+
must remain (categories outside this surface) — do not expand the snapshot to chase them in this
123+
work order.
124+
- `TextBoxBase` Net11 border rendering: consume `FocusBorderMetrics` + `TextScaleFactor` as the
125+
border-prominence input where fixed constants are currently used (coordinate with the animation /
126+
focus-indicator renderer as applicable).
127+
- Animated renderers (`AnimatedControlRenderer` / `AnimationManager`): honor
128+
`ClientAreaAnimationEnabled == false` by rendering final state immediately and suppressing
129+
transitions; react to the `Animations` category change at runtime.
130+
131+
## 6. Supersede the piecemeal APIs
132+
133+
- `Application.GetWindowsAccentColor``Application.SystemVisualSettings.AccentColor`. The method
134+
has not shipped stable: **remove it** on this branch (preferred) rather than obsoleting, to avoid
135+
two sources of truth. If removal is blocked by preview-compat policy, `[Obsolete]` with pointer.
136+
- The standalone text-size change event (Application- and/or Form-level) → `Changed.HasFlag(TextScale)`
137+
on the unified event / cascade. Same removal-vs-obsolete decision, same preference.
138+
- Migrate all in-box call sites.
139+
140+
## 7. Tests
141+
142+
- Snapshot immutability and correct SPI mapping per property (mock/native-shim as the repo's test
143+
infra allows).
144+
- Normalization: N top-level windows + one settings transition ⇒ exactly one Application-level raise;
145+
every window's tree cascaded exactly once; delivery on each tree's own thread.
146+
- Flags correctness per category, including multi-category transitions (HC toggle typically changes
147+
colors + HC + metrics in one burst — must coalesce to one event with combined flags).
148+
- Leak test: create/dispose forms subscribing to the **control-level** event in a loop; assert
149+
collectability (`WeakReference`), proving no static rooting. Counter-test documenting that the
150+
static event does root (expected, documented behavior).
151+
- HC toggle end-to-end: cascade triggers effective-mode change path once, no duplicate layout.
152+
153+
## Constraints
154+
155+
- All new public surface XML-documented in the branch's voice; internal tracker fully internal.
156+
- No settable members anywhere on the new types — if implementation pressure suggests one, stop and
157+
flag rather than adding it.
158+
- Windows-only P/Invoke via `LibraryImport`, gated per existing repo practice.

0 commit comments

Comments
 (0)