Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
# Work Order: HighPrecisionTimer Rework (Animation Timing)

**Branch:** `Net11/Integration-2` (KlausLoeffelmann/winforms)
**Files:**
- `src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Animation/HighPrecisionTimer.cs`
- `src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Animation/HighPrecisionTimerTick.cs`
- `src/System.Windows.Forms.Primitives/tests/UnitTests/System/Windows/Forms/Animation/HighPrecisionTimerTests.cs`
- Consumer: `src/System.Windows.Forms/System/Windows/Forms/Rendering/Animation/AnimationManager.cs`

This work order is self-contained; it results from a code review of the current implementation.
Read the current sources first, verify each finding against the code as it stands (the branch may have
moved), then implement.

**Keep as-is (explicitly not up for redesign):** the registration model — per-registration
`SynchronizationContext` capture, `InFlight` CAS-based frame coalescing with a `DroppedFrames` counter
surfaced in `HighPrecisionTimerTick`, and the id-based `TimerRegistration` disposable struct
(double-dispose safe, `default` safe). The *clock/pacing core* is what gets replaced.

---

## Finding 1 (critical): fixed-cadence pacer + spin causes a sawtooth burning ~50% of a core

Current design: `PeriodicTimer` at 14 ms (60 Hz path), then `SpinToTarget` spins to the 16.667 ms
frame target with `SpinOnce(sleep1Threshold: -1)` (never sleeps).

`PeriodicTimer` fires on its own **fixed** cadence (14, 28, 42, 56, …) and does not re-phase per
`WaitForNextTickAsync` call, while frame targets are 16.67, 33.33, 50, 66.67, … The phase slips
2.67 ms per frame, so spin duration grows every frame — wake 28 → spin 5.3 ms; wake 42 → spin 8 ms;
wake 56 → spin 10.7 ms — until phases wrap and the sawtooth restarts. Average spin ≈ half a frame
≈ 8 ms of every 16.67 ms ⇒ ~50% of one core, continuously, for as long as **any** animation is
registered. Infinite cycles (e.g. a pulsing focus indicator) make this permanent. The 30 Hz path
(30 ms tick vs 33.33 ms frame) has the identical slip.

**Required fix (architecture, not constant-tuning):** absolute schedule. Due times are
`epoch + frameIndex * period` against a single clock; wait on a mechanism that can hit them
(Finding 3), with at most a sub-millisecond residual spin. Overshoot must be amortized against the
absolute schedule (no `lastTick + period` relative scheduling — that accumulates and runs slow).

## Finding 2: integer-millisecond arithmetic throughout

`stopwatch.ElapsedMilliseconds` (truncated `long`) feeds `lastTickTimestamp`, `elapsed`, the spin
target, drift detection, and the `Timestamp`/`Elapsed` values delivered to consumers — ±1 ms
quantization (6% of a 16.667 ms budget) plus systematic truncation bias. Use `ElapsedTicks` /
`Elapsed.TotalMilliseconds` (double) end to end; `HighPrecisionTimerTick` fields stay `TimeSpan`,
constructed from ticks.

## Finding 3: replace `timeBeginPeriod(1)` with a high-resolution waitable timer

The code gates on `windows10.0.17134` — which `timeBeginPeriod` (ancient) does not need, but which is
exactly the build (1803) that introduced `CreateWaitableTimerExW` with
`CREATE_WAITABLE_TIMER_HIGH_RESOLUTION`. Use it:

- absolute due times (negative-relative or absolute FILETIME) with sub-ms accuracy,
- no process-wide timer-resolution raise (current code holds 1 ms resolution for the entire lifetime
of any animation — a documented power/battery anti-pattern, and post-Win11 the effective resolution
changes for occluded windows, silently shifting the timing floor),
- composes directly with Finding 1's absolute schedule and eliminates the spin loop almost entirely.

Fallback below 17134: keep a coarse path (30 Hz, plain waits, no `timeBeginPeriod`) — document that
sub-frame precision is not attempted there. Remove `TimeBeginPeriod`/`TimeEndPeriod` P/Invokes if no
longer referenced.

## Finding 4: `Register`/`Unregister` vs `StopTimer` race strands registrations

`Unregister` does `TryRemove` → `IsEmpty?` → `StopTimer()` without coordinating with `Register`.
Interleaving: A removes the last entry and observes empty; B adds a registration and `EnsureRunning`
sees `s_loopTask != null` (still running) and returns; A stops the timer. Result: live registration,
dead timer — animation frozen until an unrelated `Register` restarts the loop.
**Fix:** perform the emptiness check + stop decision under `s_lock` together with loop-state
transitions, or introduce a generation counter that `StopTimer` validates before actually stopping.

## Finding 5: per-frame, per-registration closure allocations on the hot path

`SyncContext.Post(_ => _ = InvokeCallbackAsync(registration, tick, cancellationToken), null)`
allocates closure + delegate per registration per frame (60 Hz × N renderers of steady GC pressure).
**Fix:** one cached `static SendOrPostCallback`; pass state via a per-registration state object —
`InFlight` guarantees exclusivity, so tick data can be written into a reusable per-registration slot
before posting. Target: zero allocations per frame in steady state.

## Finding 6: drift `Debug.Assert` is an assert storm

Drift >20% for 10 frames is normal under a debugger, breakpoints, or CI load. Once tripped,
`consecutiveDriftFrames` keeps incrementing, so the assert fires **every subsequent frame**.
**Fix:** replace with tracing/EventSource counters (drift, dropped frames, spin time). If any assert
remains, reset the counter after firing once.

## Finding 7: lifecycle edges

- `StopTimer` never observes `s_loopTask`; a stop/start pair can transiently run two loops, and a
stopping loop can dispatch one final frame with a canceled token. Decide and document: either join
the old loop (bounded) or make late dispatch provably benign.
- Post-unregister ticks can still be in flight toward a disposed consumer; `AnimationManager` /
`AnimatedControlRenderer` must tolerate late callbacks — add a test.
- `Reset()` (test hook) mutates state without locking — document the serialization requirement or
lock it.
- Dead code: `Registration.Id` is never read.
- `InvokeCallbackAsync` swallows non-OCE exceptions with `Debug.Fail` and keeps invoking the same
callback forever; consider auto-unregistering a registration after N consecutive faults.

## Finding 8: single-SyncContext funnel in `AnimationManager` (design note)

`HighPrecisionTimer` correctly captures a `SynchronizationContext` **per registration**, but
`AnimationManager` is a process-wide singleton with a single registration, so every animation in a
multi-message-loop application marshals to the first UI thread — and dies with it. Minimum: document
the constraint. Better: make the manager per-UI-thread (e.g. `[ThreadStatic]` instance keyed off the
message-loop thread), preserving one timer-registration-per-UI-thread. Also note the duplicate
timeline: the manager keeps its own `Stopwatch` instead of deriving progress from
`HighPrecisionTimerTick.Timestamp/Elapsed`; animation progress should use the tick's timeline so frame
coalescing (`DroppedFrames`) is accounted for consistently.

## Finding 9: 60 Hz is a settled ceiling; the period is a pacer-owned runtime value for *downshift*

**Decision (do not relitigate):** the timer targets a hard 60 Hz ceiling (30 Hz fallback). Do not
add refresh-rate matching or raise the cadence. Rationale, for the record:

- Under DWM, windowed GDI/GDI+ apps do not tear (composition is tear-free from the redirection
surface); the only artifact of an unsynced 60 Hz timer is judder, which is imperceptible for this
content class (focus pulses, hover fades, toggle transitions — not motion/scrolling).
- GDI+ raster cost scales linearly with rate; driving N animated controls at 120–144 Hz multiplies
compute for no perceptible gain (e.g. batched MVVM-driven updates across many controls).
- A process-wide timer cannot refresh-match on mixed-rate multi-monitor setups ("the" refresh rate
is ill-defined), and `DwmFlush`-style vblank pacing blocks per frame, binds to one monitor, and
misbehaves under RDP — unfit for a process-wide UI timer by construction.

**Required now (structural):** the frame period must be a runtime value owned by the pacer
(queryable/settable internally), not compile-time constants woven through the loop. The 30 Hz
fallback already makes the period variable; the forward-looking motivation is **downshifting**, not
matching: future power-driven reductions (30 Hz or full pause for occluded/minimized windows — where
Windows 11 timer coalescing already alters the effective cadence — and battery-saver scenarios)
must be addable without another rework.

---

## Acceptance criteria

1. Steady-state CPU of the timer loop with one registered infinite animation: **< 2% of one core**
(measure; the current implementation is the ~50% baseline per Finding 1).
2. No `timeBeginPeriod` while animations run (verify via `powercfg /energy` or timer-resolution
query on a Win10 1803+ box).
3. Frame delivery: mean interval within ±0.5 ms of target over a 10 s run on an idle machine at
60 Hz; no monotonic slow drift (absolute-schedule check: 600th frame due time within one frame of
`epoch + 600 × period`).
4. Zero per-frame heap allocations in steady state (verify with an allocation-tracking test or
`GC.GetAllocatedBytesForCurrentThread` bracketing).
5. Race test for Finding 4: concurrent register/unregister stress leaves no registration without a
running loop.
6. Existing `HighPrecisionTimerTests` updated/extended accordingly; late-callback tolerance test for
consumers (Finding 7).
7. `HighPrecisionTimerTick` surface unchanged (internal consumers depend on it); all other churn is
internal to the pacer.

## Constraints

- Everything stays `internal`; no public API review implications.
- Follow repo conventions (`LibraryImport`, nullable enabled, existing XML-doc voice).
- Windows-only paths gated with `[SupportedOSPlatform]` / `OperatingSystem.IsWindowsVersionAtLeast`
as currently practiced in the file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
# Work Order: SystemVisualSettings (Implementation)

**Branch:** `Net11/Integration-2` (KlausLoeffelmann/winforms)
**Scope:** Code changes only. The GitHub proposal issue is handled by a separate work order
(`ApiReview-IssueUpdates.md`, Section 3) and run only after this implementation stands.
**Supersedes:** `Application.GetWindowsAccentColor` and the standalone accessibility text-size change
event currently on the branch (see item 6).

---

## Background

Windows delivers visual/accessibility setting changes through four channels — `WM_SETTINGCHANGE`,
`WM_DWMCOLORIZATIONCOLORCHANGED`, `WM_THEMECHANGED`, `WM_SYSCOLORCHANGE` — and today each consumer
normalizes that zoo individually. This work order introduces one typed, read-only snapshot plus one
unified change notification, with a **leak-free consumption path for controls** (virtual cascade, no
static-event subscription — the `SystemEvents.UserPreferenceChanged` leak class must be structurally
impossible for the default path).

Deliberate non-goals, enforced by the type system: **no settable properties.** Perceptual adjustments
(contrast, color filtering, text scale, focus prominence, motion) are user-owned via Windows
accessibility settings; app-side theming is the business of control vendor partners. Renderers derive
output from this snapshot combined with `EffectiveVisualStylesMode`.

---

## 1. New types (`System.Windows.Forms`)

```csharp
public sealed class SystemVisualSettings
{
public Color AccentColor { get; }
public float TextScaleFactor { get; } // 1.0–2.25, Windows a11y text scale
public bool HighContrastEnabled { get; }
public bool ClientAreaAnimationEnabled { get; } // SPI_GETCLIENTAREAANIMATION
public bool KeyboardCuesVisible { get; } // SPI_GETKEYBOARDCUES (system default)
public Size FocusBorderMetrics { get; } // SPI_GETFOCUSBORDERWIDTH/HEIGHT, pixels
}

[Flags]
public enum SystemVisualSettingsCategories
{
None = 0,
AccentColor = 1 << 0,
TextScale = 1 << 1,
HighContrast = 1 << 2,
Animations = 1 << 3,
KeyboardCues = 1 << 4,
FocusMetrics = 1 << 5
}

public class SystemVisualSettingsChangedEventArgs : EventArgs
{
public SystemVisualSettings OldSettings { get; }
public SystemVisualSettings NewSettings { get; }
public SystemVisualSettingsCategories Changed { get; }
}
```

Immutable snapshot semantics — values must not shift under an event handler comparing old vs new.
XML remarks per the design notes: `HighContrastEnabled` cross-references the effective-mode clamp;
`KeyboardCuesVisible` documents the system-default vs per-window (`WM_UPDATEUISTATE`) distinction;
`FocusBorderMetrics` is documented as the baseline input for border/focus prominence in renderers
(scaled for DPI and `TextScaleFactor`), replacing fixed constants; the class remarks state the
no-app-side-overrides principle explicitly.

## 2. `Application` surface

```csharp
public static SystemVisualSettings SystemVisualSettings { get; }
public static event EventHandler<SystemVisualSettingsChangedEventArgs>? SystemVisualSettingsChanged;
```

Event XML remarks must state: (a) raised once per settings transition, normalized across the four
underlying messages; (b) handlers should early-out via `e.Changed`; (c) **audience is app-lifetime
consumers** (theming engines, services); components with shorter lifetime than the application must
unsubscribe; **controls and forms should use the `Control`-level virtual/instance event instead,
which requires no unsubscription** (see item 3). This positioning is the leak fix — make the
leak-free path the documented default.

## 3. `Control`-level consumption (the leak-free path)

```csharp
protected virtual void OnSystemVisualSettingsChanged(SystemVisualSettingsChangedEventArgs e);
public event EventHandler<SystemVisualSettingsChangedEventArgs>? SystemVisualSettingsChanged;
```

- Model the cascade on the existing `OnSystemColorsChanged` pattern in `Control.cs`: virtual
dispatch parent→children, instance event raised from within the virtual. No subscription to any
static event exists anywhere in this path — lifetime coupling is structural.
- Keep the cascade pattern-identical to `OnVisualStylesModeChanged` / `OnParentVisualStylesModeChanged`
(see the VisualStylesMode impact work order). A `HighContrast` category change resolves as an
effective-visual-styles-mode change for affected controls and must route through that machinery's
early-out/dispatch — no duplicate HC handling in this cascade.
- **Remove/replace the existing Form-level replicated text-size event**: it generalizes into this
cascade and disappears as a special case. Migrate in-box usages.
- Staleness rule (document in XML remarks, mirroring `OnSystemColorsChanged` folklore — this time
written down): the cascade only reaches parented controls; a control created but not yet parented
misses transitions and must re-query `Application.SystemVisualSettings` on handle creation /
`OnParentChanged`.

## 4. Message plumbing and normalization

Central internal tracker (e.g. `SystemVisualSettingsTracker`) holding the current snapshot:

- Every **top-level** window already receives the four raw messages; handle them in the existing
top-level `WndProc` paths.
- On receipt: the window asks the tracker to re-query. The **first** arriver computes the diff
against the current snapshot, atomically swaps it (`Interlocked` reference swap), raises the static
`Application` event **once**, and cascades into its own tree. Subsequent top-levels re-query, see
no diff, raise nothing at Application level, but **still cascade into their own trees** using the
already-computed args.
- Threading falls out for free: each tree is notified on the thread owning its top-level — no
marshaling, correct for multi-message-loop applications. Do not centralize onto one thread.
- Coalesce message storms: a single user action can produce several of the four messages; debounce
within a message-pump iteration (re-query once per burst per top-level, not per message).

## 5. In-box consumption cleanup

- Audit in-box `Microsoft.Win32.SystemEvents` subscriptions (`UserPreferenceChanged` et al.) in
controls/renderers; migrate those covered by the new categories to the cascade. Document any that
must remain (categories outside this surface) — do not expand the snapshot to chase them in this
work order.
- `TextBoxBase` Net11 border rendering: consume `FocusBorderMetrics` + `TextScaleFactor` as the
border-prominence input where fixed constants are currently used (coordinate with the animation /
focus-indicator renderer as applicable).
- Animated renderers (`AnimatedControlRenderer` / `AnimationManager`): honor
`ClientAreaAnimationEnabled == false` by rendering final state immediately and suppressing
transitions; react to the `Animations` category change at runtime.

## 6. Supersede the piecemeal APIs

- `Application.GetWindowsAccentColor` → `Application.SystemVisualSettings.AccentColor`. The method
has not shipped stable: **remove it** on this branch (preferred) rather than obsoleting, to avoid
two sources of truth. If removal is blocked by preview-compat policy, `[Obsolete]` with pointer.
- The standalone text-size change event (Application- and/or Form-level) → `Changed.HasFlag(TextScale)`
on the unified event / cascade. Same removal-vs-obsolete decision, same preference.
- Migrate all in-box call sites.

## 7. Tests

- Snapshot immutability and correct SPI mapping per property (mock/native-shim as the repo's test
infra allows).
- Normalization: N top-level windows + one settings transition ⇒ exactly one Application-level raise;
every window's tree cascaded exactly once; delivery on each tree's own thread.
- Flags correctness per category, including multi-category transitions (HC toggle typically changes
colors + HC + metrics in one burst — must coalesce to one event with combined flags).
- Leak test: create/dispose forms subscribing to the **control-level** event in a loop; assert
collectability (`WeakReference`), proving no static rooting. Counter-test documenting that the
static event does root (expected, documented behavior).
- HC toggle end-to-end: cascade triggers effective-mode change path once, no duplicate layout.

## Constraints

- All new public surface XML-documented in the branch's voice; internal tracker fully internal.
- No settable members anywhere on the new types — if implementation pressure suggests one, stop and
flag rather than adding it.
- Windows-only P/Invoke via `LibraryImport`, gated per existing repo practice.
Loading