Allow ThrottledFunc to work on different types of dispatcher (#10187)

#### ⚠️ targets #10051

## Summary of the Pull Request

This updates our `ThrottledFunc`s to take a dispatcher parameter. This means that we can use the `Windows::UI::Core::CoreDispatcher` in the `TermControl`, where there's always a `CoreDispatcher`, and use a `Windows::System::DispatcherQueue` in `ControlCore`/`ControlInteractivity`. When running in-proc, these are always the _same thing_. However, out-of-proc, the core needs a dispatcher queue that's not tied to a UI thread (because the content proces _doesn't have a UI thread!_). 

This lets us get rid of the output event, because we don't need to bubble that event out to the `TermControl` to let it throttle that update anymore. 

## References
* Tear-out: #1256
* Megathread: #5000
* Project: https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] This is a part of #1256
* [x] I work here
* [n/a] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Fortunately, `winrt::resume_foreground` works the same on both a `CoreDispatcher` and a `DispatcherQueue`, so this wasn't too hard!

## Validation Steps Performed

This was validated in `dev/migrie/oop/the-whole-thing` (or `dev/migrie/oop/connection-factory`, I forget which), and I made sure that it worked both in-proc and x-proc. Not only that, _it wasn't any slower_!This reverts commit 04b751faa7.
This commit is contained in:
Mike Griese 2021-08-09 10:21:59 -05:00 committed by GitHub
parent 90ff261c35
commit 9f2d40614b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 169 additions and 100 deletions

View file

@ -203,7 +203,7 @@ namespace winrt::TerminalApp::implementation
_isElevated = _isUserAdmin();
_root = winrt::make_self<TerminalPage>();
_reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(_root->Dispatcher(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() {
_reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(winrt::Windows::System::DispatcherQueue::GetForCurrentThread(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() {
if (auto self{ weakSelf.get() })
{
self->_ReloadSettings();

View file

@ -23,6 +23,16 @@ using namespace winrt::Windows::Graphics::Display;
using namespace winrt::Windows::System;
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);
// The minimum delay between updating the TSF input control.
constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(100);
// The minimum delay between updating the locations of regex patterns
constexpr const auto UpdatePatternLocationsInterval = std::chrono::milliseconds(500);
namespace winrt::Microsoft::Terminal::Control::implementation
{
// Helper static function to ensure that all ambiguous-width glyphs are reported as narrow.
@ -94,6 +104,61 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto pfnTerminalTaskbarProgressChanged = std::bind(&ControlCore::_terminalTaskbarProgressChanged, this);
_terminal->TaskbarProgressChangedCallback(pfnTerminalTaskbarProgressChanged);
// Get our dispatcher. If we're hosted in-proc with XAML, this will get
// us the same dispatcher as TermControl::Dispatcher(). If we're out of
// proc, this'll return null. We'll need to instead make a new
// DispatcherQueue (on a new thread), so we can use that for throttled
// functions.
_dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread();
if (!_dispatcher)
{
auto controller{ winrt::Windows::System::DispatcherQueueController::CreateOnDedicatedThread() };
_dispatcher = controller.DispatcherQueue();
}
// A few different events should be throttled, so they don't fire absolutely all the time:
// * _tsfTryRedrawCanvas: When the cursor position moves, we need to
// inform TSF, so it can move the canvas for the composition. We
// throttle this so that we're not hopping across the process boundary
// every time that the cursor moves.
// * _updatePatternLocations: When there's new output, or we scroll the
// viewport, we should re-check if there are any visible hyperlinks.
// But we don't really need to do this every single time text is
// output, we can limit this update to once every 500ms.
// * _updateScrollBar: Same idea as the TSF update - we don't _really_
// need to hop across the process boundary every time text is output.
// We can throttle this to once every 8ms, which will get us out of
// the way of the main output & rendering threads.
_tsfTryRedrawCanvas = std::make_shared<ThrottledFuncTrailing<>>(
_dispatcher,
TsfRedrawInterval,
[weakThis = get_weak()]() {
if (auto core{ weakThis.get() }; !core->_IsClosing())
{
core->_CursorPositionChangedHandlers(*core, nullptr);
}
});
_updatePatternLocations = std::make_shared<ThrottledFuncTrailing<>>(
_dispatcher,
UpdatePatternLocationsInterval,
[weakThis = get_weak()]() {
if (auto core{ weakThis.get() }; !core->_IsClosing())
{
core->UpdatePatternLocations();
}
});
_updateScrollBar = std::make_shared<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>>(
_dispatcher,
ScrollBarUpdateInterval,
[weakThis = get_weak()](const auto& update) {
if (auto core{ weakThis.get() }; !core->_IsClosing())
{
core->_ScrollPositionChangedHandlers(*core, update);
}
});
UpdateSettings(settings);
}
@ -1103,15 +1168,28 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// TODO GH#9617: refine locking around pattern tree
_terminal->ClearPatternTree();
_ScrollPositionChangedHandlers(*this,
winrt::make<ScrollPositionChangedArgs>(viewTop,
viewHeight,
bufferSize));
// Start the throttled update of our scrollbar.
auto update{ winrt::make<ScrollPositionChangedArgs>(viewTop,
viewHeight,
bufferSize) };
if (!_inUnitTests)
{
_updateScrollBar->Run(update);
}
else
{
_ScrollPositionChangedHandlers(*this, update);
}
// Additionally, start the throttled update of where our links are.
_updatePatternLocations->Run();
}
void ControlCore::_terminalCursorPositionChanged()
{
_CursorPositionChangedHandlers(*this, nullptr);
// When the buffer's cursor moves, start the throttled func to
// eventually dispatch a CursorPositionChanged event.
_tsfTryRedrawCanvas->Run();
}
void ControlCore::_terminalTaskbarProgressChanged()
@ -1221,8 +1299,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void ControlCore::Close()
{
if (!_closing.exchange(true))
if (!_IsClosing())
{
_closing = true;
// Stop accepting new output and state changes before we disconnect everything.
_connection.TerminalOutput(_connectionOutputEventToken);
_connectionStateChangedRevoker.revoke();
@ -1400,18 +1480,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
_terminal->Write(hstr);
// NOTE: We're raising an event here to inform the TermControl that
// output has been received, so it can queue up a throttled
// UpdatePatternLocations call. In the future, we should have the
// _updatePatternLocations ThrottledFunc internal to this class, and
// run on this object's dispatcher queue.
//
// We're not doing that quite yet, because the Core will eventually
// be out-of-proc from the UI thread, and won't be able to just use
// the UI thread as the dispatcher queue thread.
//
// See TODO: https://github.com/microsoft/terminal/projects/5#card-50760282
_ReceivedOutputHandlers(*this, nullptr);
// Start the throttled update of where our hyperlinks are.
_updatePatternLocations->Run();
}
}

View file

@ -168,7 +168,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
bool _initializedTerminal{ false };
std::atomic<bool> _closing{ false };
bool _closing{ false };
TerminalConnection::ITerminalConnection _connection{ nullptr };
event_token _connectionOutputEventToken;
@ -206,6 +206,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
double _panelHeight{ 0 };
double _compositionScale{ 0 };
winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr };
std::shared_ptr<ThrottledFuncTrailing<>> _tsfTryRedrawCanvas;
std::shared_ptr<ThrottledFuncTrailing<>> _updatePatternLocations;
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> _updateScrollBar;
winrt::fire_and_forget _asyncCloseConnection();
void _setFontSize(int fontSize);
@ -239,8 +244,24 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void _connectionOutputHandler(const hstring& hstr);
void _updateHoveredCell(const std::optional<til::point> terminalPosition);
inline bool _IsClosing() const noexcept
{
#ifndef NDEBUG
if (_dispatcher)
{
// _closing isn't atomic and may only be accessed from the main thread.
//
// Though, the unit tests don't actually run in TAEF's main
// thread, so we don't care when we're running in tests.
assert(_inUnitTests || _dispatcher.HasThreadAccess());
}
#endif
return _closing;
}
friend class ControlUnitTests::ControlCoreTests;
friend class ControlUnitTests::ControlInteractivityTests;
bool _inUnitTests{ false };
};
}

View file

@ -33,7 +33,8 @@ using namespace winrt::Windows::ApplicationModel::DataTransfer;
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);
// This is already throttled primarily in the ControlCore, with a timeout of 100ms. We're adding another smaller one here, as the (potentially x-proc) call will come in off the UI thread
constexpr const auto TsfRedrawInterval = std::chrono::milliseconds(8);
// The minimum delay between updating the locations of regex patterns
constexpr const auto UpdatePatternLocationsInterval = std::chrono::milliseconds(500);
@ -64,10 +65,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_interactivity = winrt::make<implementation::ControlInteractivity>(settings, connection);
_core = _interactivity.Core();
// Use a manual revoker on the output event, so we can immediately stop
// worrying about it on destruction.
_coreOutputEventToken = _core.ReceivedOutput({ this, &TermControl::_coreReceivedOutput });
// These events might all be triggered by the connection, but that
// should be drained and closed before we complete destruction. So these
// are safe.
@ -104,37 +101,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
});
// Many of these ThrottledFunc's should be inside ControlCore. However,
// currently they depend on the Dispatcher() of the UI thread, which the
// Core eventually won't have access to. When we get to
// https://github.com/microsoft/terminal/projects/5#card-50760282
// then we'll move the applicable ones.
//
// These four throttled functions are triggered by terminal output and interact with the UI.
// Get our dispatcher. This will get us the same dispatcher as
// TermControl::Dispatcher().
auto dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread();
// These three throttled functions are triggered by terminal output and interact with the UI.
// Since Close() is the point after which we are removed from the UI, but before the
// destructor has run, we MUST check control->_IsClosing() before actually doing anything.
_tsfTryRedrawCanvas = std::make_shared<ThrottledFuncTrailing<>>(
Dispatcher(),
TsfRedrawInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->TSFInputControl().TryRedrawCanvas();
}
});
_updatePatternLocations = std::make_shared<ThrottledFuncTrailing<>>(
Dispatcher(),
UpdatePatternLocationsInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->_core.UpdatePatternLocations();
}
});
_playWarningBell = std::make_shared<ThrottledFuncLeading>(
Dispatcher(),
dispatcher,
TerminalWarningBellInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() }; !control->_IsClosing())
@ -144,7 +119,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
});
_updateScrollBar = std::make_shared<ThrottledFuncTrailing<ScrollBarUpdate>>(
Dispatcher(),
dispatcher,
ScrollBarUpdateInterval,
[weakThis = get_weak()](const auto& update) {
if (auto control{ weakThis.get() }; !control->_IsClosing())
@ -540,7 +515,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// create a custom automation peer with this code pattern:
// (https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/custom-automation-peers)
if (const auto& interactivityAutoPeer = _interactivity.OnCreateAutomationPeer())
if (const auto& interactivityAutoPeer{ _interactivity.OnCreateAutomationPeer() })
{
_automationPeer = winrt::make<implementation::TermControlAutomationPeer>(this, interactivityAutoPeer);
return _automationPeer;
@ -1276,23 +1251,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
CATCH_LOG();
}
void TermControl::_coreReceivedOutput(const IInspectable& /*sender*/,
const IInspectable& /*args*/)
{
// Queue up a throttled UpdatePatternLocations call. In the future, we
// should have the _updatePatternLocations ThrottledFunc internal to
// ControlCore, and run on that object's dispatcher queue.
//
// We're not doing that quite yet, because the Core will eventually
// be out-of-proc from the UI thread, and won't be able to just use
// the UI thread as the dispatcher queue thread.
//
// THIS IS CALLED ON EVERY STRING OF TEXT OUTPUT TO THE TERMINAL. Think
// twice before adding anything here.
_updatePatternLocations->Run();
}
// Method Description:
// - Reset the font size of the terminal to its default size.
// Arguments:
@ -1330,8 +1288,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_updateScrollBar->ModifyPending([](auto& update) {
update.newValue.reset();
});
_updatePatternLocations->Run();
}
// Method Description:
@ -1665,7 +1621,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
update.newValue = args.ViewTop();
_updateScrollBar->Run(update);
_updatePatternLocations->Run();
}
// Method Description:
@ -1673,10 +1628,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// to be where the current cursor position is.
// Arguments:
// - N/A
void TermControl::_CursorPositionChanged(const IInspectable& /*sender*/,
const IInspectable& /*args*/)
winrt::fire_and_forget TermControl::_CursorPositionChanged(const IInspectable& /*sender*/,
const IInspectable& /*args*/)
{
_tsfTryRedrawCanvas->Run();
// Prior to GH#10187, this fired a trailing throttled func to update the
// TSF canvas only every 100ms. Now, the throttling occurs on the
// ControlCore side. If we're told to update the cursor position, we can
// just go ahead and do it.
// This can come in off the COM thread - hop back to the UI thread.
auto weakThis{ get_weak() };
co_await resume_foreground(Dispatcher());
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->TSFInputControl().TryRedrawCanvas();
}
}
hstring TermControl::Title()
@ -1730,8 +1695,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
_closing = true;
_core.ReceivedOutput(_coreOutputEventToken);
_RestorePointerCursorHandlers(*this, nullptr);
// Disconnect the TSF input control so it doesn't receive EditContext events.
TSFInputControl().Close();
_autoScrollTimer.Stop();

View file

@ -153,8 +153,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
bool _focused{ false };
bool _initializedTerminal{ false };
std::shared_ptr<ThrottledFuncTrailing<>> _tsfTryRedrawCanvas;
std::shared_ptr<ThrottledFuncTrailing<>> _updatePatternLocations;
std::shared_ptr<ThrottledFuncLeading> _playWarningBell;
struct ScrollBarUpdate
@ -164,7 +162,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
double newMinimum;
double newViewportSize;
};
std::shared_ptr<ThrottledFuncTrailing<ScrollBarUpdate>> _updateScrollBar;
bool _isInternalScrollBarUpdate;
// Auto scroll occurs when user, while selecting, drags cursor outside
@ -181,8 +181,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
std::optional<Windows::UI::Xaml::DispatcherTimer> _cursorTimer;
std::optional<Windows::UI::Xaml::DispatcherTimer> _blinkTimer;
event_token _coreOutputEventToken;
winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;
inline bool _IsClosing() const noexcept
@ -233,7 +231,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void _TerminalTabColorChanged(const std::optional<til::color> color);
void _ScrollPositionChanged(const IInspectable& sender, const Control::ScrollPositionChangedArgs& args);
void _CursorPositionChanged(const IInspectable& sender, const IInspectable& args);
winrt::fire_and_forget _CursorPositionChanged(const IInspectable& sender, const IInspectable& args);
bool _CapturePointer(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& e);
bool _ReleasePointerCapture(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& e);
@ -265,7 +263,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const int fontHeight,
const bool isInitialChange);
winrt::fire_and_forget _coreTransparencyChanged(IInspectable sender, Control::TransparencyChangedEventArgs args);
void _coreReceivedOutput(const IInspectable& sender, const IInspectable& args);
void _coreRaisedNotice(const IInspectable& s, const Control::NoticeEventArgs& args);
void _coreWarningBell(const IInspectable& sender, const IInspectable& args);
};

View file

@ -56,6 +56,16 @@ namespace ControlUnitTests
return { settings, conn };
}
winrt::com_ptr<Control::implementation::ControlCore> createCore(Control::IControlSettings settings,
TerminalConnection::ITerminalConnection conn)
{
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(settings, conn);
core->_inUnitTests = true;
return core;
}
};
void ControlCoreTests::ComPtrSettings()
@ -71,8 +81,7 @@ namespace ControlUnitTests
{
auto [settings, conn] = _createSettingsAndConnection();
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(*settings, *conn);
auto core = createCore(*settings, *conn);
VERIFY_IS_NOT_NULL(core);
}
@ -80,8 +89,7 @@ namespace ControlUnitTests
{
auto [settings, conn] = _createSettingsAndConnection();
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(*settings, *conn);
auto core = createCore(*settings, *conn);
VERIFY_IS_NOT_NULL(core);
VERIFY_IS_FALSE(core->_initializedTerminal);
@ -99,8 +107,7 @@ namespace ControlUnitTests
settings->UseAcrylic(true);
settings->TintOpacity(0.5f);
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(*settings, *conn);
auto core = createCore(*settings, *conn);
VERIFY_IS_NOT_NULL(core);
// A callback to make sure that we're raising TransparencyChanged events
@ -167,8 +174,7 @@ namespace ControlUnitTests
{
auto [settings, conn] = _createSettingsAndConnection();
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(*settings, *conn);
auto core = createCore(*settings, *conn);
VERIFY_IS_NOT_NULL(core);
Log::Comment(L"Close the Core, like a TermControl would");
@ -190,8 +196,7 @@ namespace ControlUnitTests
// that you don't default to Cascadia*
settings->FontFace(L"Impact");
Log::Comment(L"Create ControlCore object");
auto core = winrt::make_self<Control::implementation::ControlCore>(*settings, *conn);
auto core = createCore(*settings, *conn);
VERIFY_IS_NOT_NULL(core);
VERIFY_ARE_EQUAL(L"Impact", std::wstring_view{ core->_actualFont.GetFaceName() });

View file

@ -73,6 +73,7 @@ namespace ControlUnitTests
auto interactivity = winrt::make_self<Control::implementation::ControlInteractivity>(settings, conn);
VERIFY_IS_NOT_NULL(interactivity);
auto core = interactivity->_core;
core->_inUnitTests = true;
VERIFY_IS_NOT_NULL(core);
return { core, interactivity };
@ -163,6 +164,10 @@ namespace ControlUnitTests
void ControlInteractivityTests::TestScrollWithMouse()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES()
WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{};
auto [settings, conn] = _createSettingsAndConnection();
@ -243,7 +248,7 @@ namespace ControlUnitTests
buttonState);
Log::Comment(NoThrowString().Format(L"internal scrollbar pos:%f", interactivity->_internalScrollbarPosition));
}
Log::Comment(L"Scrolling up more should do nothing");
Log::Comment(L"Scrolling down more should do nothing");
expectedTop = 21;
interactivity->MouseWheel(modifiers,
-WHEEL_DELTA,
@ -257,6 +262,10 @@ namespace ControlUnitTests
void ControlInteractivityTests::CreateSubsequentSelectionWithDragging()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
END_TEST_METHOD_PROPERTIES()
// This is a test for GH#9725
WEX::TestExecution::DisableVerifyExceptions disableVerifyExceptions{};

View file

@ -47,6 +47,8 @@ Licensed under the MIT license.
// Manually include til after we include Windows.Foundation to give it winrt superpowers
#include "til.h"
#include "ThrottledFunc.h"
// Common includes for most tests:
#include "../../inc/argb.h"
#include "../../inc/conattrs.hpp"

View file

@ -24,7 +24,7 @@ public:
//
// After `func` was invoked the state is reset and this cycle is repeated again.
ThrottledFunc(
winrt::Windows::UI::Core::CoreDispatcher dispatcher,
winrt::Windows::System::DispatcherQueue dispatcher,
filetime_duration delay,
function func) :
_dispatcher{ std::move(dispatcher) },
@ -81,7 +81,7 @@ private:
{
if constexpr (leading)
{
_dispatcher.RunAsync(winrt::Windows::UI::Core::CoreDispatcherPriority::Normal, [weakSelf = this->weak_from_this()]() {
_dispatcher.TryEnqueue(winrt::Windows::System::DispatcherQueuePriority::Normal, [weakSelf = this->weak_from_this()]() {
if (auto self{ weakSelf.lock() })
{
try
@ -108,7 +108,7 @@ private:
}
else
{
_dispatcher.RunAsync(winrt::Windows::UI::Core::CoreDispatcherPriority::Normal, [weakSelf = this->weak_from_this()]() {
_dispatcher.TryEnqueue(winrt::Windows::System::DispatcherQueuePriority::Normal, [weakSelf = this->weak_from_this()]() {
if (auto self{ weakSelf.lock() })
{
try
@ -129,7 +129,7 @@ private:
}
FILETIME _delay;
winrt::Windows::UI::Core::CoreDispatcher _dispatcher;
winrt::Windows::System::DispatcherQueue _dispatcher;
function _func;
wil::unique_threadpool_timer _timer;