From 58f5d7c72e085abf68ba1b10bd7b18e22f9ca0db Mon Sep 17 00:00:00 2001 From: greg904 <56923875+greg904@users.noreply.github.com> Date: Tue, 23 Jun 2020 23:05:40 +0200 Subject: [PATCH] Update _TerminalCursorPositionChanged to use ThrottledFunc (#6492) * Update _TerminalCursorPositionChanged to use ThrottledFunc. * Rename previous ThrottledFunc to ThrottledArgFunc because now ThrottledFunc is for functions that do not take an argument. * Update ThrottledFunc and ThrottledArgFunc to accept a CoreDispatcher on which the function should be called for convenience. * Don't use coroutines/winrt::fire_and_forget in ThrottledFunc/ThrottledArgFunc because they are too slow (see PR). _AdjustCursorPosition went from 17% of samples to 3% in performance testing. --- src/cascadia/TerminalControl/TermControl.cpp | 79 ++++------ src/cascadia/TerminalControl/TermControl.h | 10 +- .../TerminalControl/TerminalControl.vcxproj | 2 +- .../TerminalControl.vcxproj.filters | 2 +- .../TerminalControl/ThreadSafeOptional.h | 54 ------- .../TerminalControl/ThrottledFunc.cpp | 54 +++++++ src/cascadia/TerminalControl/ThrottledFunc.h | 141 ++++++++++++------ 7 files changed, 179 insertions(+), 163 deletions(-) delete mode 100644 src/cascadia/TerminalControl/ThreadSafeOptional.h create mode 100644 src/cascadia/TerminalControl/ThrottledFunc.cpp diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 341897b17..3d95c078f 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -31,6 +31,9 @@ using namespace winrt::Windows::ApplicationModel::DataTransfer; // The updates are throttled to limit power usage. constexpr const auto ScrollBarUpdateInterval = std::chrono::milliseconds(8); +// The minimum delay between updating the TSF input control. +constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(100); + namespace winrt::Microsoft::Terminal::TerminalControl::implementation { // Helper static function to ensure that all ambiguous-width glyphs are reported as narrow. @@ -124,31 +127,36 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation } }); + _tsfTryRedrawCanvas = std::make_shared>( + [weakThis = get_weak()]() { + if (auto control{ weakThis.get() }) + { + control->TSFInputControl().TryRedrawCanvas(); + } + }, + TsfRedrawInterval, + Dispatcher()); + _updateScrollBar = std::make_shared>( [weakThis = get_weak()](const auto& update) { if (auto control{ weakThis.get() }) { - control->Dispatcher() - .RunAsync(CoreDispatcherPriority::Normal, [=]() { - if (auto control2{ weakThis.get() }) - { - control2->_isInternalScrollBarUpdate = true; + control->_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); + auto scrollBar = control->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; - } - }); + control->_isInternalScrollBarUpdate = false; } }, - ScrollBarUpdateInterval); + ScrollBarUpdateInterval, + Dispatcher()); static constexpr auto AutoScrollUpdateInterval = std::chrono::microseconds(static_cast(1.0 / 30.0 * 1000000)); _autoScrollTimer.Interval(AutoScrollUpdateInterval); @@ -2047,42 +2055,9 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // to be where the current cursor position is. // Arguments: // - N/A - winrt::fire_and_forget TermControl::_TerminalCursorPositionChanged() + void TermControl::_TerminalCursorPositionChanged() { - bool expectedFalse{ false }; - if (!_coroutineDispatchStateUpdateInProgress.compare_exchange_weak(expectedFalse, true)) - { - // somebody's already in here. - return; - } - - if (_closing.load()) - { - return; - } - - auto dispatcher{ Dispatcher() }; // cache a strong ref to this in case TermControl dies - auto weakThis{ get_weak() }; - - // Muffle 2: Muffle Harder - // If we're the lucky coroutine who gets through, we'll still wait 100ms to clog - // the atomic above so we don't service the cursor update too fast. If we get through - // and finish processing the update quickly but similar requests are still beating - // down the door above in the atomic, we may still update the cursor way more than - // is visible to anyone's eye, which is a waste of effort. - static constexpr auto CursorUpdateQuiesceTime{ std::chrono::milliseconds(100) }; - co_await winrt::resume_after(CursorUpdateQuiesceTime); - - co_await winrt::resume_foreground(dispatcher); - - if (auto control{ weakThis.get() }) - { - if (!_closing.load()) - { - TSFInputControl().TryRedrawCanvas(); - } - _coroutineDispatchStateUpdateInProgress.store(false); - } + _tsfTryRedrawCanvas->Run(); } hstring TermControl::Title() diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 9c4740a79..f2805b5f3 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -136,6 +136,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation FontInfoDesired _desiredFont; FontInfo _actualFont; + std::shared_ptr> _tsfTryRedrawCanvas; + struct ScrollBarUpdate { std::optional newValue; @@ -210,7 +212,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _RefreshSizeUnderLock(); void _TerminalTitleChanged(const std::wstring_view& wstr); void _TerminalScrollPositionChanged(const int viewTop, const int viewHeight, const int bufferSize); - winrt::fire_and_forget _TerminalCursorPositionChanged(); + void _TerminalCursorPositionChanged(); void _MouseScrollHandler(const double mouseDelta, const Windows::Foundation::Point point, const bool isLeftButtonPressed); void _MouseZoomHandler(const double delta); @@ -246,12 +248,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _FontInfoHandler(const IInspectable& sender, const FontInfoEventArgs& eventArgs); winrt::fire_and_forget _AsyncCloseConnection(); - - // this atomic is to be used as a guard against dispatching billions of coroutines for - // routine state changes that might happen millions of times a second. - // Unbounded main dispatcher use leads to massive memory leaks and intense slowdowns - // on the UI thread. - std::atomic _coroutineDispatchStateUpdateInProgress{ false }; }; } diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj b/src/cascadia/TerminalControl/TerminalControl.vcxproj index b6fc6f164..bdad948d0 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj @@ -43,7 +43,6 @@ TermControlAutomationPeer.idl - TSFInputControl.xaml @@ -61,6 +60,7 @@ TermControl.xaml + TSFInputControl.xaml diff --git a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters index 04b21717a..f4bdeb33d 100644 --- a/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters +++ b/src/cascadia/TerminalControl/TerminalControl.vcxproj.filters @@ -19,6 +19,7 @@ + @@ -26,7 +27,6 @@ - diff --git a/src/cascadia/TerminalControl/ThreadSafeOptional.h b/src/cascadia/TerminalControl/ThreadSafeOptional.h deleted file mode 100644 index cca4d941a..000000000 --- a/src/cascadia/TerminalControl/ThreadSafeOptional.h +++ /dev/null @@ -1,54 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -#pragma once -#include "pch.h" - -template -class ThreadSafeOptional -{ -public: - template - bool Emplace(Args&&... args) - { - std::lock_guard guard{ _lock }; - - bool hadValue = _inner.has_value(); - _inner.emplace(std::forward(args)...); - return !hadValue; - } - - std::optional Take() - { - std::lock_guard guard{ _lock }; - - std::optional 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: - // - - template - void ModifyValue(F f) - { - std::lock_guard guard{ _lock }; - - if (_inner.has_value()) - { - f(_inner.value()); - } - } - -private: - std::mutex _lock; - std::optional _inner; -}; diff --git a/src/cascadia/TerminalControl/ThrottledFunc.cpp b/src/cascadia/TerminalControl/ThrottledFunc.cpp new file mode 100644 index 000000000..41c60b7b2 --- /dev/null +++ b/src/cascadia/TerminalControl/ThrottledFunc.cpp @@ -0,0 +1,54 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" + +#include "ThrottledFunc.h" + +using namespace winrt::Windows::Foundation; +using namespace winrt::Windows::UI::Core; +using namespace winrt::Windows::UI::Xaml; + +ThrottledFunc<>::ThrottledFunc(ThrottledFunc::Func func, TimeSpan delay, CoreDispatcher dispatcher) : + _func{ func }, + _delay{ delay }, + _dispatcher{ dispatcher }, + _isRunPending{} +{ +} + +// Method Description: +// - Runs the function later, except if `Run` is called again before +// with a new argument, in which case the request will be ignored. +// - For more information, read the class' documentation. +// - This method is always thread-safe. It can be called multiple times on +// different threads. +// Arguments: +// - +// Return Value: +// - +void ThrottledFunc<>::Run() +{ + if (_isRunPending.test_and_set()) + { + // already pending + return; + } + + _dispatcher.RunAsync(CoreDispatcherPriority::Low, [weakThis = this->weak_from_this()]() { + if (auto self{ weakThis.lock() }) + { + DispatcherTimer timer; + timer.Interval(self->_delay); + timer.Tick([=](auto&&...) { + if (auto self{ weakThis.lock() }) + { + timer.Stop(); + self->_isRunPending.clear(); + self->_func(); + } + }); + timer.Start(); + } + }); +} diff --git a/src/cascadia/TerminalControl/ThrottledFunc.h b/src/cascadia/TerminalControl/ThrottledFunc.h index 3d02b3bba..c3887e76b 100644 --- a/src/cascadia/TerminalControl/ThrottledFunc.h +++ b/src/cascadia/TerminalControl/ThrottledFunc.h @@ -4,94 +4,139 @@ 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 -class ThrottledFunc : public std::enable_shared_from_this> +// Class Description: +// - Represents a function that takes arguments and whose invocation is +// delayed by a specified duration and rate-limited such that if the code +// tries to run the function while a call to the function is already +// pending, then the previous call with the previous arguments will be +// cancelled and the call will be made with the new arguments instead. +// - The function will be run on the the specified dispatcher. +template +class ThrottledFunc : public std::enable_shared_from_this> { public: - using Func = std::function; + using Func = std::function; - ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay) : + ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher) : _func{ func }, - _delay{ delay } + _delay{ delay }, + _dispatcher{ dispatcher } { } // 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. + // - Runs the function later with the specified arguments, except if `Run` + // is called again before with new arguments, in which case the new + // arguments will be used instead. + // - For more information, read the class' documentation. + // - This method is always thread-safe. It can be called multiple times on + // different threads. // Arguments: // - arg: the argument to pass to the function // Return Value: // - - winrt::fire_and_forget Run(T arg) + template + void Run(MakeArgs&&... args) { - if (!_pendingCallArg.Emplace(arg)) { - // already pending - return; - } + std::lock_guard guard{ _lock }; - auto weakThis = this->weak_from_this(); + bool hadValue = _pendingRunArgs.has_value(); + _pendingRunArgs.emplace(std::forward(args)...); - co_await winrt::resume_after(_delay); - - if (auto self{ weakThis.lock() }) - { - auto arg = self->_pendingCallArg.Take(); - if (arg.has_value()) + if (hadValue) { - self->_func(arg.value()); - } - else - { - // should not happen + // already pending + return; } } + + _dispatcher.RunAsync(CoreDispatcherPriority::Low, [weakThis = this->weak_from_this()]() { + if (auto self{ weakThis.lock() }) + { + DispatcherTimer timer; + timer.Interval(self->_delay); + timer.Tick([=](auto&&...) { + if (auto self{ weakThis.lock() }) + { + timer.Stop(); + + std::optional> args; + { + std::lock_guard guard{ self->_lock }; + self->_pendingRunArgs.swap(args); + } + std::apply(self->_func, args.value()); + } + }); + timer.Start(); + } + }); } // Method Description: - // - Modifies the pending argument for the next function invocation, if + // - Modifies the pending arguments 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 + // - Let's say that you just called the `Run` method with some arguments. + // After the delay specified in the constructor, the function specified + // in the constructor will be called with these arguments. + // - By using this method, you can modify the arguments before the function + // is called. + // - You pass a function to this method which will take references to + // the arguments (one argument corresponds to one reference to an + // argument) and will modify them. + // - When there is no pending invocation of the function, 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 + // - f: the function to call with references to the arguments // Return Value: // - template void ModifyPending(F f) { - _pendingCallArg.ModifyValue(f); + std::lock_guard guard{ _lock }; + + if (_pendingRunArgs.has_value()) + { + std::apply(f, _pendingRunArgs.value()); + } } private: Func _func; winrt::Windows::Foundation::TimeSpan _delay; - ThreadSafeOptional _pendingCallArg; + winrt::Windows::UI::Core::CoreDispatcher _dispatcher; + + std::optional> _pendingRunArgs; + std::mutex _lock; +}; + +// Class Description: +// - Represents a function whose invocation is delayed by a specified duration +// and rate-limited such that if the code tries to run the function while a +// call to the function is already pending, the request will be ignored. +// - The function will be run on the the specified dispatcher. +template<> +class ThrottledFunc<> : public std::enable_shared_from_this> +{ +public: + using Func = std::function; + + ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher); + + void Run(); + +private: + Func _func; + winrt::Windows::Foundation::TimeSpan _delay; + winrt::Windows::UI::Core::CoreDispatcher _dispatcher; + + std::atomic_flag _isRunPending; };