Throttle scrollbar updates in TermControl to ~one per 8ms

.github/actions/spell-check/dictionary/apis.txt

@@ -7,6 +7,7 @@ EXPCMDFLAGS
 EXPCMDSTATE
 fullkbd
 href
+IAsync
 IBox
 IBind
 IClass

src/cascadia/TerminalControl/TermControl.cpp

@@ -27,6 +27,10 @@ using namespace winrt::Windows::System;
 using namespace winrt::Microsoft::Terminal::Settings;
 using namespace winrt::Windows::ApplicationModel::DataTransfer;
 
+// The minimum delay between updates to the scroll bar's values.
+// The updates are throttled to limit power usage.
+constexpr const auto ScrollBarUpdateInterval = std::chrono::milliseconds(8);
+
 namespace winrt::Microsoft::Terminal::TerminalControl::implementation
 {
     // Helper static function to ensure that all ambiguous-width glyphs are reported as narrow.
@@ -58,7 +62,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         _initializedTerminal{ false },
         _settings{ settings },
         _closing{ false },
-        _isTerminalInitiatedScroll{ false },
+        _isInternalScrollBarUpdate{ false },
         _autoScrollVelocity{ 0 },
         _autoScrollingPointerPoint{ std::nullopt },
         _autoScrollTimer{},
@@ -120,6 +124,32 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             }
         });
 
+        _updateScrollBar = std::make_shared<ThrottledFunc<ScrollBarUpdate>>(
+            [weakThis = get_weak()](const auto& update) {
+                if (auto control{ weakThis.get() })
+                {
+                    control->Dispatcher()
+                        .RunAsync(CoreDispatcherPriority::Normal, [=]() {
+                            if (auto control2{ weakThis.get() })
+                            {
+                                control2->_isInternalScrollBarUpdate = true;
+
+                                auto scrollBar = control2->ScrollBar();
+                                if (update.newValue.has_value())
+                                {
+                                    scrollBar.Value(update.newValue.value());
+                                }
+                                scrollBar.Maximum(update.newMaximum);
+                                scrollBar.Minimum(update.newMinimum);
+                                scrollBar.ViewportSize(update.newViewportSize);
+
+                                control2->_isInternalScrollBarUpdate = false;
+                            }
+                        });
+                }
+            },
+            ScrollBarUpdateInterval);
+
         static constexpr auto AutoScrollUpdateInterval = std::chrono::microseconds(static_cast<int>(1.0 / 30.0 * 1000000));
         _autoScrollTimer.Interval(AutoScrollUpdateInterval);
         _autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll });
@@ -214,7 +244,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         {
             if (_closing)
             {
-                return;
+                co_return;
             }
 
             // Update our control settings
@@ -1427,8 +1457,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
     void TermControl::_ScrollbarChangeHandler(Windows::Foundation::IInspectable const& /*sender*/,
                                               Controls::Primitives::RangeBaseValueChangedEventArgs const& args)
     {
-        if (_isTerminalInitiatedScroll || _closing)
+        if (_isInternalScrollBarUpdate || _closing)
         {
+            // The update comes from ourselves, more specifically from the
+            // terminal. So we don't have to update the terminal because it
+            // already knows.
             return;
         }
 
@@ -1438,9 +1471,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         //      itself - it was initiated by the mouse wheel, or the scrollbar.
         _terminal->UserScrollViewport(newValue);
 
-        // We've just told the terminal to update its viewport to reflect the
-        // new scroll value so the scroll bar matches the viewport now.
-        _willUpdateScrollBarToMatchViewport.store(false);
+        // User input takes priority over terminal events so cancel
+        // any pending scroll bar update if the user scrolls.
+        _updateScrollBar->ModifyPending([](auto& update) {
+            update.newValue.reset();
+        });
     }
 
     // Method Description:
@@ -1939,35 +1974,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         _titleChangedHandlers(winrt::hstring{ wstr });
     }
 
-    // Method Description:
-    // - Update the position and size of the scrollbar to match the given
-    //      viewport top, viewport height, and buffer size.
-    //   The change will be actually handled in _ScrollbarChangeHandler.
-    //   This should be done on the UI thread. Make sure the caller is calling
-    //      us in a RunAsync block.
-    // Arguments:
-    // - viewTop: the top of the visible viewport, in rows. 0 indicates the top
-    //      of the buffer.
-    // - viewHeight: the height of the viewport in rows.
-    // - bufferSize: the length of the buffer, in rows
-    void TermControl::_ScrollbarUpdater(Controls::Primitives::ScrollBar scrollBar,
-                                        const int viewTop,
-                                        const int viewHeight,
-                                        const int bufferSize)
-    {
-        // The terminal is already in the scroll position it wants, so no need
-        // to tell it to scroll.
-        _isTerminalInitiatedScroll = true;
-
-        const auto hiddenContent = bufferSize - viewHeight;
-        scrollBar.Maximum(hiddenContent);
-        scrollBar.Minimum(0);
-        scrollBar.ViewportSize(viewHeight);
-        scrollBar.Value(viewTop);
-
-        _isTerminalInitiatedScroll = false;
-    }
-
     // Method Description:
     // - Update the position and size of the scrollbar to match the given
     //      viewport top, viewport height, and buffer size.
@@ -1978,9 +1984,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
     //      of the buffer.
     // - viewHeight: the height of the viewport in rows.
     // - bufferSize: the length of the buffer, in rows
-    winrt::fire_and_forget TermControl::_TerminalScrollPositionChanged(const int viewTop,
-                                                                       const int viewHeight,
-                                                                       const int bufferSize)
+    void TermControl::_TerminalScrollPositionChanged(const int viewTop,
+                                                     const int viewHeight,
+                                                     const int bufferSize)
     {
         // Since this callback fires from non-UI thread, we might be already
         // closed/closing.
@@ -1991,21 +1997,14 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
 
         _scrollPositionChangedHandlers(viewTop, viewHeight, bufferSize);
 
-        auto weakThis{ get_weak() };
-
-        co_await winrt::resume_foreground(Dispatcher());
+        ScrollBarUpdate update;
+        const auto hiddenContent = bufferSize - viewHeight;
+        update.newMaximum = hiddenContent;
+        update.newMinimum = 0;
+        update.newViewportSize = viewHeight;
+        update.newValue = viewTop;
 
-        // Even if we weren't closed/closing few lines above, we might be
-        // while waiting for this block of code to be dispatched.
-        // If 'weakThis' is locked, then we can safely work with 'this'
-        if (auto control{ weakThis.get() })
-        {
-            if (!_closing.load())
-            {
-                // Update our scrollbar
-                _ScrollbarUpdater(ScrollBar(), viewTop, viewHeight, bufferSize);
-            }
-        }
+        _updateScrollBar->Run(update);
     }
 
     // Method Description:

src/cascadia/TerminalControl/TermControl.h

@@ -15,6 +15,7 @@
 #include "../buffer/out/search.h"
 #include "cppwinrt_utils.h"
 #include "SearchBoxControl.h"
+#include "ThrottledFunc.h"
 
 namespace winrt::Microsoft::Terminal::TerminalControl::implementation
 {
@@ -135,8 +136,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         FontInfoDesired _desiredFont;
         FontInfo _actualFont;
 
-        bool _isTerminalInitiatedScroll;
-        std::atomic<bool> _willUpdateScrollBarToMatchViewport;
+        struct ScrollBarUpdate
+        {
+            std::optional<double> newValue;
+            double newMaximum;
+            double newMinimum;
+            double newViewportSize;
+        };
+        std::shared_ptr<ThrottledFunc<ScrollBarUpdate>> _updateScrollBar;
+        bool _isInternalScrollBarUpdate;
 
         int _rowsToScroll;
 
@@ -201,7 +209,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _DoResizeUnderLock(const double newWidth, const double newHeight);
         void _RefreshSizeUnderLock();
         void _TerminalTitleChanged(const std::wstring_view& wstr);
-        winrt::fire_and_forget _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize);
+        void _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize);
         winrt::fire_and_forget _TerminalCursorPositionChanged();
 
         void _MouseScrollHandler(const double mouseDelta, const Windows::Foundation::Point point, const bool isLeftButtonPressed);
@@ -216,7 +224,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _TryStopAutoScroll(const uint32_t pointerId);
         void _UpdateAutoScroll(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e);
 
-        void _ScrollbarUpdater(Windows::UI::Xaml::Controls::Primitives::ScrollBar scrollbar, const int viewTop, const int viewHeight, const int bufferSize);
         static Windows::UI::Xaml::Thickness _ParseThicknessFromPadding(const hstring padding);
 
         void _KeyHandler(Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e, const bool keyDown);

src/cascadia/TerminalControl/TerminalControl.vcxproj

@@ -43,6 +43,8 @@
     <ClInclude Include="TermControlAutomationPeer.h">
       <DependentUpon>TermControlAutomationPeer.idl</DependentUpon>
     </ClInclude>
+    <ClInclude Include="ThreadSafeOptional.h" />
+    <ClInclude Include="ThrottledFunc.h" />
     <ClInclude Include="TSFInputControl.h">
       <DependentUpon>TSFInputControl.xaml</DependentUpon>
     </ClInclude>

src/cascadia/TerminalControl/TerminalControl.vcxproj.filters

@@ -26,6 +26,8 @@
     <ClInclude Include="TermControlAutomationPeer.h" />
     <ClInclude Include="XamlUiaTextRange.h" />
     <ClInclude Include="TermControlUiaProvider.hpp" />
+    <ClInclude Include="ThreadSafeOptional.h" />
+    <ClInclude Include="ThrottledFunc.h" />
     <ClInclude Include="UiaTextRange.hpp" />
   </ItemGroup>
   <ItemGroup>

src/cascadia/TerminalControl/ThreadSafeOptional.h

@@ -0,0 +1,54 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#pragma once
+#include "pch.h"
+
+template<typename T>
+class ThreadSafeOptional
+{
+public:
+    template<class... Args>
+    bool Emplace(Args&&... args)
+    {
+        std::lock_guard guard{ _lock };
+
+        bool hadValue = _inner.has_value();
+        _inner.emplace(std::forward<Args>(args)...);
+        return !hadValue;
+    }
+
+    std::optional<T> Take()
+    {
+        std::lock_guard guard{ _lock };
+
+        std::optional<T> value;
+        _inner.swap(value);
+
+        return value;
+    }
+
+    // Method Description:
+    // - If the optional has a value, then call the specified function with a
+    //   reference to the value.
+    // - This method is always thread-safe. It can be called multiple times on
+    //   different threads.
+    // Arguments:
+    // - f: the function to call with a reference to the value
+    // Return Value:
+    // - <none>
+    template<typename F>
+    void ModifyValue(F f)
+    {
+        std::lock_guard guard{ _lock };
+
+        if (_inner.has_value())
+        {
+            f(_inner.value());
+        }
+    }
+
+private:
+    std::mutex _lock;
+    std::optional<T> _inner;
+};

src/cascadia/TerminalControl/ThrottledFunc.h

@@ -0,0 +1,97 @@
+/*++
+Copyright (c) Microsoft Corporation
+Licensed under the MIT license.
+
+Module Name:
+- ThrottledFunc.h
+
+Abstract:
+- This module defines a class to throttle function calls.
+- You create an instance of a `ThrottledFunc` with a function and the delay
+  between two function calls.
+- The function takes an argument of type `T`, the template argument of
+  `ThrottledFunc`.
+- Use the `Run` method to wait and then call the function.
+--*/
+
+#pragma once
+#include "pch.h"
+
+#include "ThreadSafeOptional.h"
+
+template<typename T>
+class ThrottledFunc : public std::enable_shared_from_this<ThrottledFunc<T>>
+{
+public:
+    using Func = std::function<void(T arg)>;
+
+    ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay) :
+        _func{ func },
+        _delay{ delay }
+    {
+    }
+
+    // Method Description:
+    // - Runs the function later with the specified argument, except if `Run`
+    //   is called again before with a new argument, in which case the new
+    //   argument will be instead.
+    // - For more information, read the "Abstract" section in the header file.
+    // Arguments:
+    // - arg: the argument to pass to the function
+    // Return Value:
+    // - <none>
+    winrt::fire_and_forget Run(T arg)
+    {
+        if (!_pendingCallArg.Emplace(arg))
+        {
+            // already pending
+            return;
+        }
+
+        auto weakThis = this->weak_from_this();
+
+        co_await winrt::resume_after(_delay);
+
+        if (auto self{ weakThis.lock() })
+        {
+            auto arg = self->_pendingCallArg.Take();
+            if (arg.has_value())
+            {
+                self->_func(arg.value());
+            }
+            else
+            {
+                // should not happen
+            }
+        }
+    }
+
+    // Method Description:
+    // - Modifies the pending argument for the next function invocation, if
+    //   there is one pending currently.
+    // - Let's say that you just called the `Run` method with argument A.
+    //   After the delay specified in the constructor, the function R
+    //   specified in the constructor will be called with argument A.
+    // - By using this method, you can modify argument A before the function R
+    //   is called with argument A.
+    // - You pass a function to this method which will take a reference to
+    //   argument A and will modify it.
+    // - When there is no pending invocation of function R, this method will
+    //   not do anything.
+    // - This method is always thread-safe. It can be called multiple times on
+    //   different threads.
+    // Arguments:
+    // - f: the function to call with a reference to the argument
+    // Return Value:
+    // - <none>
+    template<typename F>
+    void ModifyPending(F f)
+    {
+        _pendingCallArg.ModifyValue(f);
+    }
+
+private:
+    Func _func;
+    winrt::Windows::Foundation::TimeSpan _delay;
+    ThreadSafeOptional<T> _pendingCallArg;
+};