Improve responsiveness of conhost/ConPTY for large inputs

src/host/CursorBlinker.cpp

@@ -9,22 +9,32 @@
 using namespace Microsoft::Console;
 using namespace Microsoft::Console::Interactivity;
 
+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();
@@ -36,7 +46,7 @@ void CursorBlinker::UpdateSystemMetrics()
     blinkingState.SetBlinkingAllowed(animationsEnabled && _uCaretBlinkTime != INFINITE);
 }
 
-void CursorBlinker::SettingsChanged()
+void CursorBlinker::SettingsChanged() noexcept
 {
     DWORD const dwCaretBlinkTime = ServiceLocator::LocateSystemConfigurationProvider()->GetCaretBlinkTime();
 
@@ -48,12 +58,12 @@ void CursorBlinker::SettingsChanged()
     }
 }
 
-void CursorBlinker::FocusEnd()
+void CursorBlinker::FocusEnd() const noexcept
 {
     KillCaretTimer();
 }
 
-void CursorBlinker::FocusStart()
+void CursorBlinker::FocusStart() const noexcept
 {
     SetCaretTimer();
 }
@@ -65,7 +75,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();
@@ -145,100 +155,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. 1ms is 10000ns.
+    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::Render::BlinkingState;
 using Microsoft::Console::VirtualTerminal::VtIo;
@@ -44,42 +46,41 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() :
 {
     ZeroMemory((void*)&CPInfo, sizeof(CPInfo));
     ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo));
-    InitializeCriticalSection(&_csConsoleLock);
 }
 
-CONSOLE_INFORMATION::~CONSOLE_INFORMATION()
-{
-    DeleteCriticalSection(&_csConsoleLock);
-}
+static thread_local ULONG recursionCount = 0;
+static til::ticket_lock lock;
 
-bool CONSOLE_INFORMATION::IsConsoleLocked() const
+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);
+    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);
+    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

@@ -78,7 +78,6 @@ class CONSOLE_INFORMATION :
 {
 public:
     CONSOLE_INFORMATION();
-    ~CONSOLE_INFORMATION();
     CONSOLE_INFORMATION(const CONSOLE_INFORMATION& c) = delete;
     CONSOLE_INFORMATION& operator=(const CONSOLE_INFORMATION& c) = delete;
 
@@ -104,11 +103,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();
 
@@ -148,7 +146,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;