From 71e12948f34051997974c962bbd8d0da0c29218c Mon Sep 17 00:00:00 2001 From: Allan Thraen Date: Fri, 7 Aug 2026 15:09:25 +0200 Subject: [PATCH] perf(terminal): coalesce PTY output posts and unblock the keystroke path Typing into a session could lag by minutes with many live sessions. Three compounding causes, all on the shared UI dispatcher (issue #70): 1. TerminalBridge.OnPtyData did one Dispatcher.BeginInvoke per PTY chunk, so chatty background sessions flooded the queue. Introduce OutputCoalescer: chunks arriving before the dispatcher gets a turn collapse into a single post. The scheduler and emitter are injected so the state machine is unit-testable without a dispatcher or WebView2. 2. Keystrokes were starved by that same queue, not just rendering. WebView2 raises WebMessageReceived on the UI thread, so an input event waits behind the backlog, and the echo waits again to render - two queue traversals per visible character. Background sessions now post at DispatcherPriority .Background and only the active pane posts at Normal, via the new TerminalBridge.IsForeground flag driven from UpdateActiveTerminalHighlight. 3. Every keystroke synchronously recomputed AlertCount (an O(N) scan over all sessions) twice and invalidated WPF bindings both times. AlertCleared fires unconditionally from NotifyUserInteracted, so the second raise in the UserInput handler was redundant - removed. The remaining raise goes through RaiseAlertCountIfChanged, which suppresses no-op raises. AlertRaised and AlertCleared switch from blocking Invoke to BeginInvoke; neither caller consumes a result and AlertRaised arrives on a threadpool timer callback. The pre-ready output buffer now drains through the coalescer too, so load-time output and post-ready chunks share one buffer and keep arrival order. MainViewModel's GroupsChanged Invoke is deliberately left blocking: it is low-frequency group CRUD, not on the hot path, and deferring it could reorder sidebar rebuilds. Verified: 213/213 unit tests pass (7 new for the coalescer); app starts clean. The latency improvement itself is NOT yet measured under load - that needs a real multi-session run with DebugTerminalTrace enabled. Co-Authored-By: Claude Opus 5 (1M context) --- src/CodeShellManager/MainWindow.xaml.cs | 7 + .../Terminal/OutputCoalescer.cs | 72 +++++++++ .../Terminal/TerminalBridge.cs | 67 +++++--- .../ViewModels/MainViewModel.cs | 34 +++-- .../OutputCoalescerTests.cs | 144 ++++++++++++++++++ 5 files changed, 291 insertions(+), 33 deletions(-) create mode 100644 src/CodeShellManager/Terminal/OutputCoalescer.cs create mode 100644 tests/CodeShellManager.Tests/OutputCoalescerTests.cs diff --git a/src/CodeShellManager/MainWindow.xaml.cs b/src/CodeShellManager/MainWindow.xaml.cs index 61bf63e..abe661d 100644 --- a/src/CodeShellManager/MainWindow.xaml.cs +++ b/src/CodeShellManager/MainWindow.xaml.cs @@ -1635,6 +1635,13 @@ private void UpdateSidebarActiveState() private void UpdateActiveTerminalHighlight() { string? activeId = _vm.ActiveSession?.Id; + + // Keep each bridge's output dispatcher priority in sync with focus, so a chatty + // background session posts at Background priority and can't sit ahead of the + // active pane's rendering or its keystrokes (issue #70). + foreach (var s in _vm.Sessions) + if (s.Bridge != null) s.Bridge.IsForeground = s.Id == activeId; + foreach (var (id, ui) in _sessionUi) { if (id == activeId) diff --git a/src/CodeShellManager/Terminal/OutputCoalescer.cs b/src/CodeShellManager/Terminal/OutputCoalescer.cs new file mode 100644 index 0000000..3229f59 --- /dev/null +++ b/src/CodeShellManager/Terminal/OutputCoalescer.cs @@ -0,0 +1,72 @@ +using System; +using System.Text; + +namespace CodeShellManager.Terminal; + +/// +/// Collapses many small PTY output chunks into a single scheduled post (issue #70). +/// +/// Without this, every PTY chunk from every session became its own dispatcher work item. +/// Chatty background sessions (Claude spinners, status-line repaints) flooded the shared +/// UI queue, and both the foreground terminal's rendering *and* its keystrokes — which +/// arrive on the same UI thread via WebView2's WebMessageReceived — had to wait behind +/// that backlog. See issue #70 for the full trace. +/// +/// The scheduler and emitter are injected so the state machine is testable with no WPF +/// dispatcher and no WebView2 present. +/// +public sealed class OutputCoalescer +{ + private readonly object _lock = new(); + private readonly StringBuilder _buffer = new(); + private readonly Action _schedule; + private readonly Action _emit; + + // True between scheduling a flush and that flush draining the buffer. While set, + // further appends piggyback on the already-queued flush instead of adding to the queue. + private bool _flushPending; + + /// Marshals the flush onto the UI thread (Dispatcher.BeginInvoke in production). + /// Delivers one coalesced payload (PostWebMessageAsString in production). + public OutputCoalescer(Action schedule, Action emit) + { + _schedule = schedule; + _emit = emit; + } + + /// Buffers a PTY chunk. Safe to call from any thread. Never blocks on the UI thread. + public void Append(string data) + { + if (string.IsNullOrEmpty(data)) return; + + bool scheduleNow = false; + lock (_lock) + { + _buffer.Append(data); + if (!_flushPending) + { + _flushPending = true; + scheduleNow = true; + } + } + + // Outside the lock: the scheduler may run the flush inline on this thread. + if (scheduleNow) _schedule(Flush); + } + + private void Flush() + { + string payload; + lock (_lock) + { + payload = _buffer.ToString(); + _buffer.Clear(); + // Cleared *before* emitting so a chunk arriving during the emit below + // schedules a fresh flush rather than being silently dropped. + _flushPending = false; + } + + // Emit outside the lock — it re-enters WebView2 and must not hold up Append. + if (payload.Length > 0) _emit(payload); + } +} diff --git a/src/CodeShellManager/Terminal/TerminalBridge.cs b/src/CodeShellManager/Terminal/TerminalBridge.cs index 518e8b7..3f13ad1 100644 --- a/src/CodeShellManager/Terminal/TerminalBridge.cs +++ b/src/CodeShellManager/Terminal/TerminalBridge.cs @@ -36,6 +36,17 @@ public sealed class TerminalBridge : IDisposable // Output that arrived before the page finished loading is buffered here private readonly System.Text.StringBuilder _outputBuffer = new(); + // Coalesces PTY chunks into one dispatcher post per tick (issue #70). + private readonly OutputCoalescer _coalescer; + + /// + /// True when this session's pane is the active one. Foreground output posts at + /// Normal priority; background sessions post at Background priority so a chatty + /// off-screen session can't delay the pane the user is actually typing into. + /// Set by MainWindow whenever MainViewModel.ActiveSession changes. + /// + public bool IsForeground { get; set; } + // Diagnostics — gated by AppSettings.DebugTerminalTrace. Zero cost when off. /// AppSettings reference whose DebugTerminalTrace flag gates [DEBUG-tt] logging. public AppSettings? DebugSettings { get; set; } @@ -96,6 +107,30 @@ private void PostBootDoneIfNeeded() public TerminalBridge(WebView2 webView) { _webView = webView; + _coalescer = new OutputCoalescer(ScheduleFlush, PostOutput); + } + + // Queues one coalesced flush. Background sessions yield to the foreground pane so + // their output can't sit ahead of the active session's rendering or its keystrokes. + private void ScheduleFlush(Action flush) + { + var dispatcher = WpfApplication.Current?.Dispatcher; + if (dispatcher == null) return; + dispatcher.BeginInvoke( + IsForeground + ? System.Windows.Threading.DispatcherPriority.Normal + : System.Windows.Threading.DispatcherPriority.Background, + flush); + } + + // Runs on the UI thread. One WebView2 post per coalesced batch. + private void PostOutput(string data) + { + string json = JsonSerializer.Serialize(new { type = "output", data }); + try { _webView.CoreWebView2?.PostWebMessageAsString(json); } + catch { } + if (DebugSettings?.DebugTerminalTrace == true) + Trace($"OUTPUT flush len={data.Length}"); } /// @@ -200,16 +235,10 @@ void NavCompleted(object? s, CoreWebView2NavigationCompletedEventArgs e) buffered = _outputBuffer.ToString(); _outputBuffer.Clear(); } - if (buffered.Length > 0) - { - string json = System.Text.Json.JsonSerializer.Serialize( - new { type = "output", data = buffered }); - WpfApplication.Current?.Dispatcher.BeginInvoke(() => - { - try { _webView.CoreWebView2?.PostWebMessageAsString(json); } - catch { } - }); - } + // Route through the coalescer rather than posting directly: chunks that land + // between `_ready = true` above and this drain go to the coalescer, so sharing + // one buffer keeps load-time output in arrival order. + if (buffered.Length > 0) _coalescer.Append(buffered); navDone.TrySetResult(true); } @@ -253,20 +282,10 @@ private void OnPtyData(string rawData) return; } - string json = JsonSerializer.Serialize(new { type = "output", data = rawData }); - long enqueueAt = DebugSettings?.DebugTerminalTrace == true ? Environment.TickCount64 : 0; - int len = rawData.Length; - WpfApplication.Current?.Dispatcher.BeginInvoke(() => - { - // Capture latency before any work so Trace's file I/O doesn't inflate the - // measurement, then post the WebView2 message before tracing so the trace - // overhead doesn't delay terminal rendering. - long latencyMs = enqueueAt != 0 ? Environment.TickCount64 - enqueueAt : 0; - try { _webView.CoreWebView2?.PostWebMessageAsString(json); } - catch { } - if (enqueueAt != 0) - Trace($"OUTPUT post dispatcher-latency={latencyMs}ms len={len}"); - }); + // Buffer instead of posting per chunk. Many chunks arriving before the dispatcher + // gets a turn collapse into a single post, so background sessions can no longer + // flood the shared UI queue and starve the foreground pane (issue #70). + _coalescer.Append(rawData); } private void OnAcceleratorKeyPressed(object? sender, WpfKeyEventArgs e) diff --git a/src/CodeShellManager/ViewModels/MainViewModel.cs b/src/CodeShellManager/ViewModels/MainViewModel.cs index 3802d43..2f9a09f 100644 --- a/src/CodeShellManager/ViewModels/MainViewModel.cs +++ b/src/CodeShellManager/ViewModels/MainViewModel.cs @@ -60,6 +60,19 @@ public partial class MainViewModel : ObservableObject public int AlertCount => Sessions.Count(s => s.NeedsAttention); + // AlertCount is an O(N) scan and every raise invalidates WPF bindings. It's touched on + // the keystroke path (via AlertCleared), where the value is almost always unchanged, so + // suppress no-op raises rather than re-running binding invalidation per character (#70). + private int _lastRaisedAlertCount = -1; + + private void RaiseAlertCountIfChanged() + { + int count = AlertCount; + if (count == _lastRaisedAlertCount) return; + _lastRaisedAlertCount = count; + OnPropertyChanged(nameof(AlertCount)); + } + public event Action? SessionClosed; public event Action? GroupsChanged; public event Action? SelectionChanged; @@ -267,31 +280,34 @@ public void RegisterSession(SessionViewModel vm) if (vm.Bridge != null) { - vm.Bridge.UserInput += () => - { - vm.AlertDetector?.NotifyUserInteracted(); - App.Current.Dispatcher.Invoke(() => OnPropertyChanged(nameof(AlertCount))); - }; + // Runs on the UI thread for every keystroke (WebView2 raises WebMessageReceived + // there), so it must stay cheap. NotifyUserInteracted already fires AlertCleared + // unconditionally, whose handler below raises AlertCount — so this deliberately + // does not raise it a second time (issue #70). + vm.Bridge.UserInput += () => vm.AlertDetector?.NotifyUserInteracted(); } if (vm.AlertDetector != null) { + // BeginInvoke, not Invoke: AlertRaised arrives on a System.Threading.Timer + // callback, and blocking a threadpool thread on a busy UI thread serves no + // purpose — no caller consumes a result. vm.AlertDetector.AlertRaised += alert => { - App.Current.Dispatcher.Invoke(() => + App.Current.Dispatcher.BeginInvoke(() => { vm.RaiseAlert(alert.Message, alert.Type); - OnPropertyChanged(nameof(AlertCount)); + RaiseAlertCountIfChanged(); if (Settings.ShowToastNotifications) ToastHelper.Show(vm.DisplayName, alert.Message, Settings.ShowNotificationSound); }); }; vm.AlertDetector.AlertCleared += _ => { - App.Current.Dispatcher.Invoke(() => + App.Current.Dispatcher.BeginInvoke(() => { vm.ClearAlert(); - OnPropertyChanged(nameof(AlertCount)); + RaiseAlertCountIfChanged(); }); }; } diff --git a/tests/CodeShellManager.Tests/OutputCoalescerTests.cs b/tests/CodeShellManager.Tests/OutputCoalescerTests.cs new file mode 100644 index 0000000..cd35469 --- /dev/null +++ b/tests/CodeShellManager.Tests/OutputCoalescerTests.cs @@ -0,0 +1,144 @@ +using CodeShellManager.Terminal; +using Xunit; + +namespace CodeShellManager.Tests; + +/// +/// Covers the coalescing state machine that collapses N PTY output chunks into a single +/// dispatcher post (issue #70). The scheduler and emitter are injected so these run +/// headlessly with no WPF dispatcher and no WebView2. +/// +public class OutputCoalescerTests +{ + /// Captures scheduled flushes so the test controls when they run. + private sealed class ManualScheduler + { + private readonly List _queued = new(); + public int ScheduleCount { get; private set; } + public List Emitted { get; } = new(); + + public void Schedule(Action flush) + { + ScheduleCount++; + _queued.Add(flush); + } + + public void Emit(string payload) => Emitted.Add(payload); + + /// Runs every flush queued so far (mimics the dispatcher draining). + public void Drain() + { + var batch = _queued.ToList(); + _queued.Clear(); + foreach (var f in batch) f(); + } + } + + private static (OutputCoalescer, ManualScheduler) Build() + { + var s = new ManualScheduler(); + return (new OutputCoalescer(s.Schedule, s.Emit), s); + } + + [Fact] + public void SingleAppend_SchedulesOneFlush_AndEmitsThatData() + { + var (c, s) = Build(); + + c.Append("hello"); + + Assert.Equal(1, s.ScheduleCount); + Assert.Empty(s.Emitted); // nothing emitted until the dispatcher runs the flush + s.Drain(); + Assert.Equal(new[] { "hello" }, s.Emitted); + } + + [Fact] + public void ManyAppendsBeforeFlush_ScheduleOnlyOneFlush() + { + var (c, s) = Build(); + + for (int i = 0; i < 50; i++) c.Append("x"); + + Assert.Equal(1, s.ScheduleCount); + } + + [Fact] + public void ManyAppendsBeforeFlush_EmitOnceWithConcatenatedDataInOrder() + { + var (c, s) = Build(); + + c.Append("a"); + c.Append("b"); + c.Append("c"); + s.Drain(); + + Assert.Equal(new[] { "abc" }, s.Emitted); + } + + [Fact] + public void AppendAfterFlush_SchedulesANewFlush() + { + var (c, s) = Build(); + + c.Append("first"); + s.Drain(); + c.Append("second"); + s.Drain(); + + Assert.Equal(2, s.ScheduleCount); + Assert.Equal(new[] { "first", "second" }, s.Emitted); + } + + [Fact] + public void FlushWithNothingBuffered_DoesNotEmit() + { + var (c, s) = Build(); + + c.Append("only"); + s.Drain(); // drains "only" + s.Drain(); // no-op: queue is empty, nothing new buffered + + Assert.Equal(new[] { "only" }, s.Emitted); + } + + [Fact] + public void DataAppendedDuringFlush_IsNotLost() + { + var s = new ManualScheduler(); + OutputCoalescer? c = null; + bool reentered = false; + // Emit re-enters Append, simulating a PTY chunk landing while the flush runs. + c = new OutputCoalescer(s.Schedule, payload => + { + s.Emit(payload); + if (!reentered) + { + reentered = true; + c!.Append("late"); + } + }); + + c.Append("early"); + s.Drain(); // emits "early", during which "late" is appended + s.Drain(); // must flush "late" + + Assert.Equal(new[] { "early", "late" }, s.Emitted); + } + + [Fact] + public void ConcurrentAppends_LoseNoData() + { + var (c, s) = Build(); + const int threads = 8, perThread = 500; + + Parallel.For(0, threads, _ => + { + for (int i = 0; i < perThread; i++) c.Append("z"); + }); + // Drain repeatedly: appends racing with a flush may leave a second flush pending. + for (int i = 0; i < 5; i++) s.Drain(); + + Assert.Equal(threads * perThread, string.Concat(s.Emitted).Length); + } +}