Fix output stuttering using a ticket lock

Author: lhecker

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

@@ -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,18 @@ void HwndTerminal::_UpdateFont(int newDpi)
 
 IRawElementProviderSimple* HwndTerminal::_GetUiaProvider() noexcept
 {
-    if (nullptr == _uiaProvider && !_uiaProviderInitialized)
+    if (!_uiaProvider)
     {
-        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));
         }
         catch (...)
         {
             LOG_HR(wil::ResultFromCaughtException());
             _uiaProvider = nullptr;
         }
-        _uiaProviderInitialized = true;
     }
 
     return _uiaProvider.Get();

src/cascadia/PublicTerminalCore/HwndTerminal.hpp

@@ -73,7 +73,6 @@ struct HwndTerminal : ::Microsoft::Console::Types::IControlAccessibilityInfo
     FontInfoDesired _desiredFont;
     FontInfo _actualFont;
     int _currentDpi;
-    bool _uiaProviderInitialized;
     std::function<void(wchar_t*)> _pfnWriteCallback;
     ::Microsoft::WRL::ComPtr<::Microsoft::Terminal::TermControlUiaProvider> _uiaProvider;
 

src/cascadia/TerminalCore/Terminal.cpp

@@ -866,19 +866,19 @@ 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:
 // - Acquire a write lock on the terminal.
 // 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

src/cascadia/TerminalCore/Terminal.hpp

@@ -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 @@ class Microsoft::Terminal::Core::Terminal final :
     // 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 @@ class Microsoft::Terminal::Core::Terminal final :
     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 @@ class Microsoft::Terminal::Core::Terminal final :
     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;

src/cascadia/TerminalCore/terminalrenderdata.cpp

@@ -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:

src/inc/til/atomic.h

@@ -0,0 +1,26 @@
+// 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)
+    {
+        static_assert(sizeof(atomic) == sizeof(current));
+        WaitOnAddress(const_cast<std::atomic<T>*>(&atomic), const_cast<T*>(&current), sizeof(current), waitMilliseconds);
+    }
+
+    template<typename T>
+    inline void atomic_notify_one(const std::atomic<T>& atomic)
+    {
+        WakeByAddressSingle(const_cast<std::atomic<T>*>(&atomic));
+    }
+
+    template<typename T>
+    inline void atomic_notify_all(const std::atomic<T>& atomic)
+    {
+        WakeByAddressAll(const_cast<std::atomic<T>*>(&atomic));
+    }
+}

src/inc/til/ticket_lock.h

@@ -0,0 +1,44 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#pragma once
+
+#include "atomic.h"
+
+namespace til
+{
+    // The use of alignas(std::hardware_destructive_interference_size) with this class is highly suggested.
+    struct ticket_lock
+    {
+        void lock()
+        {
+            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()
+        {
+            _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.
+        std::atomic<uint32_t> _next_ticket{ 0 };
+        std::atomic<uint32_t> _now_serving{ 0 };
+    };
+}