Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce cost of cursor invalidation #15500

Merged
merged 6 commits into from
Apr 10, 2024
Merged

Reduce cost of cursor invalidation #15500

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7aa3731
Reduce cost of cursor invalidation
lhecker Jun 2, 2023
4da71ff
Merge remote-tracking branch 'origin/main' into dev/lhecker/vt-perf4
lhecker Mar 26, 2024
0600f08
Address feedback
lhecker Mar 26, 2024
ec7ceb8
Address first half of feedback
lhecker Mar 30, 2024
7cae3c5
Address remaining feedback
lhecker Mar 30, 2024
47ef6e7
Address feedback
lhecker Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/buffer/out/LineRendition.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ enum class LineRendition : uint8_t
DoubleHeightBottom
};

constexpr til::rect ScreenToBufferLine(const til::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 };
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right hand side may be off by one when you're dealing with exclusive coordinates with an odd value. For example, if the right screen coordinate is 7 exclusive (6 inclusive), that should map to a buffer coordinate of 4 exclusive (3 inclusive). A simple right shift works for inclusive coordinates (6 >> 1 = 3), but not for exclusive coordinates (7 >> 1 = 3, but we want 4).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I think the current approach is valid as well, in a certain way. Currently, these two related functions round down the width of the buffer:

  • TextBuffer::GetLineWidth
  • ROW::GetReadableColumnCount

If this function where to round up the right coordinate, then passing a viewport sized til::rect will end up having a different size than the size reported by the above two functions.

I do prefer your suggestion, but do we need to change other code first before we can round exclusive coordinates up here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess this means we've had this inconsistency for a while right? Because the inclusive_rect variant of ScreenToBufferLine may have reported an inclusive .right of 59 while the above two functions reported a max. width of 59. Hmm... I'm not sure how to best resolve this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they're expected to be inconsistent. GetLineWidth tells you how many buffer columns you can fit on the screen, so it has to round down if only half of the last column will fit. But ScreenToBufferLine is used to determine how much of the buffer is required to cover a given screen area. If you round down, you won't get enough buffer content, and the last screen cell won't be updated.

Copy link
Member Author

@lhecker lhecker Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah I see what you mean. However, I'm also a little squirmish about what you said. This function isn't used anywhere yet and so I'll remove it for now and we can revisit it later. 🙂

Idle thoughts: Personally speaking, I would prefer if we could consistently use til::rect with its exclusive coordinates everywhere, even in areas of our code where inclusive coordinates would be a better fit, just so that everything works consistently. I wonder if this issue would go away with such a change as well... Probably not really, but it may still avoid some potential for confusion.

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.
Expand All @@ -35,6 +42,13 @@ constexpr til::point ScreenToBufferLine(const til::point& line, const LineRendit
return { line.x >> scale, line.y };
}

constexpr til::rect BufferToScreenLine(const til::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 };
}
lhecker marked this conversation as resolved.
Show resolved Hide resolved

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.
Expand Down
6 changes: 1 addition & 5 deletions src/buffer/out/cursor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,7 @@ void Cursor::_RedrawCursor() noexcept
// - <none>
void Cursor::_RedrawCursorAlways() noexcept
{
try
{
_parentBuffer.TriggerRedrawCursor(_cPosition);
}
CATCH_LOG();
_parentBuffer.NotifyPaintFrame();
}

void Cursor::SetPosition(const til::point cPosition) noexcept
Expand Down
8 changes: 4 additions & 4 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,19 +1240,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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/textBuffer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,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);
Expand Down
6 changes: 2 additions & 4 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
Close();

if (_renderer)
{
_renderer->TriggerTeardown();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer need to _pThread->WaitForPaintCompletionAndDisable(INFINITE);?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of. The destructor blocks until the renderer is fully shut down, which is similar to WaitForPaintCompletionAndDisable but simpler and faster.

I'll re-add the explicit destructor calls here just to be sure nothing regresses. We use a lot of plain/unsafe pointers after all.

}
_renderer.reset();
_renderEngine.reset();
}

void ControlCore::Detach()
Expand Down
11 changes: 2 additions & 9 deletions src/cascadia/TerminalControl/HwndTerminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,15 +249,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();
lhecker marked this conversation as resolved.
Show resolved Hide resolved
// renderer is destroyed
}
// renderEngine is destroyed
}
_renderer.reset();
_renderEngine.reset();

if (auto localHwnd{ _hwnd.release() })
{
Expand Down
2 changes: 1 addition & 1 deletion src/host/getset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading
Loading