From 20b0bed46df71c596b4fb68edfb6bb4b4ebf1c89 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 10 Apr 2024 20:51:02 +0200 Subject: [PATCH] Reduce cost of cursor invalidation (#15500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Performance of printing enwik8.txt at the following block sizes: 4KiB (printf): 53MB/s -> 58MB/s 128KiB (cat): 170MB/s -> 235MB/s This commit is imperfect. Support for more than one rendering engine was "hacked" into `Renderer` and is not quite correct. As such, this commit cannot fix cursor invalidation correctly either, and while some bugs are fixed (engines may see highly inconsistent TextBuffer and Cursor states), it introduces others (an error in the first engine may result in the second engine not executing). Neither of those are good and the underlying issue remains to be fixed. ## Validation Steps Performed * Seems ok? ✅ --- src/buffer/out/LineRendition.hpp | 11 +- src/buffer/out/cursor.cpp | 6 +- src/buffer/out/textBuffer.cpp | 20 +- src/buffer/out/textBuffer.hpp | 2 +- src/cascadia/TerminalControl/ControlCore.cpp | 6 +- src/cascadia/TerminalControl/HwndTerminal.cpp | 11 +- src/host/getset.cpp | 2 +- src/renderer/base/renderer.cpp | 190 +++++++++--------- src/renderer/base/renderer.hpp | 3 +- 9 files changed, 125 insertions(+), 126 deletions(-) diff --git a/src/buffer/out/LineRendition.hpp b/src/buffer/out/LineRendition.hpp index 954bd38509f..564f39f03f0 100644 --- a/src/buffer/out/LineRendition.hpp +++ b/src/buffer/out/LineRendition.hpp @@ -23,21 +23,24 @@ enum class LineRendition : uint8_t constexpr til::inclusive_rect ScreenToBufferLine(const til::inclusive_rect& line, const LineRendition lineRendition) { - // Use shift right to quickly divide the Left and Right by 2 for double width lines. const auto scale = lineRendition == LineRendition::SingleWidth ? 0 : 1; return { line.left >> scale, line.top, line.right >> scale, line.bottom }; } -constexpr til::point ScreenToBufferLine(const til::point& line, const LineRendition lineRendition) +constexpr til::point ScreenToBufferLineInclusive(const til::point& line, const LineRendition lineRendition) { - // Use shift right to quickly divide the Left and Right by 2 for double width lines. const auto scale = lineRendition == LineRendition::SingleWidth ? 0 : 1; return { line.x >> scale, line.y }; } +constexpr til::rect BufferToScreenLine(const til::rect& line, const LineRendition lineRendition) +{ + const auto scale = lineRendition == LineRendition::SingleWidth ? 0 : 1; + return { line.left << scale, line.top, line.right << scale, line.bottom }; +} + constexpr til::inclusive_rect BufferToScreenLine(const til::inclusive_rect& line, const LineRendition lineRendition) { - // Use shift left to quickly multiply the Left and Right by 2 for double width lines. const auto scale = lineRendition == LineRendition::SingleWidth ? 0 : 1; return { line.left << scale, line.top, (line.right << scale) + scale, line.bottom }; } diff --git a/src/buffer/out/cursor.cpp b/src/buffer/out/cursor.cpp index 9084a60003d..273c05f8ea1 100644 --- a/src/buffer/out/cursor.cpp +++ b/src/buffer/out/cursor.cpp @@ -188,11 +188,7 @@ void Cursor::_RedrawCursor() noexcept // - void Cursor::_RedrawCursorAlways() noexcept { - try - { - _parentBuffer.TriggerRedrawCursor(_cPosition); - } - CATCH_LOG(); + _parentBuffer.NotifyPaintFrame(); } void Cursor::SetPosition(const til::point cPosition) noexcept diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index bb2be49fd1f..1b5b28c621b 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1265,19 +1265,19 @@ Microsoft::Console::Render::Renderer& TextBuffer::GetRenderer() noexcept return _renderer; } -void TextBuffer::TriggerRedraw(const Viewport& viewport) +void TextBuffer::NotifyPaintFrame() noexcept { if (_isActiveBuffer) { - _renderer.TriggerRedraw(viewport); + _renderer.NotifyPaintFrame(); } } -void TextBuffer::TriggerRedrawCursor(const til::point position) +void TextBuffer::TriggerRedraw(const Viewport& viewport) { if (_isActiveBuffer) { - _renderer.TriggerRedrawCursor(&position); + _renderer.TriggerRedraw(viewport); } } @@ -1920,8 +1920,8 @@ std::vector TextBuffer::GetTextSpans(til::point start, til::poi // equivalent buffer offsets, taking line rendition into account. if (!bufferCoordinates) { - higherCoord = ScreenToBufferLine(higherCoord, GetLineRendition(higherCoord.y)); - lowerCoord = ScreenToBufferLine(lowerCoord, GetLineRendition(lowerCoord.y)); + higherCoord = ScreenToBufferLineInclusive(higherCoord, GetLineRendition(higherCoord.y)); + lowerCoord = ScreenToBufferLineInclusive(lowerCoord, GetLineRendition(lowerCoord.y)); } til::inclusive_rect asRect = { higherCoord.x, higherCoord.y, lowerCoord.x, lowerCoord.y }; @@ -2032,8 +2032,8 @@ std::tuple TextBuffer::_RowCopyHelper(cons if (req.blockSelection) { const auto lineRendition = row.GetLineRendition(); - const auto minX = req.bufferCoordinates ? req.minX : ScreenToBufferLine(til::point{ req.minX, iRow }, lineRendition).x; - const auto maxX = req.bufferCoordinates ? req.maxX : ScreenToBufferLine(til::point{ req.maxX, iRow }, lineRendition).x; + const auto minX = req.bufferCoordinates ? req.minX : ScreenToBufferLineInclusive(til::point{ req.minX, iRow }, lineRendition).x; + const auto maxX = req.bufferCoordinates ? req.maxX : ScreenToBufferLineInclusive(til::point{ req.maxX, iRow }, lineRendition).x; rowBeg = minX; rowEnd = maxX + 1; // +1 to get an exclusive end @@ -2041,8 +2041,8 @@ std::tuple TextBuffer::_RowCopyHelper(cons else { const auto lineRendition = row.GetLineRendition(); - const auto beg = req.bufferCoordinates ? req.beg : ScreenToBufferLine(req.beg, lineRendition); - const auto end = req.bufferCoordinates ? req.end : ScreenToBufferLine(req.end, lineRendition); + const auto beg = req.bufferCoordinates ? req.beg : ScreenToBufferLineInclusive(req.beg, lineRendition); + const auto end = req.bufferCoordinates ? req.end : ScreenToBufferLineInclusive(req.end, lineRendition); rowBeg = iRow != beg.y ? 0 : beg.x; rowEnd = iRow != end.y ? row.GetReadableColumnCount() : end.x + 1; // +1 to get an exclusive end diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 2e1d0532215..c7b68926ef8 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -169,8 +169,8 @@ class TextBuffer final Microsoft::Console::Render::Renderer& GetRenderer() noexcept; + void NotifyPaintFrame() noexcept; void TriggerRedraw(const Microsoft::Console::Types::Viewport& viewport); - void TriggerRedrawCursor(const til::point position); void TriggerRedrawAll(); void TriggerScroll(); void TriggerScroll(const til::point delta); diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index fbe2eaafb77..8a71034217c 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -218,10 +218,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation { Close(); - if (_renderer) - { - _renderer->TriggerTeardown(); - } + _renderer.reset(); + _renderEngine.reset(); } void ControlCore::Detach() diff --git a/src/cascadia/TerminalControl/HwndTerminal.cpp b/src/cascadia/TerminalControl/HwndTerminal.cpp index bd05b7ca8a2..598028a88eb 100644 --- a/src/cascadia/TerminalControl/HwndTerminal.cpp +++ b/src/cascadia/TerminalControl/HwndTerminal.cpp @@ -248,15 +248,8 @@ try // This ensures that teardown is reentrant. // Shut down the renderer (and therefore the thread) before we implode - if (auto localRenderEngine{ std::exchange(_renderEngine, nullptr) }) - { - if (auto localRenderer{ std::exchange(_renderer, nullptr) }) - { - localRenderer->TriggerTeardown(); - // renderer is destroyed - } - // renderEngine is destroyed - } + _renderer.reset(); + _renderEngine.reset(); if (auto localHwnd{ _hwnd.release() }) { diff --git a/src/host/getset.cpp b/src/host/getset.cpp index 495771fdad3..304b4fd65c7 100644 --- a/src/host/getset.cpp +++ b/src/host/getset.cpp @@ -730,7 +730,7 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont // When evaluating the X offset, we must convert the buffer position to // equivalent screen coordinates, taking line rendition into account. const auto lineRendition = buffer.GetTextBuffer().GetLineRendition(position.y); - const auto screenPosition = BufferToScreenLine({ position.x, position.y, position.x, position.y }, lineRendition); + const auto screenPosition = BufferToScreenLine(til::inclusive_rect{ position.x, position.y, position.x, position.y }, lineRendition); if (currentViewport.left > screenPosition.left) { diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index c7c5c491815..4137d140d36 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -64,44 +64,90 @@ Renderer::~Renderer() // - HRESULT S_OK, GDI error, Safe Math error, or state/argument errors. [[nodiscard]] HRESULT Renderer::PaintFrame() { - FOREACH_ENGINE(pEngine) + auto tries = maxRetriesForRenderEngine; + while (tries > 0) { - auto tries = maxRetriesForRenderEngine; - while (tries > 0) + if (_destructing) { - if (_destructing) - { - return S_FALSE; - } + return S_FALSE; + } + + // BODGY: Optimally we would want to retry per engine, but that causes different + // problems (intermittent inconsistent states between text renderer and UIA output, + // not being able to lock the cursor location, etc.). + const auto hr = _PaintFrame(); + if (SUCCEEDED(hr)) + { + break; + } + + LOG_HR_IF(hr, hr != E_PENDING); - const auto hr = _PaintFrameForEngine(pEngine); - if (SUCCEEDED(hr)) + if (--tries == 0) + { + // Stop trying. + _pThread->DisablePainting(); + if (_pfnRendererEnteredErrorState) { - break; + _pfnRendererEnteredErrorState(); } + // If there's no callback, we still don't want to FAIL_FAST: the renderer going black + // isn't near as bad as the entire application aborting. We're a component. We shouldn't + // abort applications that host us. + return S_FALSE; + } + + // Add a bit of backoff. + // Sleep 150ms, 300ms, 450ms before failing out and disabling the renderer. + Sleep(renderBackoffBaseTimeMilliseconds * (maxRetriesForRenderEngine - tries)); + } - LOG_HR_IF(hr, hr != E_PENDING); + return S_OK; +} - if (--tries == 0) +[[nodiscard]] HRESULT Renderer::_PaintFrame() noexcept +{ + { + _pData->LockConsole(); + auto unlock = wil::scope_exit([&]() { + _pData->UnlockConsole(); + }); + + // Last chance check if anything scrolled without an explicit invalidate notification since the last frame. + _CheckViewportAndScroll(); + + if (_currentCursorOptions) + { + const auto coord = _currentCursorOptions->coordCursor; + const auto& buffer = _pData->GetTextBuffer(); + const auto lineRendition = buffer.GetLineRendition(coord.y); + const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1; + + til::rect cursorRect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 }; + cursorRect = BufferToScreenLine(cursorRect, lineRendition); + + if (buffer.GetSize().TrimToViewport(&cursorRect)) { - // Stop trying. - _pThread->DisablePainting(); - if (_pfnRendererEnteredErrorState) + FOREACH_ENGINE(pEngine) { - _pfnRendererEnteredErrorState(); + LOG_IF_FAILED(pEngine->InvalidateCursor(&cursorRect)); } - // If there's no callback, we still don't want to FAIL_FAST: the renderer going black - // isn't near as bad as the entire application aborting. We're a component. We shouldn't - // abort applications that host us. - return S_FALSE; } + } - // Add a bit of backoff. - // Sleep 150ms, 300ms, 450ms before failing out and disabling the renderer. - Sleep(renderBackoffBaseTimeMilliseconds * (maxRetriesForRenderEngine - tries)); + _currentCursorOptions = _GetCursorInfo(); + + FOREACH_ENGINE(pEngine) + { + RETURN_IF_FAILED(_PaintFrameForEngine(pEngine)); } } + FOREACH_ENGINE(pEngine) + { + RETURN_IF_FAILED(pEngine->Present()); + } + return S_OK; } @@ -110,14 +156,6 @@ try { FAIL_FAST_IF_NULL(pEngine); // This is a programming error. Fail fast. - _pData->LockConsole(); - auto unlock = wil::scope_exit([&]() { - _pData->UnlockConsole(); - }); - - // Last chance check if anything scrolled without an explicit invalidate notification since the last frame. - _CheckViewportAndScroll(); - // Try to start painting a frame const auto hr = pEngine->StartPaint(); RETURN_IF_FAILED(hr); @@ -172,12 +210,6 @@ try // Force scope exit end paint to finish up collecting information and possibly painting endPaint.reset(); - // Force scope exit unlock to let go of global lock so other threads can run - unlock.reset(); - - // Trigger out-of-lock presentation for renderers that can support it - RETURN_IF_FAILED(pEngine->Present()); - // As we leave the scope, EndPaint will be called (declared above) return S_OK; } @@ -255,47 +287,6 @@ void Renderer::TriggerRedraw(const til::point* const pcoord) TriggerRedraw(Viewport::FromCoord(*pcoord)); // this will notify to paint if we need it. } -// Routine Description: -// - Called when the cursor has moved in the buffer. Allows for RenderEngines to -// differentiate between cursor movements and other invalidates. -// Visual Renderers (ex GDI) should invalidate the position, while the VT -// engine ignores this. See MSFT:14711161. -// Arguments: -// - pcoord: The buffer-space position of the cursor. -// Return Value: -// - -void Renderer::TriggerRedrawCursor(const til::point* const pcoord) -{ - // We first need to make sure the cursor position is within the buffer, - // otherwise testing for a double width character can throw an exception. - const auto& buffer = _pData->GetTextBuffer(); - if (buffer.GetSize().IsInBounds(*pcoord)) - { - // We then calculate the region covered by the cursor. This requires - // converting the buffer coordinates to an equivalent range of screen - // cells for the cursor, taking line rendition into account. - const auto lineRendition = buffer.GetLineRendition(pcoord->y); - const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1; - til::inclusive_rect cursorRect = { pcoord->x, pcoord->y, pcoord->x + cursorWidth - 1, pcoord->y }; - cursorRect = BufferToScreenLine(cursorRect, lineRendition); - - // That region is then clipped within the viewport boundaries and we - // only trigger a redraw if the resulting region is not empty. - auto view = _pData->GetViewport(); - auto updateRect = til::rect{ cursorRect }; - if (view.TrimToViewport(&updateRect)) - { - view.ConvertToOrigin(&updateRect); - FOREACH_ENGINE(pEngine) - { - LOG_IF_FAILED(pEngine->InvalidateCursor(&updateRect)); - } - - NotifyPaintFrame(); - } - } -} - // Routine Description: // - Called when something that changes the output state has occurred and the entire frame is now potentially invalid. // - NOTE: Use sparingly. Try to reduce the refresh region where possible. Only use when a global state change has occurred. @@ -336,6 +327,8 @@ void Renderer::TriggerTeardown() noexcept // We need to shut down the paint thread on teardown. _pThread->WaitForPaintCompletionAndDisable(INFINITE); + auto repaint = false; + // Then walk through and do one final paint on the caller's thread. FOREACH_ENGINE(pEngine) { @@ -343,10 +336,15 @@ void Renderer::TriggerTeardown() noexcept auto hr = pEngine->PrepareForTeardown(&fEngineRequestsRepaint); LOG_IF_FAILED(hr); - if (SUCCEEDED(hr) && fEngineRequestsRepaint) - { - LOG_IF_FAILED(_PaintFrameForEngine(pEngine)); - } + repaint |= SUCCEEDED(hr) && fEngineRequestsRepaint; + } + + // BODGY: The only time repaint is true is when VtEngine is used. + // Coincidentally VtEngine always runs alone, so if repaint is true, there's only a single engine + // to repaint anyways and there's no danger is accidentally repainting an engine that didn't want to. + if (repaint) + { + LOG_IF_FAILED(_PaintFrame()); } } @@ -468,6 +466,7 @@ void Renderer::TriggerScroll(const til::point* const pcoordDelta) void Renderer::TriggerFlush(const bool circling) { const auto rects = _GetSelectionRects(); + auto repaint = false; FOREACH_ENGINE(pEngine) { @@ -477,10 +476,15 @@ void Renderer::TriggerFlush(const bool circling) LOG_IF_FAILED(pEngine->InvalidateSelection(rects)); - if (SUCCEEDED(hr) && fEngineRequestsRepaint) - { - LOG_IF_FAILED(_PaintFrameForEngine(pEngine)); - } + repaint |= SUCCEEDED(hr) && fEngineRequestsRepaint; + } + + // BODGY: The only time repaint is true is when VtEngine is used. + // Coincidentally VtEngine always runs alone, so if repaint is true, there's only a single engine + // to repaint anyways and there's no danger is accidentally repainting an engine that didn't want to. + if (repaint) + { + LOG_IF_FAILED(_PaintFrame()); } } @@ -642,7 +646,6 @@ void Renderer::EnablePainting() // When the renderer is constructed, the initial viewport won't be available yet, // but once EnablePainting is called it should be safe to retrieve. _viewport = _pData->GetViewport(); - _forceUpdateViewport = true; // When running the unit tests, we may be using a render without a render thread. if (_pThread) @@ -1106,10 +1109,9 @@ bool Renderer::_isInHoveredInterval(const til::point coordTarget) const noexcept // - void Renderer::_PaintCursor(_In_ IRenderEngine* const pEngine) { - const auto cursorInfo = _GetCursorInfo(); - if (cursorInfo.has_value()) + if (_currentCursorOptions) { - LOG_IF_FAILED(pEngine->PaintCursor(cursorInfo.value())); + LOG_IF_FAILED(pEngine->PaintCursor(*_currentCursorOptions)); } } @@ -1127,7 +1129,7 @@ void Renderer::_PaintCursor(_In_ IRenderEngine* const pEngine) [[nodiscard]] HRESULT Renderer::_PrepareRenderInfo(_In_ IRenderEngine* const pEngine) { RenderFrameInfo info; - info.cursorInfo = _GetCursorInfo(); + info.cursorInfo = _currentCursorOptions; return pEngine->PrepareRenderInfo(info); } @@ -1340,6 +1342,11 @@ void Renderer::_ScrollPreviousSelection(const til::point delta) { rc += delta; } + + if (_currentCursorOptions) + { + _currentCursorOptions->coordCursor += delta; + } } } @@ -1361,6 +1368,7 @@ void Renderer::AddRenderEngine(_In_ IRenderEngine* const pEngine) if (!p) { p = pEngine; + _forceUpdateViewport = true; return; } } diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index 1cd61799a8f..dc027acea20 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -50,7 +50,6 @@ namespace Microsoft::Console::Render void TriggerSystemRedraw(const til::rect* const prcDirtyClient); void TriggerRedraw(const Microsoft::Console::Types::Viewport& region); void TriggerRedraw(const til::point* const pcoord); - void TriggerRedrawCursor(const til::point* const pcoord); void TriggerRedrawAll(const bool backgroundChanged = false, const bool frameChanged = false); void TriggerTeardown() noexcept; @@ -96,6 +95,7 @@ namespace Microsoft::Console::Render static GridLineSet s_GetGridlines(const TextAttribute& textAttribute) noexcept; static bool s_IsSoftFontChar(const std::wstring_view& v, const size_t firstSoftFontChar, const size_t lastSoftFontChar); + [[nodiscard]] HRESULT _PaintFrame() noexcept; [[nodiscard]] HRESULT _PaintFrameForEngine(_In_ IRenderEngine* const pEngine) noexcept; bool _CheckViewportAndScroll(); [[nodiscard]] HRESULT _PaintBackground(_In_ IRenderEngine* const pEngine); @@ -126,6 +126,7 @@ namespace Microsoft::Console::Render uint16_t _hyperlinkHoveredId = 0; std::optional::interval> _hoveredInterval; Microsoft::Console::Types::Viewport _viewport; + std::optional _currentCursorOptions; std::vector _clusterBuffer; std::vector _previousSelection; std::vector _previousSearchSelection;