Improve responsiveness of conhost/ConPTY for large inputs (#11890)

This commit replaces the console lock was replaced with a fair ticket
lock implementation, as only fair locks guarantee us that the renderer
(or other parts) can acquire the lock, once the VT parser has finally
released it.

This is related to #11794.

## Validation Steps Performed
* No obvious crashes ✅
* No text/VT performance regression ✅
* Cursor blinker starts/stops on focus/defocus of the window ✅

src/host/CursorBlinker.cpp

@@ -10,22 +10,32 @@ using namespace Microsoft::Console;
 using namespace Microsoft::Console::Interactivity;
 using namespace Microsoft::Console::Render;
 
+static void CALLBACK CursorTimerRoutineWrapper(_Inout_ PTP_CALLBACK_INSTANCE /*Instance*/, _Inout_opt_ PVOID /*Context*/, _Inout_ PTP_TIMER /*Timer*/)
+{
+    CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+
+    // There's a slight race condition here.
+    // CreateThreadpoolTimer callbacks may be scheduled even after they were canceled.
+    // But I'm not too concerned that this will lead to issues at the time of writing,
+    // as CursorBlinker is allocated as a static variable through the Globals class.
+    // It'd be nice to fix this, but realistically it'll likely not lead to issues.
+    gci.LockConsole();
+    gci.GetCursorBlinker().TimerRoutine(gci.GetActiveOutputBuffer());
+    gci.UnlockConsole();
+}
+
 CursorBlinker::CursorBlinker() :
-    _hCaretBlinkTimer(INVALID_HANDLE_VALUE),
-    _hCaretBlinkTimerQueue(THROW_LAST_ERROR_IF_NULL(CreateTimerQueue())),
+    _timer(THROW_LAST_ERROR_IF_NULL(CreateThreadpoolTimer(&CursorTimerRoutineWrapper, nullptr, nullptr))),
     _uCaretBlinkTime(INFINITE) // default to no blink
 {
 }
 
 CursorBlinker::~CursorBlinker()
 {
-    if (_hCaretBlinkTimerQueue)
-    {
-        DeleteTimerQueueEx(_hCaretBlinkTimerQueue, INVALID_HANDLE_VALUE);
-    }
+    KillCaretTimer();
 }
 
-void CursorBlinker::UpdateSystemMetrics()
+void CursorBlinker::UpdateSystemMetrics() noexcept
 {
     // This can be -1 in a TS session
     _uCaretBlinkTime = ServiceLocator::LocateSystemConfigurationProvider()->GetCaretBlinkTime();
@@ -37,7 +47,7 @@ void CursorBlinker::UpdateSystemMetrics()
     renderSettings.SetRenderMode(RenderSettings::Mode::BlinkAllowed, animationsEnabled && _uCaretBlinkTime != INFINITE);
 }
 
-void CursorBlinker::SettingsChanged()
+void CursorBlinker::SettingsChanged() noexcept
 {
     DWORD const dwCaretBlinkTime = ServiceLocator::LocateSystemConfigurationProvider()->GetCaretBlinkTime();
 
@@ -49,12 +59,12 @@ void CursorBlinker::SettingsChanged()
     }
 }
 
-void CursorBlinker::FocusEnd()
+void CursorBlinker::FocusEnd() const noexcept
 {
     KillCaretTimer();
 }
 
-void CursorBlinker::FocusStart()
+void CursorBlinker::FocusStart() const noexcept
 {
     SetCaretTimer();
 }
@@ -66,7 +76,7 @@ void CursorBlinker::FocusStart()
 // - ScreenInfo - reference to screen info structure.
 // Return Value:
 // - <none>
-void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo)
+void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo) const noexcept
 {
     auto& buffer = ScreenInfo.GetTextBuffer();
     auto& cursor = buffer.GetCursor();
@@ -146,100 +156,23 @@ void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo)
     Scrolling::s_ScrollIfNecessary(ScreenInfo);
 }
 
-void CALLBACK CursorTimerRoutineWrapper(_In_ PVOID /* lpParam */, _In_ BOOLEAN /* TimerOrWaitFired */)
-{
-    // Suppose the following sequence of events takes place:
-    //
-    // 1. The user resizes the console;
-    // 2. The console acquires the console lock;
-    // 3. The current SCREEN_INFORMATION instance is deleted;
-    // 4. This causes the current Cursor instance to be deleted, too;
-    // 5. The Cursor's destructor is called;
-    // => Somewhere between 1 and 5, the timer fires:
-    //    Timer queue timer callbacks execute asynchronously with respect to
-    //    the UI thread under which the numbered steps are taking place.
-    //    Because the callback touches console state, it needs to acquire the
-    //    console lock. But what if the timer callback fires at just the right
-    //    time such that 2 has already acquired the lock?
-    // 6. The Cursor's destructor deletes the timer queue and thereby destroys
-    //    the timer queue timer used for blinking. However, because this
-    //    timer's callback modifies console state, it is prudent to not
-    //    continue the destruction if the callback has already started but has
-    //    not yet finished. Therefore, the destructor waits for the callback to
-    //    finish executing.
-    // => Meanwhile, the callback just happens to be stuck waiting for the
-    //    console lock acquired in step 2. Since the destructor is waiting on
-    //    the callback to complete, and the callback is waiting on the lock,
-    //    which will only be released way after the Cursor instance is deleted,
-    //    the console has now deadlocked.
-    //
-    // As a solution, skip the blink if the console lock is already being held.
-    // Note that critical sections to not have a waitable synchronization
-    // object unless there readily is contention on it. As a result, if we
-    // wanted to wait until the lock became available under the condition of
-    // not being destroyed, things get too complicated.
-    CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-
-    if (gci.TryLockConsole() != false)
-    {
-        // Cursor& cursor = gci.GetActiveOutputBuffer().GetTextBuffer().GetCursor();
-        gci.GetCursorBlinker().TimerRoutine(gci.GetActiveOutputBuffer());
-
-        // This was originally just UnlockConsole, not CONSOLE_INFORMATION::UnlockConsole
-        // Is there a reason it would need to be the global version?
-        gci.UnlockConsole();
-    }
-}
-
 // Routine Description:
 // - If guCaretBlinkTime is -1, we don't want to blink the caret. However, we
 //   need to make sure it gets drawn, so we'll set a short timer. When that
 //   goes off, we'll hit CursorTimerRoutine, and it'll do the right thing if
 //   guCaretBlinkTime is -1.
-void CursorBlinker::SetCaretTimer()
+void CursorBlinker::SetCaretTimer() const noexcept
 {
-    static const DWORD dwDefTimeout = 0x212;
-
-    KillCaretTimer();
+    using filetime_duration = std::chrono::duration<int64_t, std::ratio<1, 10000000>>;
+    static constexpr DWORD dwDefTimeout = 0x212;
 
-    if (_hCaretBlinkTimer == INVALID_HANDLE_VALUE)
-    {
-        bool bRet = true;
-        DWORD dwEffectivePeriod = _uCaretBlinkTime == -1 ? dwDefTimeout : _uCaretBlinkTime;
-
-        bRet = CreateTimerQueueTimer(&_hCaretBlinkTimer,
-                                     _hCaretBlinkTimerQueue,
-                                     CursorTimerRoutineWrapper,
-                                     this,
-                                     dwEffectivePeriod,
-                                     dwEffectivePeriod,
-                                     0);
-
-        LOG_LAST_ERROR_IF(!bRet);
-    }
+    const auto periodInMS = _uCaretBlinkTime == -1 ? dwDefTimeout : _uCaretBlinkTime;
+    // The FILETIME struct measures time in 100ns steps. 10000 thus equals 1ms.
+    auto periodInFiletime = -static_cast<int64_t>(periodInMS) * 10000;
+    SetThreadpoolTimer(_timer.get(), reinterpret_cast<FILETIME*>(&periodInFiletime), periodInMS, 0);
 }
 
-void CursorBlinker::KillCaretTimer()
+void CursorBlinker::KillCaretTimer() const noexcept
 {
-    if (_hCaretBlinkTimer != INVALID_HANDLE_VALUE)
-    {
-        bool bRet = true;
-
-        bRet = DeleteTimerQueueTimer(_hCaretBlinkTimerQueue,
-                                     _hCaretBlinkTimer,
-                                     nullptr);
-
-        // According to https://msdn.microsoft.com/en-us/library/windows/desktop/ms682569(v=vs.85).aspx
-        // A failure to delete the timer with the LastError being ERROR_IO_PENDING means that the timer is
-        // currently in use and will get cleaned up when released. Delete should not be called again.
-        // We treat that case as a success.
-        if (!bRet && GetLastError() != ERROR_IO_PENDING)
-        {
-            LOG_LAST_ERROR();
-        }
-        else
-        {
-            _hCaretBlinkTimer = INVALID_HANDLE_VALUE;
-        }
-    }
+    SetThreadpoolTimer(_timer.get(), nullptr, 0, 0);
 }

src/host/CursorBlinker.hpp

@@ -20,20 +20,18 @@ namespace Microsoft::Console
         CursorBlinker();
         ~CursorBlinker();
 
-        void FocusStart();
-        void FocusEnd();
+        void FocusStart() const noexcept;
+        void FocusEnd() const noexcept;
 
-        void UpdateSystemMetrics();
-        void SettingsChanged();
-        void TimerRoutine(SCREEN_INFORMATION& ScreenInfo);
+        void UpdateSystemMetrics() noexcept;
+        void SettingsChanged() noexcept;
+        void TimerRoutine(SCREEN_INFORMATION& ScreenInfo) const noexcept;
 
     private:
-        // These use Timer Queues:
-        // https://msdn.microsoft.com/en-us/library/windows/desktop/ms687003(v=vs.85).aspx
-        HANDLE _hCaretBlinkTimer; // timer used to periodically blink the cursor
-        HANDLE _hCaretBlinkTimerQueue; // timer queue where the blink timer lives
+        void SetCaretTimer() const noexcept;
+        void KillCaretTimer() const noexcept;
+
+        wil::unique_threadpool_timer_nowait _timer;
         UINT _uCaretBlinkTime;
-        void SetCaretTimer();
-        void KillCaretTimer();
     };
 }

src/host/consoleInformation.cpp

@@ -11,6 +11,8 @@
 #include "../interactivity/inc/ServiceLocator.hpp"
 #include "../types/inc/convert.hpp"
 
+#include <til/ticket_lock.h>
+
 using Microsoft::Console::Interactivity::ServiceLocator;
 using Microsoft::Console::VirtualTerminal::VtIo;
 
@@ -43,42 +45,52 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() :
 {
     ZeroMemory((void*)&CPInfo, sizeof(CPInfo));
     ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo));
-    InitializeCriticalSection(&_csConsoleLock);
-}
-
-CONSOLE_INFORMATION::~CONSOLE_INFORMATION()
-{
-    DeleteCriticalSection(&_csConsoleLock);
 }
 
-bool CONSOLE_INFORMATION::IsConsoleLocked() const
+// Access to thread-local variables is thread-safe, because each thread
+// gets its own copy of this variable with a default value of 0.
+//
+// Whenever we want to acquire the lock we increment recursionCount and on
+// each release we decrement it again. We can then make the lock safe for
+// reentrancy by only acquiring/releasing the lock if the recursionCount is 0.
+// In a sense, recursionCount is counting the actual function
+// call recursion depth of the caller. This works as long as
+// the caller makes sure to properly call Unlock() once for each Lock().
+static thread_local ULONG recursionCount = 0;
+static til::ticket_lock lock;
+
+bool CONSOLE_INFORMATION::IsConsoleLocked()
 {
-    // The critical section structure's OwningThread field contains the ThreadId despite having the HANDLE type.
-    // This requires us to hard cast the ID to compare.
-    return _csConsoleLock.OwningThread == (HANDLE)GetCurrentThreadId();
+    return recursionCount != 0;
 }
 
 #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
 void CONSOLE_INFORMATION::LockConsole()
 {
-    EnterCriticalSection(&_csConsoleLock);
-}
-
-#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
-bool CONSOLE_INFORMATION::TryLockConsole()
-{
-    return !!TryEnterCriticalSection(&_csConsoleLock);
+    // See description of recursionCount a few lines above.
+    const auto rc = ++recursionCount;
+    FAIL_FAST_IF(rc == 0);
+    if (rc == 1)
+    {
+        lock.lock();
+    }
 }
 
 #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
 void CONSOLE_INFORMATION::UnlockConsole()
 {
-    LeaveCriticalSection(&_csConsoleLock);
+    // See description of recursionCount a few lines above.
+    const auto rc = --recursionCount;
+    FAIL_FAST_IF(rc == ULONG_MAX);
+    if (rc == 0)
+    {
+        lock.unlock();
+    }
 }
 
 ULONG CONSOLE_INFORMATION::GetCSRecursionCount()
 {
-    return _csConsoleLock.RecursionCount;
+    return recursionCount;
 }
 
 // Routine Description:

src/host/server.h

@@ -77,7 +77,6 @@ class CONSOLE_INFORMATION :
 {
 public:
     CONSOLE_INFORMATION();
-    ~CONSOLE_INFORMATION();
     CONSOLE_INFORMATION(const CONSOLE_INFORMATION& c) = delete;
     CONSOLE_INFORMATION& operator=(const CONSOLE_INFORMATION& c) = delete;
 
@@ -103,11 +102,10 @@ class CONSOLE_INFORMATION :
 
     ConsoleImeInfo ConsoleIme;
 
-    void LockConsole();
-    bool TryLockConsole();
-    void UnlockConsole();
-    bool IsConsoleLocked() const;
-    ULONG GetCSRecursionCount();
+    static void LockConsole();
+    static void UnlockConsole();
+    static bool IsConsoleLocked();
+    static ULONG GetCSRecursionCount();
 
     Microsoft::Console::VirtualTerminal::VtIo* GetVtIo();
 
@@ -144,7 +142,6 @@ class CONSOLE_INFORMATION :
     RenderData renderData;
 
 private:
-    CRITICAL_SECTION _csConsoleLock; // serialize input and output using this
     std::wstring _Title;
     std::wstring _Prefix; // Eg Select, Mark - things that we manually prepend to the title.
     std::wstring _TitleAndPrefix;