Fix output stuttering using a ticket lock (#10653)
`SRWLOCK`, as used by `std::shared_mutex`, is a inherently unfair mutex
and makes no guarantee whatsoever whether a thread may acquire the lock
in a timely manner. This is problematic for our renderer which relies on
being able to acquire the lock in a timely and predictable manner.
Drawing stalls of up to one minute have been observed in tests.
This issue can be solved with a primitive ticket lock, which is 10x
slower than a `SRWLOCK` but still sufficiently fast for our use case
(10M locks per second per thread). It's likely that any non-trivial lock
duration will diminish the difference to "negligible".
## Validation Steps Performed
* It still blends ✔️
This commit is contained in:
parent
fca87b2bb5
commit
f68324cd09
|
@ -172,7 +172,6 @@ HwndTerminal::HwndTerminal(HWND parentHwnd) :
|
|||
_desiredFont{ L"Consolas", 0, DEFAULT_FONT_WEIGHT, { 0, 14 }, CP_UTF8 },
|
||||
_actualFont{ L"Consolas", 0, DEFAULT_FONT_WEIGHT, { 0, 14 }, CP_UTF8, false },
|
||||
_uiaProvider{ nullptr },
|
||||
_uiaProviderInitialized{ false },
|
||||
_currentDpi{ USER_DEFAULT_SCREEN_DPI },
|
||||
_pfnWriteCallback{ nullptr },
|
||||
_multiClickTime{ 500 } // this will be overwritten by the windows system double-click time
|
||||
|
@ -324,26 +323,22 @@ void HwndTerminal::_UpdateFont(int newDpi)
|
|||
|
||||
IRawElementProviderSimple* HwndTerminal::_GetUiaProvider() noexcept
|
||||
{
|
||||
if (nullptr == _uiaProvider && !_uiaProviderInitialized)
|
||||
// If TermControlUiaProvider throws during construction,
|
||||
// we don't want to try constructing an instance again and again.
|
||||
// _uiaProviderInitialized helps us prevent this.
|
||||
if (!_uiaProviderInitialized)
|
||||
{
|
||||
std::unique_lock<std::shared_mutex> lock;
|
||||
try
|
||||
{
|
||||
#pragma warning(suppress : 26441) // The lock is named, this appears to be a false positive
|
||||
lock = _terminal->LockForWriting();
|
||||
if (_uiaProviderInitialized)
|
||||
{
|
||||
return _uiaProvider.Get();
|
||||
}
|
||||
|
||||
auto lock = _terminal->LockForWriting();
|
||||
LOG_IF_FAILED(::Microsoft::WRL::MakeAndInitialize<::Microsoft::Terminal::TermControlUiaProvider>(&_uiaProvider, this->GetUiaData(), this));
|
||||
_uiaProviderInitialized = true;
|
||||
}
|
||||
catch (...)
|
||||
{
|
||||
LOG_HR(wil::ResultFromCaughtException());
|
||||
_uiaProvider = nullptr;
|
||||
}
|
||||
_uiaProviderInitialized = true;
|
||||
}
|
||||
|
||||
return _uiaProvider.Get();
|
||||
|
|
|
@ -73,7 +73,6 @@ private:
|
|||
FontInfoDesired _desiredFont;
|
||||
FontInfo _actualFont;
|
||||
int _currentDpi;
|
||||
bool _uiaProviderInitialized;
|
||||
std::function<void(wchar_t*)> _pfnWriteCallback;
|
||||
::Microsoft::WRL::ComPtr<::Microsoft::Terminal::TermControlUiaProvider> _uiaProvider;
|
||||
|
||||
|
@ -83,6 +82,7 @@ private:
|
|||
std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine;
|
||||
|
||||
bool _focused{ false };
|
||||
bool _uiaProviderInitialized{ false };
|
||||
|
||||
std::chrono::milliseconds _multiClickTime;
|
||||
unsigned int _multiClickCounter{};
|
||||
|
|
|
@ -866,9 +866,9 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
|
|||
// Return Value:
|
||||
// - a shared_lock which can be used to unlock the terminal. The shared_lock
|
||||
// will release this lock when it's destructed.
|
||||
[[nodiscard]] std::shared_lock<std::shared_mutex> Terminal::LockForReading()
|
||||
[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForReading()
|
||||
{
|
||||
return std::shared_lock<std::shared_mutex>(_readWriteLock);
|
||||
return std::unique_lock{ _readWriteLock };
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
|
@ -876,9 +876,9 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
|
|||
// Return Value:
|
||||
// - a unique_lock which can be used to unlock the terminal. The unique_lock
|
||||
// will release this lock when it's destructed.
|
||||
[[nodiscard]] std::unique_lock<std::shared_mutex> Terminal::LockForWriting()
|
||||
[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForWriting()
|
||||
{
|
||||
return std::unique_lock<std::shared_mutex>(_readWriteLock);
|
||||
return std::unique_lock{ _readWriteLock };
|
||||
}
|
||||
|
||||
Viewport Terminal::_GetMutableViewport() const noexcept
|
||||
|
|
|
@ -18,6 +18,8 @@
|
|||
#include "../../cascadia/terminalcore/ITerminalApi.hpp"
|
||||
#include "../../cascadia/terminalcore/ITerminalInput.hpp"
|
||||
|
||||
#include <til/ticket_lock.h>
|
||||
|
||||
static constexpr std::wstring_view linkPattern{ LR"(\b(https?|ftp|file)://[-A-Za-z0-9+&@#/%?=~_|$!:,.;]*[A-Za-z0-9+&@#/%=~_|$])" };
|
||||
static constexpr size_t TaskbarMinProgress{ 10 };
|
||||
|
||||
|
@ -77,8 +79,8 @@ public:
|
|||
// WritePastedText goes directly to the connection
|
||||
void WritePastedText(std::wstring_view stringView);
|
||||
|
||||
[[nodiscard]] std::shared_lock<std::shared_mutex> LockForReading();
|
||||
[[nodiscard]] std::unique_lock<std::shared_mutex> LockForWriting();
|
||||
[[nodiscard]] std::unique_lock<til::ticket_lock> LockForReading();
|
||||
[[nodiscard]] std::unique_lock<til::ticket_lock> LockForWriting();
|
||||
|
||||
short GetBufferHeight() const noexcept;
|
||||
|
||||
|
@ -244,6 +246,15 @@ private:
|
|||
std::function<void()> _pfnWarningBell;
|
||||
std::function<void(std::wstring_view)> _pfnTitleChanged;
|
||||
std::function<void(std::wstring_view)> _pfnCopyToClipboard;
|
||||
|
||||
// I've specifically put this instance here as it requires
|
||||
// alignas(std::hardware_destructive_interference_size)
|
||||
// for best performance.
|
||||
//
|
||||
// But we can abuse the fact that the surrounding members rarely change and are huge
|
||||
// (std::function is like 64 bytes) to create some natural padding without wasting space.
|
||||
til::ticket_lock _readWriteLock;
|
||||
|
||||
std::function<void(const int, const int, const int)> _pfnScrollPositionChanged;
|
||||
std::function<void(const til::color)> _pfnBackgroundColorChanged;
|
||||
std::function<void()> _pfnCursorPositionChanged;
|
||||
|
@ -299,8 +310,6 @@ private:
|
|||
SelectionExpansionMode _multiClickSelectionMode;
|
||||
#pragma endregion
|
||||
|
||||
std::shared_mutex _readWriteLock;
|
||||
|
||||
// TODO: These members are not shared by an alt-buffer. They should be
|
||||
// encapsulated, such that a Terminal can have both a main and alt buffer.
|
||||
std::unique_ptr<TextBuffer> _buffer;
|
||||
|
|
|
@ -230,14 +230,14 @@ catch (...)
|
|||
// they're done with any querying they need to do.
|
||||
void Terminal::LockConsole() noexcept
|
||||
{
|
||||
_readWriteLock.lock_shared();
|
||||
_readWriteLock.lock();
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
// - Unlocks the terminal after a call to Terminal::LockConsole.
|
||||
void Terminal::UnlockConsole() noexcept
|
||||
{
|
||||
_readWriteLock.unlock_shared();
|
||||
_readWriteLock.unlock();
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
|
|
|
@ -0,0 +1,29 @@
|
|||
// Copyright (c) Microsoft Corporation.
|
||||
// Licensed under the MIT license.
|
||||
|
||||
#pragma once
|
||||
|
||||
namespace til
|
||||
{
|
||||
template<typename T>
|
||||
inline void atomic_wait(const std::atomic<T>& atomic, const T& current, DWORD waitMilliseconds = INFINITE) noexcept
|
||||
{
|
||||
static_assert(sizeof(atomic) == sizeof(current));
|
||||
#pragma warning(suppress : 26492) // Don't use const_cast to cast away const or volatile
|
||||
WaitOnAddress(const_cast<std::atomic<T>*>(&atomic), const_cast<T*>(¤t), sizeof(current), waitMilliseconds);
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
inline void atomic_notify_one(const std::atomic<T>& atomic) noexcept
|
||||
{
|
||||
#pragma warning(suppress : 26492) // Don't use const_cast to cast away const or volatile
|
||||
WakeByAddressSingle(const_cast<std::atomic<T>*>(&atomic));
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
inline void atomic_notify_all(const std::atomic<T>& atomic) noexcept
|
||||
{
|
||||
#pragma warning(suppress : 26492) // Don't use const_cast to cast away const or volatile
|
||||
WakeByAddressAll(const_cast<std::atomic<T>*>(&atomic));
|
||||
}
|
||||
}
|
|
@ -0,0 +1,56 @@
|
|||
// Copyright (c) Microsoft Corporation.
|
||||
// Licensed under the MIT license.
|
||||
|
||||
#pragma once
|
||||
|
||||
#include "atomic.h"
|
||||
|
||||
namespace til
|
||||
{
|
||||
// ticket_lock implements a classic fair lock.
|
||||
//
|
||||
// Compared to a SRWLOCK this implementation is significantly more unsafe to use:
|
||||
// Forgetting to call unlock or calling unlock more than once, will lead to deadlocks,
|
||||
// as _now_serving will remain out of sync with _next_ticket and prevent any further lockings.
|
||||
//
|
||||
// I recommend to use the following with this class:
|
||||
// * A low number of concurrent accesses (this lock doesn't scale well beyond 2 threads)
|
||||
// * alignas(std::hardware_destructive_interference_size) to prevent false sharing
|
||||
// * std::unique_lock or std::scoped_lock to prevent unbalanced lock/unlock calls
|
||||
struct ticket_lock
|
||||
{
|
||||
void lock() noexcept
|
||||
{
|
||||
const auto ticket = _next_ticket.fetch_add(1, std::memory_order_relaxed);
|
||||
|
||||
for (;;)
|
||||
{
|
||||
const auto current = _now_serving.load(std::memory_order_acquire);
|
||||
if (current == ticket)
|
||||
{
|
||||
break;
|
||||
}
|
||||
|
||||
til::atomic_wait(_now_serving, current);
|
||||
}
|
||||
}
|
||||
|
||||
void unlock() noexcept
|
||||
{
|
||||
_now_serving.fetch_add(1, std::memory_order_release);
|
||||
til::atomic_notify_all(_now_serving);
|
||||
}
|
||||
|
||||
private:
|
||||
// You may be inclined to add alignas(std::hardware_destructive_interference_size)
|
||||
// here to force the two atomics on separate cache lines, but I suggest to carefully
|
||||
// benchmark such a change. Since this ticket_lock is primarily used to synchronize
|
||||
// exactly 2 threads, it actually helps us that these atomic are on the same cache line
|
||||
// as any change by one thread is flushed to the other, which will then read it anyways.
|
||||
//
|
||||
// Integer overflow doesn't break the algorithm, as these two
|
||||
// atomics are treated more like "IDs" and less like counters.
|
||||
std::atomic<uint32_t> _next_ticket{ 0 };
|
||||
std::atomic<uint32_t> _now_serving{ 0 };
|
||||
};
|
||||
}
|
Loading…
Reference in New Issue