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

Improve conhost's scrolling performance #16333

Merged
merged 7 commits into from
Dec 5, 2023
Merged

Improve conhost's scrolling performance #16333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4f2cdb2
Improve conhost's scrolling performance
lhecker Nov 17, 2023
2b5dfef
Fix OneCore build
lhecker Nov 17, 2023
0a6b98f
Fix formatting
lhecker Nov 21, 2023
7c48114
Merge remote-tracking branch 'origin/main' into dev/lhecker/conhost-s…
lhecker Nov 30, 2023
625e232
The tests forced my hand
lhecker Nov 30, 2023
43964cf
Merge remote-tracking branch 'origin/main' into dev/lhecker/conhost-s…
lhecker Dec 4, 2023
88088b6
Fix build
lhecker Dec 4, 2023
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
5 changes: 5 additions & 0 deletions src/host/consoleInformation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ void CONSOLE_INFORMATION::UnlockConsole() noexcept
_lock.unlock();
}

til::recursive_ticket_lock_suspension CONSOLE_INFORMATION::SuspendLock() noexcept
{
return _lock.suspend();
}

ULONG CONSOLE_INFORMATION::GetCSRecursionCount() const noexcept
{
return _lock.recursion_depth();
Expand Down
6 changes: 4 additions & 2 deletions src/host/renderData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,16 @@ std::vector<Viewport> RenderData::GetSelectionRects() noexcept
// they're done with any querying they need to do.
void RenderData::LockConsole() noexcept
{
::LockConsole();
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.LockConsole();
}

// Method Description:
// - Unlocks the console after a call to RenderData::LockConsole.
void RenderData::UnlockConsole() noexcept
{
::UnlockConsole();
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.UnlockConsole();
}

// Method Description:
Expand Down
54 changes: 12 additions & 42 deletions src/host/screenInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ SCREEN_INFORMATION::SCREEN_INFORMATION(
const TextAttribute popupAttributes,
const FontInfo fontInfo) :
OutputMode{ ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT },
ResizingWindow{ 0 },
WheelDelta{ 0 },
HWheelDelta{ 0 },
_textBuffer{ nullptr },
Expand Down Expand Up @@ -641,64 +640,35 @@ VOID SCREEN_INFORMATION::UpdateScrollBars()
return;
}

if (gci.Flags & CONSOLE_UPDATING_SCROLL_BARS)
if (gci.Flags & CONSOLE_UPDATING_SCROLL_BARS || ServiceLocator::LocateConsoleWindow() == nullptr)
{
return;
}

gci.Flags |= CONSOLE_UPDATING_SCROLL_BARS;

if (ServiceLocator::LocateConsoleWindow() != nullptr)
{
ServiceLocator::LocateConsoleWindow()->PostUpdateScrollBars();
}
LOG_IF_WIN32_BOOL_FALSE(ServiceLocator::LocateConsoleWindow()->PostUpdateScrollBars());
}

VOID SCREEN_INFORMATION::InternalUpdateScrollBars()
SCREEN_INFORMATION::ScrollBarState SCREEN_INFORMATION::FetchScrollBarState()
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto pWindow = ServiceLocator::LocateConsoleWindow();

WI_ClearFlag(gci.Flags, CONSOLE_UPDATING_SCROLL_BARS);

if (!IsActiveScreenBuffer())
{
return;
}

ResizingWindow++;

if (pWindow != nullptr)
{
const auto buffer = GetBufferSize();

// If this is the main buffer, make sure we enable both of the scroll bars.
// The alt buffer likely disabled the scroll bars, this is the only
// way to re-enable it.
if (!_IsAltBuffer())
{
pWindow->EnableBothScrollBars();
}

pWindow->UpdateScrollBar(true,
_IsAltBuffer(),
_viewport.Height(),
gci.IsTerminalScrolling() ? _virtualBottom : buffer.BottomInclusive(),
_viewport.Top());
pWindow->UpdateScrollBar(false,
_IsAltBuffer(),
_viewport.Width(),
buffer.RightInclusive(),
_viewport.Left());
}

// Fire off an event to let accessibility apps know the layout has changed.
if (_pAccessibilityNotifier)
{
_pAccessibilityNotifier->NotifyConsoleLayoutEvent();
}

ResizingWindow--;
const auto buffer = GetBufferSize();
const auto isAltBuffer = _IsAltBuffer();
const auto maxSizeVer = gci.IsTerminalScrolling() ? _virtualBottom : buffer.BottomInclusive();
const auto maxSizeHor = buffer.RightInclusive();
return ScrollBarState{
.maxSize = { maxSizeHor, maxSizeVer },
.viewport = _viewport.ToExclusive(),
.isAltBuffer = isAltBuffer,
};
}

// Routine Description:
Expand Down
9 changes: 7 additions & 2 deletions src/host/screenInfo.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,14 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
bool HasAccessibilityEventing() const noexcept;
void NotifyAccessibilityEventing(const til::CoordType sStartX, const til::CoordType sStartY, const til::CoordType sEndX, const til::CoordType sEndY);

struct ScrollBarState
{
til::size maxSize;
til::rect viewport;
bool isAltBuffer = false;
};
void UpdateScrollBars();
void InternalUpdateScrollBars();
ScrollBarState FetchScrollBarState();
Copy link
Member Author

@lhecker lhecker Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with the tests was that briefly unlocking the console allowed window-resize messages to creep in.

To fix that, I had to do what I explained last week in our meeting: Instead of reaching through SCREEN_INFORMATION back into Window, I now leave control entirely inside Window. This allows me to properly unlock the console at the right time and also leave it unlocked.
This however made it necessary to move SCREEN_INFORMATION::ResizingWindow into Window, since the console lock is now not being held anymore. That's also why all the calls got changed from InternalUpdateScrollBars to just UpdateScrollBars. That is, now we're updating the scrollbars asynchronously during launch, etc. In my testing this works just fine.

This change is also another example for why I'm convinced that "loops" in control flow should be avoided (unless they're a good fit of course). Here, Window calls into SCREEN_INFORMATION and it calls back into Window with some extra data. That is very similar to our other callback-centric designs. I don't believe it's a coincidence that they had the same problems too. For instance TerminalInput accessing Terminal state without holding locks, is approximately similar to this code holding the console lock despite not needing it.
Basically I'm advocating for the latter in this graph, and I believe most of the former designs can be transformed to the latter, while retaining all the benefits:

sequenceDiagram
    participant A as class A
    participant B as class B
    participant C as class C
    opt A/B share control
    activate A
    A->>B: foo()
    activate B
    B->>C: bar()
    C-->>B: 
    B-->>A: 
    deactivate B
    deactivate A
    end
    opt only A is in control
    activate A
    A->>B: foo()
    B-->>A: 
    deactivate A
    activate A
    A->>C: bar()
    C-->>A: 
    deactivate A
    end
Loading


bool IsMaximizedBoth() const;
bool IsMaximizedX() const;
Expand Down Expand Up @@ -158,7 +164,6 @@ class SCREEN_INFORMATION : public ConsoleObjectHeader, public Microsoft::Console
bool CursorIsDoubleWidth() const;

DWORD OutputMode;
WORD ResizingWindow; // > 0 if we should ignore WM_SIZE messages

short WheelDelta;
short HWheelDelta;
Expand Down
1 change: 1 addition & 0 deletions src/host/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class CONSOLE_INFORMATION :

void LockConsole() noexcept;
void UnlockConsole() noexcept;
til::recursive_ticket_lock_suspension SuspendLock() noexcept;
bool IsConsoleLocked() const noexcept;
ULONG GetCSRecursionCount() const noexcept;

Expand Down
7 changes: 0 additions & 7 deletions src/interactivity/inc/IConsoleWindow.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,6 @@ namespace Microsoft::Console::Types
public:
virtual ~IConsoleWindow() = default;

virtual BOOL EnableBothScrollBars() = 0;
virtual int UpdateScrollBar(_In_ bool isVertical,
_In_ bool isAltBuffer,
_In_ UINT pageSize,
_In_ int maxSize,
_In_ int viewportPosition) = 0;

virtual bool IsInFullscreen() const = 0;

virtual void SetIsFullscreen(const bool fFullscreenEnabled) = 0;
Expand Down
14 changes: 0 additions & 14 deletions src/interactivity/onecore/ConsoleWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,6 @@
using namespace Microsoft::Console::Interactivity::OneCore;
using namespace Microsoft::Console::Types;

BOOL ConsoleWindow::EnableBothScrollBars() noexcept
{
return FALSE;
}

int ConsoleWindow::UpdateScrollBar(bool /*isVertical*/,
bool /*isAltBuffer*/,
UINT /*pageSize*/,
int /*maxSize*/,
int /*viewportPosition*/) noexcept
{
return 0;
}

bool ConsoleWindow::IsInFullscreen() const noexcept
{
return true;
Expand Down
3 changes: 0 additions & 3 deletions src/interactivity/onecore/ConsoleWindow.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ namespace Microsoft::Console::Interactivity::OneCore
{
public:
// Inherited via IConsoleWindow
BOOL EnableBothScrollBars() noexcept override;
int UpdateScrollBar(bool isVertical, bool isAltBuffer, UINT pageSize, int maxSize, int viewportPosition) noexcept override;

bool IsInFullscreen() const noexcept override;
void SetIsFullscreen(const bool fFullscreenEnabled) noexcept override;
void ChangeViewport(const til::inclusive_rect& NewWindow) override;
Expand Down
49 changes: 26 additions & 23 deletions src/interactivity/win32/window.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ void Window::_CloseWindow() const
ShowWindow(hWnd, wShowWindow);

auto& siAttached = GetScreenInfo();
siAttached.InternalUpdateScrollBars();
siAttached.UpdateScrollBars();
}

return status;
Expand Down Expand Up @@ -591,7 +591,7 @@ void Window::_UpdateWindowSize(const til::size sizeNew)

if (WI_IsFlagClear(gci.Flags, CONSOLE_IS_ICONIC))
{
ScreenInfo.InternalUpdateScrollBars();
ScreenInfo.UpdateScrollBars();

SetWindowPos(GetWindowHandle(),
nullptr,
Expand Down Expand Up @@ -621,7 +621,7 @@ void Window::_UpdateWindowSize(const til::size sizeNew)
if (!IsInFullscreen() && !IsInMaximized())
{
// Figure out how big to make the window, given the desired client area size.
siAttached.ResizingWindow++;
_resizingWindow++;

// First get the buffer viewport size
const auto WindowDimensions = siAttached.GetViewport().Dimensions();
Expand Down Expand Up @@ -691,7 +691,7 @@ void Window::_UpdateWindowSize(const til::size sizeNew)
// If the change wasn't substantial, we may still need to update scrollbar positions. Note that PSReadLine
// scrolls the window via Console.SetWindowPosition, which ultimately calls down to SetConsoleWindowInfo,
// which ends up in this function.
siAttached.InternalUpdateScrollBars();
siAttached.UpdateScrollBars();
}

// MSFT: 12092729
Expand All @@ -716,7 +716,7 @@ void Window::_UpdateWindowSize(const til::size sizeNew)
// an additional Buffer message with the same size again and do nothing special.
ScreenBufferSizeChange(siAttached.GetActiveBuffer().GetBufferSize().Dimensions());

siAttached.ResizingWindow--;
_resizingWindow--;
}

LOG_IF_FAILED(ConsoleImeResizeCompStrView());
Expand Down Expand Up @@ -875,26 +875,29 @@ void Window::HorizontalScroll(const WORD wScrollCommand, const WORD wAbsoluteCha
LOG_IF_FAILED(ScreenInfo.SetViewportOrigin(true, NewOrigin, false));
}

BOOL Window::EnableBothScrollBars()
void Window::UpdateScrollBars(const SCREEN_INFORMATION::ScrollBarState& state)
{
return EnableScrollBar(_hWnd, SB_BOTH, ESB_ENABLE_BOTH);
}
// If this is the main buffer, make sure we enable both of the scroll bars.
// The alt buffer likely disabled the scroll bars, this is the only way to re-enable it.
if (!state.isAltBuffer)
{
EnableScrollBar(_hWnd, SB_BOTH, ESB_ENABLE_BOTH);
}

int Window::UpdateScrollBar(bool isVertical,
bool isAltBuffer,
UINT pageSize,
int maxSize,
int viewportPosition)
{
SCROLLINFO si;
si.cbSize = sizeof(si);
si.fMask = isAltBuffer ? SIF_ALL | SIF_DISABLENOSCROLL : SIF_ALL;
si.nPage = pageSize;
si.nMin = 0;
si.nMax = maxSize;
si.nPos = viewportPosition;

return SetScrollInfo(_hWnd, isVertical ? SB_VERT : SB_HORZ, &si, TRUE);
SCROLLINFO si{
.cbSize = sizeof(SCROLLINFO),
.fMask = static_cast<UINT>(state.isAltBuffer ? SIF_ALL | SIF_DISABLENOSCROLL : SIF_ALL),
};

si.nMax = state.maxSize.width;
si.nPage = state.viewport.width();
si.nPos = state.viewport.left;
SetScrollInfo(_hWnd, SB_HORZ, &si, TRUE);

si.nMax = state.maxSize.height;
si.nPage = state.viewport.height();
si.nPos = state.viewport.top;
SetScrollInfo(_hWnd, SB_VERT, &si, TRUE);
}

// Routine Description:
Expand Down
8 changes: 2 additions & 6 deletions src/interactivity/win32/window.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,7 @@ namespace Microsoft::Console::Interactivity::Win32
void HorizontalScroll(const WORD wScrollCommand,
const WORD wAbsoluteChange);

BOOL EnableBothScrollBars();
int UpdateScrollBar(bool isVertical,
bool isAltBuffer,
UINT pageSize,
int maxSize,
int viewportPosition);
void UpdateScrollBars(const SCREEN_INFORMATION::ScrollBarState& state);

void UpdateWindowSize(const til::size coordSizeInChars);
void UpdateWindowPosition(_In_ const til::point ptNewPos) const;
Expand Down Expand Up @@ -185,6 +180,7 @@ namespace Microsoft::Console::Interactivity::Win32

static void s_ReinitializeFontsForDPIChange();

WORD _resizingWindow = 0; // > 0 if we should ignore WM_SIZE messages
bool _fInDPIChange = false;

static void s_ConvertWindowPosToWindowRect(const LPWINDOWPOS lpWindowPos,
Expand Down
13 changes: 11 additions & 2 deletions src/interactivity/win32/windowproc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,16 @@ using namespace Microsoft::Console::Types;

case CM_UPDATE_SCROLL_BARS:
{
ScreenInfo.InternalUpdateScrollBars();
const auto state = ScreenInfo.FetchScrollBarState();

// EnableScrollbar() and especially SetScrollInfo() are prohibitively expensive functions nowadays.
// Unlocking early here improves throughput of good old `type` in cmd.exe by ~10x.
UnlockConsole();
Unlock = FALSE;

_resizingWindow++;
UpdateScrollBars(state);
_resizingWindow--;
break;
}

Expand Down Expand Up @@ -780,7 +789,7 @@ void Window::_HandleWindowPosChanged(const LPARAM lParam)
// CONSOLE_IS_ICONIC bit appropriately. doing so in the WM_SIZE handler is incorrect because the WM_SIZE
// comes after the WM_ERASEBKGND during SetWindowPos() processing, and the WM_ERASEBKGND needs to know if
// the console window is iconic or not.
if (!ScreenInfo.ResizingWindow && (lpWindowPos->cx || lpWindowPos->cy) && !IsIconic(hWnd))
if (!_resizingWindow && (lpWindowPos->cx || lpWindowPos->cy) && !IsIconic(hWnd))
{
// calculate the dimensions for the newly proposed window rectangle
til::rect rcNew;
Expand Down