diff --git a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/BatchUpdateManager.cs b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/BatchUpdateManager.cs new file mode 100644 index 0000000000..52d6091bd8 --- /dev/null +++ b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/BatchUpdateManager.cs @@ -0,0 +1,136 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Concurrent; +using Microsoft.CmdPal.Core.Common; +using Microsoft.CmdPal.Core.Common.Helpers; + +namespace Microsoft.CmdPal.Core.ViewModels; + +internal static class BatchUpdateManager +{ + private const int ExpectedBatchSize = 32; + + // 30 ms chosen empirically to balance responsiveness and batching: + // - Keeps perceived latency low (< ~50 ms) for user-visible updates. + // - Still allows multiple COM/background events to be coalesced into a single batch. + private static readonly TimeSpan BatchDelay = TimeSpan.FromMilliseconds(30); + private static readonly ConcurrentQueue DirtyQueue = []; + private static readonly Timer Timer = new(static _ => Flush(), null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + + private static InterlockedBoolean _isFlushScheduled; + + /// + /// Enqueue a target for batched processing. Safe to call from any thread (including COM callbacks). + /// + public static void Queue(IBatchUpdateTarget target) + { + if (!target.TryMarkBatchQueued()) + { + return; // already queued in current batch window + } + + DirtyQueue.Enqueue(target); + TryScheduleFlush(); + } + + private static void TryScheduleFlush() + { + if (!_isFlushScheduled.Set()) + { + return; + } + + if (DirtyQueue.IsEmpty) + { + _isFlushScheduled.Clear(); + + if (DirtyQueue.IsEmpty) + { + return; + } + + if (!_isFlushScheduled.Set()) + { + return; + } + } + + try + { + Timer.Change(BatchDelay, Timeout.InfiniteTimeSpan); + } + catch (Exception ex) + { + _isFlushScheduled.Clear(); + CoreLogger.LogError("Failed to arm batch timer.", ex); + } + } + + private static void Flush() + { + try + { + var drained = new List(ExpectedBatchSize); + while (DirtyQueue.TryDequeue(out var item)) + { + drained.Add(item); + } + + if (drained.Count == 0) + { + return; + } + + // LOAD BEARING: + // ApplyPendingUpdates must run on a background thread. + // The VM itself is responsible for marshaling UI notifications to its _uiScheduler. + ApplyBatch(drained); + } + catch (Exception ex) + { + // Don't kill the timer thread. + CoreLogger.LogError("Batch flush failed.", ex); + } + finally + { + _isFlushScheduled.Clear(); + TryScheduleFlush(); + } + } + + private static void ApplyBatch(List items) + { + // Runs on the Timer callback thread (ThreadPool). That's fine: background work only. + foreach (var item in items) + { + // Allow re-queueing immediately if more COM events arrive during apply. + item.ClearBatchQueued(); + + try + { + item.ApplyPendingUpdates(); + } + catch (Exception ex) + { + CoreLogger.LogError("Failed to apply pending updates for a batched target.", ex); + } + } + } +} + +internal interface IBatchUpdateTarget +{ + /// UI scheduler (used by targets internally for UI marshaling). Kept here for diagnostics / consistency. + TaskScheduler UIScheduler { get; } + + /// Apply any coalesced updates. Must be safe to call on a background thread. + void ApplyPendingUpdates(); + + /// De-dupe gate: returns true only for the first enqueue until cleared. + bool TryMarkBatchQueued(); + + /// Clear the de-dupe gate so the item can be queued again. + void ClearBatchQueued(); +} diff --git a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ExtensionObjectViewModel.cs b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ExtensionObjectViewModel.cs index e7bed46db6..53af586431 100644 --- a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ExtensionObjectViewModel.cs +++ b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ExtensionObjectViewModel.cs @@ -2,36 +2,99 @@ // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Buffers; +using System.Collections.Concurrent; +using System.ComponentModel; +using System.Runtime.CompilerServices; using CommunityToolkit.Mvvm.ComponentModel; using Microsoft.CmdPal.Core.Common; +using Microsoft.CmdPal.Core.Common.Helpers; namespace Microsoft.CmdPal.Core.ViewModels; -public abstract partial class ExtensionObjectViewModel : ObservableObject +public abstract partial class ExtensionObjectViewModel : ObservableObject, IBatchUpdateTarget, IBackgroundPropertyChangedNotification { - public WeakReference PageContext { get; set; } + private const int InitialPropertyBatchingBufferSize = 16; - internal ExtensionObjectViewModel(IPageContext? context) - { - var realContext = context ?? (this is IPageContext c ? c : throw new ArgumentException("You need to pass in an IErrorContext")); - PageContext = new(realContext); - } + // Raised on the background thread before UI notifications. It's raised on the background thread to prevent + // blocking the COM proxy. + public event PropertyChangedEventHandler? PropertyChangedBackground; - internal ExtensionObjectViewModel(WeakReference context) - { - PageContext = context; - } + private readonly ConcurrentQueue _pendingProps = []; - public async virtual Task InitializePropertiesAsync() + private readonly TaskScheduler _uiScheduler; + + private InterlockedBoolean _batchQueued; + + public WeakReference PageContext { get; private set; } = null!; + + TaskScheduler IBatchUpdateTarget.UIScheduler => _uiScheduler; + + void IBatchUpdateTarget.ApplyPendingUpdates() => ApplyPendingUpdates(); + + bool IBatchUpdateTarget.TryMarkBatchQueued() => _batchQueued.Set(); + + void IBatchUpdateTarget.ClearBatchQueued() => _batchQueued.Clear(); + + private protected ExtensionObjectViewModel(TaskScheduler scheduler) { - var t = new Task(() => + if (this is not IPageContext) { - SafeInitializePropertiesSynchronous(); - }); - t.Start(); - await t; + throw new InvalidOperationException($"Constructor overload without IPageContext can only be used when the derived class implements IPageContext. Type: {GetType().FullName}"); + } + + _uiScheduler = scheduler ?? throw new ArgumentNullException(nameof(scheduler)); + + // Defer PageContext assignment - derived constructor MUST call InitializePageContext() + // or we set it lazily on first access } + private protected ExtensionObjectViewModel(IPageContext context) + { + ArgumentNullException.ThrowIfNull(context); + + PageContext = new WeakReference(context); + _uiScheduler = context.Scheduler; + + LogIfDefaultScheduler(); + } + + private protected ExtensionObjectViewModel(WeakReference contextRef) + { + ArgumentNullException.ThrowIfNull(contextRef); + + if (!contextRef.TryGetTarget(out var context)) + { + throw new ArgumentException("IPageContext must be alive when creating view models.", nameof(contextRef)); + } + + PageContext = contextRef; + _uiScheduler = context.Scheduler; + + LogIfDefaultScheduler(); + } + + protected void InitializeSelfAsPageContext() + { + if (this is not IPageContext self) + { + throw new InvalidOperationException("This method can only be called when the class implements IPageContext."); + } + + PageContext = new WeakReference(self); + } + + private void LogIfDefaultScheduler() + { + if (_uiScheduler == TaskScheduler.Default) + { + CoreLogger.LogDebug($"ExtensionObjectViewModel created with TaskScheduler.Default. Type: {GetType().FullName}"); + } + } + + public virtual Task InitializePropertiesAsync() + => Task.Run(SafeInitializePropertiesSynchronous); + public void SafeInitializePropertiesSynchronous() { try @@ -46,49 +109,151 @@ public abstract partial class ExtensionObjectViewModel : ObservableObject public abstract void InitializeProperties(); - protected void UpdateProperty(string propertyName) - { - DoOnUiThread(() => OnPropertyChanged(propertyName)); - } + protected void UpdateProperty(string propertyName) => MarkPropertyDirty(propertyName); protected void UpdateProperty(string propertyName1, string propertyName2) { - DoOnUiThread(() => - { - OnPropertyChanged(propertyName1); - OnPropertyChanged(propertyName2); - }); - } - - protected void UpdateProperty(string propertyName1, string propertyName2, string propertyName3) - { - DoOnUiThread(() => - { - OnPropertyChanged(propertyName1); - OnPropertyChanged(propertyName2); - OnPropertyChanged(propertyName3); - }); + MarkPropertyDirty(propertyName1); + MarkPropertyDirty(propertyName2); } protected void UpdateProperty(params string[] propertyNames) { - DoOnUiThread(() => + foreach (var p in propertyNames) { - foreach (var propertyName in propertyNames) - { - OnPropertyChanged(propertyName); - } - }); + MarkPropertyDirty(p); + } } + internal void MarkPropertyDirty(string? propertyName) + { + if (string.IsNullOrEmpty(propertyName)) + { + return; + } + + // We should re-consider if this worth deduping + _pendingProps.Enqueue(propertyName); + BatchUpdateManager.Queue(this); + } + + public void ApplyPendingUpdates() + { + ((IBatchUpdateTarget)this).ClearBatchQueued(); + + var buffer = ArrayPool.Shared.Rent(InitialPropertyBatchingBufferSize); + var count = 0; + var transferred = false; + + try + { + while (_pendingProps.TryDequeue(out var name)) + { + if (count == buffer.Length) + { + var bigger = ArrayPool.Shared.Rent(buffer.Length * 2); + Array.Copy(buffer, bigger, buffer.Length); + ArrayPool.Shared.Return(buffer, clearArray: true); + buffer = bigger; + } + + buffer[count++] = name; + } + + if (count == 0) + { + return; + } + + // 1) Background subscribers (must be raised before UI notifications). + var propertyChangedEventHandler = PropertyChangedBackground; + if (propertyChangedEventHandler is not null) + { + RaiseBackground(propertyChangedEventHandler, this, buffer, count); + } + + // 2) UI-facing PropertyChanged: ALWAYS marshal to UI scheduler. + // Hand-off pooled buffer to UI task (UI task returns it). + // + // It would be lovely to do nothing if no one is actually listening on PropertyChanged, + // but ObservableObject doesn't expose that information. + _ = Task.Factory.StartNew( + static state => + { + var p = (UiBatch)state!; + try + { + p.Owner.RaiseUi(p.Names, p.Count); + } + catch (Exception ex) + { + CoreLogger.LogError("Failed to raise property change notifications on UI thread.", ex); + } + finally + { + ArrayPool.Shared.Return(p.Names, clearArray: true); + } + }, + new UiBatch(this, buffer, count), + CancellationToken.None, + TaskCreationOptions.DenyChildAttach, + _uiScheduler); + + transferred = true; + } + catch (Exception ex) + { + CoreLogger.LogError("Failed to apply pending property updates.", ex); + } + finally + { + if (!transferred) + { + ArrayPool.Shared.Return(buffer, clearArray: true); + } + } + } + + private void RaiseUi(string[] names, int count) + { + for (var i = 0; i < count; i++) + { + OnPropertyChanged(Args(names[i])); + } + } + + private static void RaiseBackground(PropertyChangedEventHandler handlers, object sender, string[] names, int count) + { + try + { + for (var i = 0; i < count; i++) + { + handlers(sender, Args(names[i])); + } + } + catch (Exception ex) + { + CoreLogger.LogError("Failed to raise PropertyChangedBackground notifications.", ex); + } + } + + private sealed record UiBatch(ExtensionObjectViewModel Owner, string[] Names, int Count); + protected void ShowException(Exception ex, string? extensionHint = null) { if (PageContext.TryGetTarget(out var pageContext)) { pageContext.ShowException(ex, extensionHint); } + else + { + CoreLogger.LogError("Failed to show exception because PageContext is no longer available.", ex); + } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static PropertyChangedEventArgs Args(string name) => new(name); + protected void DoOnUiThread(Action action) { if (PageContext.TryGetTarget(out var pageContext)) diff --git a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/IBackgroundPropertyChangedNotification.cs b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/IBackgroundPropertyChangedNotification.cs new file mode 100644 index 0000000000..4db157f46d --- /dev/null +++ b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/IBackgroundPropertyChangedNotification.cs @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.ComponentModel; + +namespace Microsoft.CmdPal.Core.ViewModels; + +/// +/// Provides a notification mechanism for property changes that fires +/// synchronously on the calling thread. +/// +public interface IBackgroundPropertyChangedNotification +{ + /// + /// Occurs when the value of a property changes. + /// + event PropertyChangedEventHandler? PropertyChangedBackground; +} diff --git a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/PageViewModel.cs b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/PageViewModel.cs index 2a82f80a02..3b08b9266b 100644 --- a/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/PageViewModel.cs +++ b/src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/PageViewModel.cs @@ -77,11 +77,11 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext public IconInfoViewModel Icon { get; protected set; } public PageViewModel(IPage? model, TaskScheduler scheduler, AppExtensionHost extensionHost) - : base((IPageContext?)null) + : base(scheduler) { + InitializeSelfAsPageContext(); _pageModel = new(model); Scheduler = scheduler; - PageContext = new(this); ExtensionHost = extensionHost; Icon = new(null); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs index 6d7f830658..cc863fe362 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelViewModel.cs @@ -199,7 +199,7 @@ public sealed partial class TopLevelViewModel : ObservableObject, IListItem, IEx _fallbackId = fallback.Id; } - item.PropertyChanged += Item_PropertyChanged; + item.PropertyChangedBackground += Item_PropertyChanged; // UpdateAlias(); // UpdateHotkey();