Performance improvements

[voytas]

No description provided.

[voytas]

/review-claude

[claude]

Claude finished @voytas's task in 2m 47s —— View job

---

Claude Code Review

• Read changed files
• Analyze emulation correctness / timing
• Analyze threading / platform code
• Analyze logic / regressions
• Post final review

---

Nice, focused performance work. No emulation-correctness or T-state concerns — Z80 / ULA paths are not touched. Findings are mostly around the new threading / Windows interop, with one minor robustness question on the bitmap copy.

EmulatorTimer.cs

1. Windows timer resolution can leak if Dispose() runs without Stop() — src/Spectron.Emulation/EmulatorTimer.cs:32-45,138-145

timeBeginPeriod(1) is reference-counted process-wide, so an unmatched Stop() keeps the system at 1 ms resolution until the process exits (extra power draw, especially on laptops). In current callers (Emulator.Shutdown at Emulator.cs:160) Stop() always precedes Dispose(), so this is defensive — but the class no longer owns its own lifecycle invariant. Two cheap options:

• Track a _resolutionRequested flag and release in Dispose() if Stop() wasn't called, or
• Have Dispose() call Stop() if the worker is still alive.

2. Stop() releases timer resolution before the worker has actually stopped — src/Spectron.Emulation/EmulatorTimer.cs:39-45

Platform.ReleaseMinimumTimerResolution();   // <- here
_cancellationTokenSource.Cancel();
_stoppedEvent.Wait();

Between Release and the worker actually exiting, the worker may still execute Thread.Sleep(...) at the now-degraded resolution (~15 ms on Windows). Functionally harmless (we're shutting down) but it can stretch shutdown by a frame or two. Swapping the order — cancel, wait, then release — costs nothing and is more symmetric with Start().

3. _isDisposed check in Pause() is not race-safe — src/Spectron.Emulation/EmulatorTimer.cs:47-70

The early-out at line 49 only narrows the TOCTOU window. A concurrent Dispose() between the check and _pausedEvent.Reset() / _cancellationTokenSource.Token access still throws ObjectDisposedException. Two practical mitigations if pause-during-shutdown is a real scenario:

• Mark _isDisposed volatile (or use Volatile.Read) so the write is promptly visible across threads, and
• Wrap Pause() body in a try { … } catch (ObjectDisposedException) { }.

If pause-during-shutdown isn't a supported flow, a comment to that effect would be enough.

4. New default sleep branch — looks correct — src/Spectron.Emulation/EmulatorTimer.cs:124-126

This branch only fires for timeToWait >= 10 ms, so Math.Max(1, x − 5) is always ≥ 5 ms; the spin branches then absorb the remaining sub-ms drift. Good combo with timeBeginPeriod(1) to keep Sleep(5) actually ~5 ms on Windows.

Platform.cs / Winmm.cs

Look good. Minor: both files are missing a trailing newline (\ No newline at end of file in the diff). Most repos enforce this via .editorconfig; the existing .cs files in Spectron.Emulation all end with a newline, so this is just inconsistency rather than a hard rule.

FrameBufferConverter.cs

5. Buffer.MemoryCopy per row — correct, and a clear win — src/Spectron/Screen/FrameBufferConverter.cs:33-56

Indexing, pointer arithmetic, and byte counts all check out (Color is a 4-byte sequential struct → colCount * 4 for row bytes). Replacing the inner per-pixel *(uint*) loop with one Buffer.MemoryCopy per row should be a noticeable speedup, especially with wide borders.

One latent assumption (not new — old code had it too): both old and new code assume lockedBitmap.RowBytes == colCount * 4, i.e. no destination stride padding. Avalonia's Rgba8888 typically satisfies this, but if you want to harden against a future Avalonia/skia change you could use lockedBitmap.RowBytes for the destination advance instead of rowBytes. Worth a one-line guard or assert if you want to keep this assumption explicit. Not a blocker.

Spectron.Emulation.csproj

<AllowUnsafeBlocks>true</AllowUnsafeBlocks> was added but I don't see new unsafe code in Spectron.Emulation itself — only FrameBufferConverter.cs (which lives in the Spectron project and already had unsafe). If this was added speculatively for the interop, it isn't required (P/Invoke doesn't need AllowUnsafeBlocks). Consider dropping it unless there's an unsafe block planned for this project.

Nits

• Platform.cs line 34: setting Platform.RequestMinimumTimerResolution() only in Start() means restart cycles (Stop → Start) re-request — fine, just noting that the Windows reference count tracks request/release pairs across the process. As long as starts and stops stay 1:1, this is clean.
• _isDisposed is mutated in Dispose() and read in Pause() without volatile — see point 3.

---

Overall LGTM with the caveats above. Items 1–3 are about lifecycle robustness; none of them break the happy path that the test/run flow exercises today, but they're easy fixes and worth tightening before the next perf-sensitive change lands.
• branch performance