From 4986915dae2d65e86804f2b582e13e83857c6bc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Thu, 29 Jan 2026 04:56:11 +0100 Subject: [PATCH] CmdPal: Batch ViewModel property change notifications (#44545) ## Summary of the Pull Request This PR introduces batching for property change notifications emitted by Command Palette view models. It also adds a secondary notification path that is guaranteed to execute on a background thread. - Introduces **`BatchUpdateManager`**, which batches `INotifyPropertyChanged` events from view models and replays them in a coordinated way. - Slightly reduces UI thread contention and allows related UI updates to be applied together, reducing visual "tearing" in list items (when title, subtitle and icon are updated separately with slight delay). Batching won't mitigate all occurences, but its good enough and works auto-magically. - Adds a complementary background notification event that: - Is guaranteed to run on a background thread. - Fires before UI-thread notifications. - Allows consumers to attach handlers without blocking COM out-of-proc objects. - Updates `TopLevelViewModel` to subscribe to the background property change event instead of the UI-thread one. - This avoids unintentionally shifting work onto the UI thread and re-triggering expensive operations there. - Previously, because `TopLevelViewModel` wraps another view model and our view models raise `INPC` on the UI thread by default, its handler was executing on the UI thread and re-raising the event as `IListItem.PropertyChanged`, causing `FetchProperty` methods to run on the UI thread again. - Ideally, `TopLevelViewModel` should be reworked to address this more cleanly, but that turned out to be a non-trivial change. This PR applies a targeted mitigation in the meantime. ## PR Checklist - [ ] Closes: #xxx - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed --- .../BatchUpdateManager.cs | 136 ++++++++++ .../ExtensionObjectViewModel.cs | 249 +++++++++++++++--- .../IBackgroundPropertyChangedNotification.cs | 19 ++ .../PageViewModel.cs | 4 +- .../TopLevelViewModel.cs | 2 +- 5 files changed, 365 insertions(+), 45 deletions(-) create mode 100644 src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/BatchUpdateManager.cs create mode 100644 src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/IBackgroundPropertyChangedNotification.cs 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();