wip

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,18 +323,11 @@ 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 (...)
@@ -343,7 +335,6 @@ IRawElementProviderSimple* HwndTerminal::_GetUiaProvider() noexcept
             LOG_HR(wil::ResultFromCaughtException());
             _uiaProvider = nullptr;
         }
-        _uiaProviderInitialized = true;
     }
 
     return _uiaProvider.Get();

src/cascadia/PublicTerminalCore/HwndTerminal.hpp

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

src/cascadia/PublicTerminalCore/PublicTerminalCore.vcxproj

@@ -45,12 +45,4 @@
     </ClCompile>
   </ItemDefinitionGroup>
   <Import Project="$(SolutionDir)src\common.build.post.props" />
-
-  <!-- LATE LINK LINE OVERRIDES. This project must link named forwarders
-       instead of APISet forwarders for easier Windows 7 compatibility. -->
-  <ItemDefinitionGroup>
-    <Link>
-      <AdditionalDependencies>Uiautomationcore.lib;onecoreuap.lib;%(AdditionalDependencies)</AdditionalDependencies>
-    </Link>
-  </ItemDefinitionGroup>
 </Project>

src/cascadia/TerminalCore/Terminal.cpp

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

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

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,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*>(&current), 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));
+    }
+}

src/inc/til/gate.h

@@ -0,0 +1,66 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#pragma once
+
+#include "atomic.h"
+
+namespace til
+{
+    template<std::memory_order AcquireOrder = std::memory_order_acquire, std::memory_order ReleaseOrder = std::memory_order_release>
+    struct gate
+    {
+        constexpr explicit gate(const bool desired) noexcept :
+            _state(desired)
+        {
+        }
+
+        gate(const gate&) = delete;
+        gate& operator=(const gate&) = delete;
+
+        // acquire/release provide classical semaphore semantics.
+        void acquire() noexcept
+        {
+            while (!_state.exchange(false, AcquireOrder))
+            {
+                til::atomic_wait(_state, false);
+            }
+        }
+
+        // acquire/release provide classical semaphore semantics.
+        void release() noexcept
+        {
+            _state.store(true, ReleaseOrder);
+            til::atomic_notify_all(_state);
+        }
+
+        // open/close respectively signal that this "real world gate" is open or closed.
+        // They deviate from the classical semaphore, by not being blocking operations.
+        inline void open() noexcept
+        {
+            release();
+        }
+
+        // open/close respectively signal that this "real world gate" is open or closed.
+        // They deviate from the classical semaphore, by not being blocking operations.
+        void close() noexcept
+        {
+            _state.store(false, ReleaseOrder);
+        }
+
+        // Block until this "real world gate" can be passed, by being open.
+        // This works just like acquiring a semaphore, but doesn't decrease the count.
+        void pass() noexcept
+        {
+            while (!_state.load(AcquireOrder))
+            {
+                til::atomic_wait(_state, false);
+            }
+        }
+
+    private:
+        std::atomic<bool> _state;
+    };
+
+    using gate_relaxed = gate<std::memory_order_relaxed, std::memory_order_relaxed>;
+}

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() 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.
+        std::atomic<uint32_t> _next_ticket{ 0 };
+        std::atomic<uint32_t> _now_serving{ 0 };
+    };
+}

src/renderer/base/thread.cpp

@@ -12,12 +12,10 @@ using namespace Microsoft::Console::Render;
 RenderThread::RenderThread() :
     _pRenderer(nullptr),
     _hThread(nullptr),
-    _hEvent(nullptr),
-    _hPaintCompletedEvent(nullptr),
     _fKeepRunning(true),
-    _hPaintEnabledEvent(nullptr),
-    _fNextFrameRequested(false),
-    _fWaiting(false)
+    _hPaintCompletedEvent(true),
+    _hPaintEnabledEvent(false),
+    _fNextFrameRequested(false)
 {
 }
 
@@ -25,30 +23,11 @@ RenderThread::~RenderThread()
 {
     if (_hThread)
     {
-        _fKeepRunning = false; // stop loop after final run
+        _fKeepRunning.store(false, std::memory_order_relaxed); // stop loop after final run
         EnablePainting(); // if we want to get the last frame out, we need to make sure it's enabled
-        SignalObjectAndWait(_hEvent, _hThread, INFINITE, FALSE); // signal final paint and wait for thread to finish.
+        WaitForSingleObject(_hThread, INFINITE); // wait for thread to finish.
 
         CloseHandle(_hThread);
-        _hThread = nullptr;
-    }
-
-    if (_hEvent)
-    {
-        CloseHandle(_hEvent);
-        _hEvent = nullptr;
-    }
-
-    if (_hPaintEnabledEvent)
-    {
-        CloseHandle(_hPaintEnabledEvent);
-        _hPaintEnabledEvent = nullptr;
-    }
-
-    if (_hPaintCompletedEvent)
-    {
-        CloseHandle(_hPaintCompletedEvent);
-        _hPaintCompletedEvent = nullptr;
     }
 }
 
@@ -65,89 +44,25 @@ RenderThread::~RenderThread()
 {
     _pRenderer = pRendererParent;
 
-    HRESULT hr = S_OK;
-    // Create event before thread as thread will start immediately.
-    if (SUCCEEDED(hr))
-    {
-        HANDLE hEvent = CreateEventW(nullptr, // non-inheritable security attributes
-                                     FALSE, // auto reset event
-                                     FALSE, // initially unsignaled
-                                     nullptr // no name
-        );
+    _hThread = CreateThread(
+        nullptr, // non-inheritable security attributes
+        0, // use default stack size
+        s_ThreadProc,
+        this,
+        0, // create immediately
+        nullptr // we don't need the thread ID
+    );
+    RETURN_LAST_ERROR_IF_NULL(_hThread);
 
-        if (hEvent == nullptr)
-        {
-            hr = HRESULT_FROM_WIN32(GetLastError());
-        }
-        else
-        {
-            _hEvent = hEvent;
-        }
+    // SetThreadDescription only works on 1607 and higher. If we cannot find it,
+    // then it's no big deal. Just skip setting the description.
+    auto func = GetProcAddressByFunctionDeclaration(GetModuleHandleW(L"kernel32.dll"), SetThreadDescription);
+    if (func)
+    {
+        LOG_IF_FAILED(func(_hThread, L"Rendering Output Thread"));
     }
 
-    if (SUCCEEDED(hr))
-    {
-        HANDLE hPaintEnabledEvent = CreateEventW(nullptr,
-                                                 TRUE, // manual reset event
-                                                 FALSE, // initially signaled
-                                                 nullptr);
-
-        if (hPaintEnabledEvent == nullptr)
-        {
-            hr = HRESULT_FROM_WIN32(GetLastError());
-        }
-        else
-        {
-            _hPaintEnabledEvent = hPaintEnabledEvent;
-        }
-    }
-
-    if (SUCCEEDED(hr))
-    {
-        HANDLE hPaintCompletedEvent = CreateEventW(nullptr,
-                                                   TRUE, // manual reset event
-                                                   TRUE, // initially signaled
-                                                   nullptr);
-
-        if (hPaintCompletedEvent == nullptr)
-        {
-            hr = HRESULT_FROM_WIN32(GetLastError());
-        }
-        else
-        {
-            _hPaintCompletedEvent = hPaintCompletedEvent;
-        }
-    }
-
-    if (SUCCEEDED(hr))
-    {
-        HANDLE hThread = CreateThread(nullptr, // non-inheritable security attributes
-                                      0, // use default stack size
-                                      s_ThreadProc,
-                                      this,
-                                      0, // create immediately
-                                      nullptr // we don't need the thread ID
-        );
-
-        if (hThread == nullptr)
-        {
-            hr = HRESULT_FROM_WIN32(GetLastError());
-        }
-        else
-        {
-            _hThread = hThread;
-
-            // SetThreadDescription only works on 1607 and higher. If we cannot find it,
-            // then it's no big deal. Just skip setting the description.
-            auto func = GetProcAddressByFunctionDeclaration(GetModuleHandleW(L"kernel32.dll"), SetThreadDescription);
-            if (func)
-            {
-                LOG_IF_FAILED(func(hThread, L"Rendering Output Thread"));
-            }
-        }
-    }
-
-    return hr;
+    return S_OK;
 }
 
 DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter)
@@ -164,61 +79,21 @@ DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter)
     }
 }
 
-DWORD WINAPI RenderThread::_ThreadProc()
+DWORD RenderThread::_ThreadProc()
 {
-    while (_fKeepRunning)
+    while (_fKeepRunning.load(std::memory_order_relaxed))
     {
-        WaitForSingleObject(_hPaintEnabledEvent, INFINITE);
+        //Sleep(8); // frame rate limit to ~60 FPS in practice
 
-        if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel))
-        {
-            // <--
-            // If `NotifyPaint` is called at this point, then it will not
-            // set the event because `_fWaiting` is not `true` yet so we have
-            // to check again below.
+        _hPaintEnabledEvent.pass();
+        _fNextFrameRequested.acquire();
 
-            _fWaiting.store(true, std::memory_order_release);
-
-            // check again now (see comment above)
-            if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel))
-            {
-                // Wait until a next frame is requested.
-                WaitForSingleObject(_hEvent, INFINITE);
-            }
-
-            // <--
-            // If `NotifyPaint` is called at this point, then it _will_ set
-            // the event because `_fWaiting` is `true`, but we're not waiting
-            // anymore!
-            // This can probably happen quite often: imagine a scenario where
-            // we are waiting, and the terminal calls `NotifyPaint` twice
-            // very quickly.
-            // In that case, both calls might end up calling `SetEvent`. The
-            // first one will resume this thread and the second one will
-            // `SetEvent` the event. So the next time we wait, the event will
-            // already be set and we won't actually wait.
-            // Because it can happen often, and because rendering is an
-            // expensive operation, we should reset the event to not render
-            // again if nothing changed.
-
-            _fWaiting.store(false, std::memory_order_release);
-
-            // see comment above
-            ResetEvent(_hEvent);
-        }
-
-        ResetEvent(_hPaintCompletedEvent);
+        _hPaintCompletedEvent.store(false, std::memory_order_relaxed);
 
         _pRenderer->WaitUntilCanRender();
         LOG_IF_FAILED(_pRenderer->PaintFrame());
 
-        SetEvent(_hPaintCompletedEvent);
-
-        // extra check before we sleep since it's a "long" activity, relatively speaking.
-        if (_fKeepRunning)
-        {
-            Sleep(s_FrameLimitMilliseconds);
-        }
+        _hPaintCompletedEvent.store(true, std::memory_order_relaxed);
     }
 
     return S_OK;
@@ -226,24 +101,17 @@ DWORD WINAPI RenderThread::_ThreadProc()
 
 void RenderThread::NotifyPaint()
 {
-    if (_fWaiting.load(std::memory_order_acquire))
-    {
-        SetEvent(_hEvent);
-    }
-    else
-    {
-        _fNextFrameRequested.store(true, std::memory_order_release);
-    }
+    _fNextFrameRequested.release();
 }
 
 void RenderThread::EnablePainting()
 {
-    SetEvent(_hPaintEnabledEvent);
+    _hPaintEnabledEvent.open();
 }
 
 void RenderThread::DisablePainting()
 {
-    ResetEvent(_hPaintEnabledEvent);
+    _hPaintEnabledEvent.close();
 }
 
 void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs)
@@ -299,6 +167,10 @@ void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs)
     //       DirectX on OneCoreUAP times out while switching console
     //       applications.
 
-    ResetEvent(_hPaintEnabledEvent);
-    WaitForSingleObject(_hPaintCompletedEvent, dwTimeoutMs);
+    _hPaintEnabledEvent.close();
+
+    while (!_hPaintCompletedEvent.load(std::memory_order_relaxed))
+    {
+        til::atomic_wait(_hPaintCompletedEvent, false, dwTimeoutMs);
+    }
 }

src/renderer/base/thread.hpp

@@ -17,6 +17,8 @@ Author(s):
 #include "../inc/IRenderer.hpp"
 #include "../inc/IRenderThread.hpp"
 
+#include <til/gate.h>
+
 namespace Microsoft::Console::Render
 {
     class RenderThread final : public IRenderThread
@@ -35,20 +37,14 @@ namespace Microsoft::Console::Render
 
     private:
         static DWORD WINAPI s_ThreadProc(_In_ LPVOID lpParameter);
-        DWORD WINAPI _ThreadProc();
-
-        static DWORD const s_FrameLimitMilliseconds = 8;
+        DWORD _ThreadProc();
 
         HANDLE _hThread;
-        HANDLE _hEvent;
-
-        HANDLE _hPaintEnabledEvent;
-        HANDLE _hPaintCompletedEvent;
-
         IRenderer* _pRenderer; // Non-ownership pointer
 
-        bool _fKeepRunning;
-        std::atomic<bool> _fNextFrameRequested;
-        std::atomic<bool> _fWaiting;
+        std::atomic<bool> _fKeepRunning;
+        std::atomic<bool> _hPaintCompletedEvent;
+        til::gate_relaxed _hPaintEnabledEvent;
+        til::gate_relaxed _fNextFrameRequested;
     };
 }

src/renderer/dx/DxRenderer.cpp

@@ -493,7 +493,7 @@ void DxEngine::_ComputePixelShaderSettings() noexcept
 //   actual failure from the API itself.
 [[nodiscard]] HRESULT DxEngine::_CreateSurfaceHandle() noexcept
 {
-    wil::unique_hmodule hDComp{ LoadLibraryEx(L"Dcomp.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) };
+    wil::unique_hmodule hDComp{ LoadLibraryExW(L"Dcomp.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) };
     RETURN_LAST_ERROR_IF(hDComp.get() == nullptr);
 
     auto fn = GetProcAddressByFunctionDeclaration(hDComp.get(), DCompositionCreateSurfaceHandle);