Skip to content

Commit

Permalink
Eliminate more transient allocations: Titles and invalid rectangles a…
Browse files Browse the repository at this point in the history
…nd bitmap runs and utf8 conversions (#8621)

## References
* See also #8617 

## PR Checklist
* [x] Supports #3075
* [x] I work here.
* [x] Manual test.

## Detailed Description of the Pull Request / Additional comments

### Window Title Generation
Every time the renderer checks the title, it's doing two bad things that
I've fixed:
1. It's assembling the prefix to the full title doing a concatenation.
   No one ever gets just the prefix ever after it is set besides the
   concat. So instead of storing prefix and the title, I store the
   assembled prefix + title and the bare title.
2. A copy must be made because it was returning `std::wstring` instead
   of `std::wstring&`. Now it returns the ref.

### Dirty Area Return
Every time the renderer checks the dirty area, which is sometimes
multiple times per pass (regular text printing, again for selection,
etc.), a vector is created off the heap to return the rectangles. The
consumers only ever iterate this data. Now we return a span over a
rectangle or rectangles that the engine must store itself.
1. For some renderers, it's always a constant 1 element. They update
   that 1 element when dirty is queried and return it in the span with a
   span size of 1.
2. For other renderers with more complex behavior, they're already
   holding a cached vector of rectangles. Now it's effectively giving
   out the ref to those in the span for iteration.

### Bitmap Runs
The `til::bitmap` used a `std::optional<std::vector<til::rectangle>>`
inside itself to cache its runs and would clear the optional when the
runs became invalidated. Unfortunately doing `.reset()` to clear the
optional will destroy the underlying vector and have it release its
memory. We know it's about to get reallocated again, so we're just going
to make it a `std::pmr::vector` and give it a memory pool. 

The alternative solution here was to use a `bool` and
`std::vector<til::rectangle>` and just flag when the vector was invalid,
but that was honestly more code changes and I love excuses to try out
PMR now.

Also, instead of returning the ref to the vector... I'm just returning a
span now. Everyone just iterates it anyway, may as well not share the
implementation detail.

### UTF-8 conversions
When testing with Terminal and looking at the `conhost.exe`'s PTY
renderer, it spends a TON of allocation time on converting all the
UTF-16 stuff inside to UTF-8 before it sends it out the PTY. This was
because `ConvertToA` was allocating a string inside itself and returning
it just to have it freed after printing and looping back around again...
as a PTY does.

The change here is to use `til::u16u8` that accepts a buffer out
parameter so the caller can just hold onto it.

## Validation Steps Performed
- [x] `big.txt` in conhost.exe (GDI renderer)
- [x] `big.txt` in Terminal (DX, PTY renderer)
- [x] Ensure WDDM and BGFX build under Razzle with this change.
  • Loading branch information
miniksa committed Feb 16, 2021
1 parent ca226d6 commit 525be22
Show file tree
Hide file tree
Showing 36 changed files with 187 additions and 135 deletions.
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class Microsoft::Terminal::Core::Terminal final :
void SelectNewRegion(const COORD coordStart, const COORD coordEnd) override;
const COORD GetSelectionAnchor() const noexcept override;
const COORD GetSelectionEnd() const noexcept override;
const std::wstring GetConsoleTitle() const noexcept override;
const std::wstring_view GetConsoleTitle() const noexcept override;
void ColorSelection(const COORD coordSelectionStart, const COORD coordSelectionEnd, const TextAttribute) override;
#pragma endregion

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/terminalrenderdata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ void Terminal::SelectNewRegion(const COORD coordStart, const COORD coordEnd)
SetSelectionEnd(realCoordEnd, SelectionExpansionMode::Cell);
}

const std::wstring Terminal::GetConsoleTitle() const noexcept
const std::wstring_view Terminal::GetConsoleTitle() const noexcept
try
{
if (_title.has_value())
Expand Down
37 changes: 23 additions & 14 deletions src/host/consoleInformation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() :
// ExeAliasList initialized below
_OriginalTitle(),
_Title(),
_Prefix(),
_TitleAndPrefix(),
_LinkTitle(),
Flags(0),
PopupCount(0),
Expand Down Expand Up @@ -115,7 +117,12 @@ ULONG CONSOLE_INFORMATION::GetCSRecursionCount()
try
{
gci.SetTitle(title);
gci.SetOriginalTitle(std::wstring(TranslateConsoleTitle(gci.GetTitle().c_str(), TRUE, FALSE)));

// TranslateConsoleTitle must have a null terminated string.
// This should only happen once on startup so the copy shouldn't be costly
// but could be eliminated by rewriting TranslateConsoleTitle.
const std::wstring nullTerminatedTitle{ gci.GetTitle() };
gci.SetOriginalTitle(std::wstring(TranslateConsoleTitle(nullTerminatedTitle.c_str(), TRUE, FALSE)));
}
catch (...)
{
Expand Down Expand Up @@ -269,6 +276,7 @@ std::pair<COLORREF, COLORREF> CONSOLE_INFORMATION::LookupAttributeColors(const T
void CONSOLE_INFORMATION::SetTitle(const std::wstring_view newTitle)
{
_Title = std::wstring{ newTitle.begin(), newTitle.end() };
_TitleAndPrefix = _Prefix + _Title;

auto* const pRender = ServiceLocator::LocateGlobals().pRender;
if (pRender)
Expand All @@ -284,9 +292,10 @@ void CONSOLE_INFORMATION::SetTitle(const std::wstring_view newTitle)
// - newTitlePrefix: The new value to use for the title prefix
// Return Value:
// - <none>
void CONSOLE_INFORMATION::SetTitlePrefix(const std::wstring& newTitlePrefix)
void CONSOLE_INFORMATION::SetTitlePrefix(const std::wstring_view newTitlePrefix)
{
_TitlePrefix = newTitlePrefix;
_Prefix = newTitlePrefix;
_TitleAndPrefix = _Prefix + _Title;

auto* const pRender = ServiceLocator::LocateGlobals().pRender;
if (pRender)
Expand All @@ -302,7 +311,7 @@ void CONSOLE_INFORMATION::SetTitlePrefix(const std::wstring& newTitlePrefix)
// - originalTitle: The new value to use for the console's original title
// Return Value:
// - <none>
void CONSOLE_INFORMATION::SetOriginalTitle(const std::wstring& originalTitle)
void CONSOLE_INFORMATION::SetOriginalTitle(const std::wstring_view originalTitle)
{
_OriginalTitle = originalTitle;
}
Expand All @@ -314,7 +323,7 @@ void CONSOLE_INFORMATION::SetOriginalTitle(const std::wstring& originalTitle)
// - linkTitle: The new value to use for the console's link title
// Return Value:
// - <none>
void CONSOLE_INFORMATION::SetLinkTitle(const std::wstring& linkTitle)
void CONSOLE_INFORMATION::SetLinkTitle(const std::wstring_view linkTitle)
{
_LinkTitle = linkTitle;
}
Expand All @@ -324,8 +333,8 @@ void CONSOLE_INFORMATION::SetLinkTitle(const std::wstring& linkTitle)
// Arguments:
// - <none>
// Return Value:
// - a reference to the console's title.
const std::wstring& CONSOLE_INFORMATION::GetTitle() const noexcept
// - the console's title.
const std::wstring_view CONSOLE_INFORMATION::GetTitle() const noexcept
{
return _Title;
}
Expand All @@ -336,19 +345,19 @@ const std::wstring& CONSOLE_INFORMATION::GetTitle() const noexcept
// Arguments:
// - <none>
// Return Value:
// - a new wstring containing the combined prefix and title.
const std::wstring CONSOLE_INFORMATION::GetTitleAndPrefix() const
// - the combined prefix and title.
const std::wstring_view CONSOLE_INFORMATION::GetTitleAndPrefix() const
{
return _TitlePrefix + _Title;
return _TitleAndPrefix;
}

// Method Description:
// - return a reference to the console's original title.
// Arguments:
// - <none>
// Return Value:
// - a reference to the console's original title.
const std::wstring& CONSOLE_INFORMATION::GetOriginalTitle() const noexcept
// - the console's original title.
const std::wstring_view CONSOLE_INFORMATION::GetOriginalTitle() const noexcept
{
return _OriginalTitle;
}
Expand All @@ -358,8 +367,8 @@ const std::wstring& CONSOLE_INFORMATION::GetOriginalTitle() const noexcept
// Arguments:
// - <none>
// Return Value:
// - a reference to the console's link title.
const std::wstring& CONSOLE_INFORMATION::GetLinkTitle() const noexcept
// - the console's link title.
const std::wstring_view CONSOLE_INFORMATION::GetLinkTitle() const noexcept
{
return _LinkTitle;
}
Expand Down
20 changes: 4 additions & 16 deletions src/host/getset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1660,33 +1660,21 @@ void DoSrvPrivateRefreshWindow(_In_ const SCREEN_INFORMATION& screenInfo)
}

// Get the appropriate title and length depending on the mode.
const wchar_t* pwszTitle;
size_t cchTitleLength;

if (isOriginal)
{
pwszTitle = gci.GetOriginalTitle().c_str();
cchTitleLength = gci.GetOriginalTitle().length();
}
else
{
pwszTitle = gci.GetTitle().c_str();
cchTitleLength = gci.GetTitle().length();
}
const std::wstring_view storedTitle = isOriginal ? gci.GetOriginalTitle() : gci.GetTitle();

// Always report how much space we would need.
needed = cchTitleLength;
needed = storedTitle.size();

// If we have a pointer to receive the data, then copy it out.
if (title.has_value())
{
HRESULT const hr = StringCchCopyNW(title->data(), title->size(), pwszTitle, cchTitleLength);
HRESULT const hr = StringCchCopyNW(title->data(), title->size(), storedTitle.data(), storedTitle.size());

// Insufficient buffer is allowed. If we return a partial string, that's still OK by historical/compat standards.
// Just say how much we managed to return.
if (SUCCEEDED(hr) || STRSAFE_E_INSUFFICIENT_BUFFER == hr)
{
written = std::min(title->size(), cchTitleLength);
written = std::min(title->size(), storedTitle.size());
}
}
return S_OK;
Expand Down
2 changes: 1 addition & 1 deletion src/host/renderData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ const bool RenderData::IsGridLineDrawingAllowed() noexcept
// - Retrieves the title information to be displayed in the frame/edge of the window
// Return Value:
// - String with title information
const std::wstring RenderData::GetConsoleTitle() const noexcept
const std::wstring_view RenderData::GetConsoleTitle() const noexcept
{
const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
return gci.GetTitleAndPrefix();
Expand Down
2 changes: 1 addition & 1 deletion src/host/renderData.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class RenderData final :

const bool IsGridLineDrawingAllowed() noexcept override;

const std::wstring GetConsoleTitle() const noexcept override;
const std::wstring_view GetConsoleTitle() const noexcept override;

const std::wstring GetHyperlinkUri(uint16_t id) const noexcept override;
const std::wstring GetHyperlinkCustomId(uint16_t id) const noexcept override;
Expand Down
17 changes: 9 additions & 8 deletions src/host/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,13 @@ class CONSOLE_INFORMATION :
std::pair<COLORREF, COLORREF> LookupAttributeColors(const TextAttribute& attr) const noexcept;

void SetTitle(const std::wstring_view newTitle);
void SetTitlePrefix(const std::wstring& newTitlePrefix);
void SetOriginalTitle(const std::wstring& originalTitle);
void SetLinkTitle(const std::wstring& linkTitle);
const std::wstring& GetTitle() const noexcept;
const std::wstring& GetOriginalTitle() const noexcept;
const std::wstring& GetLinkTitle() const noexcept;
const std::wstring GetTitleAndPrefix() const;
void SetTitlePrefix(const std::wstring_view newTitlePrefix);
void SetOriginalTitle(const std::wstring_view originalTitle);
void SetLinkTitle(const std::wstring_view linkTitle);
const std::wstring_view GetTitle() const noexcept;
const std::wstring_view GetOriginalTitle() const noexcept;
const std::wstring_view GetLinkTitle() const noexcept;
const std::wstring_view GetTitleAndPrefix() const;

[[nodiscard]] static NTSTATUS AllocateConsole(const std::wstring_view title);
// MSFT:16886775 : get rid of friends
Expand All @@ -152,7 +152,8 @@ class CONSOLE_INFORMATION :
private:
CRITICAL_SECTION _csConsoleLock; // serialize input and output using this
std::wstring _Title;
std::wstring _TitlePrefix; // Eg Select, Mark - things that we manually prepend to the title.
std::wstring _Prefix; // Eg Select, Mark - things that we manually prepend to the title.
std::wstring _TitleAndPrefix;
std::wstring _OriginalTitle;
std::wstring _LinkTitle; // Path to .lnk file
SCREEN_INFORMATION* pCurrentScreenBuffer;
Expand Down
37 changes: 23 additions & 14 deletions src/host/ut_host/ApiRoutinesTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,12 @@ class ApiRoutinesTests
CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.SetTitle(L"Test window title.");

const auto title = gci.GetTitle();

int const iBytesNeeded = WideCharToMultiByte(gci.OutputCP,
0,
gci.GetTitle().c_str(),
-1,
title.data(),
gsl::narrow_cast<int>(title.size()),
nullptr,
0,
nullptr,
Expand All @@ -221,8 +223,8 @@ class ApiRoutinesTests

VERIFY_WIN32_BOOL_SUCCEEDED(WideCharToMultiByte(gci.OutputCP,
0,
gci.GetTitle().c_str(),
-1,
title.data(),
gsl::narrow_cast<int>(title.size()),
pszExpected.get(),
iBytesNeeded,
nullptr,
Expand Down Expand Up @@ -251,21 +253,26 @@ class ApiRoutinesTests
VERIFY_SUCCEEDED(_pApiRoutines->GetConsoleTitleWImpl(gsl::span<wchar_t>(pwszTitle, ARRAYSIZE(pwszTitle)), cchWritten, cchNeeded));

VERIFY_ARE_NOT_EQUAL(0u, cchWritten);

const auto title = gci.GetTitle();

// NOTE: W version of API returns string length. A version of API returns buffer length (string + null).
VERIFY_ARE_EQUAL(gci.GetTitle().length(), cchWritten);
VERIFY_ARE_EQUAL(gci.GetTitle().length(), cchNeeded);
VERIFY_ARE_EQUAL(WEX::Common::String(gci.GetTitle().c_str()), WEX::Common::String(pwszTitle));
VERIFY_ARE_EQUAL(title.length(), cchWritten);
VERIFY_ARE_EQUAL(title.length(), cchNeeded);
VERIFY_ARE_EQUAL(WEX::Common::String(title.data(), gsl::narrow_cast<int>(title.size())), WEX::Common::String(pwszTitle));
}

TEST_METHOD(ApiGetConsoleOriginalTitleA)
{
CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.SetOriginalTitle(L"Test original window title.");

const auto originalTitle = gci.GetOriginalTitle();

int const iBytesNeeded = WideCharToMultiByte(gci.OutputCP,
0,
gci.GetOriginalTitle().c_str(),
-1,
originalTitle.data(),
gsl::narrow_cast<int>(originalTitle.size()),
nullptr,
0,
nullptr,
Expand All @@ -276,8 +283,8 @@ class ApiRoutinesTests

VERIFY_WIN32_BOOL_SUCCEEDED(WideCharToMultiByte(gci.OutputCP,
0,
gci.GetOriginalTitle().c_str(),
-1,
originalTitle.data(),
gsl::narrow_cast<int>(originalTitle.size()),
pszExpected.get(),
iBytesNeeded,
nullptr,
Expand Down Expand Up @@ -306,10 +313,12 @@ class ApiRoutinesTests
VERIFY_SUCCEEDED(_pApiRoutines->GetConsoleOriginalTitleWImpl(gsl::span<wchar_t>(pwszTitle, ARRAYSIZE(pwszTitle)), cchWritten, cchNeeded));

VERIFY_ARE_NOT_EQUAL(0u, cchWritten);

const auto originalTitle = gci.GetOriginalTitle();
// NOTE: W version of API returns string length. A version of API returns buffer length (string + null).
VERIFY_ARE_EQUAL(gci.GetOriginalTitle().length(), cchWritten);
VERIFY_ARE_EQUAL(gci.GetOriginalTitle().length(), cchNeeded);
VERIFY_ARE_EQUAL(WEX::Common::String(gci.GetOriginalTitle().c_str()), WEX::Common::String(pwszTitle));
VERIFY_ARE_EQUAL(originalTitle.length(), cchWritten);
VERIFY_ARE_EQUAL(originalTitle.length(), cchNeeded);
VERIFY_ARE_EQUAL(WEX::Common::String(originalTitle.data(), gsl::narrow_cast<int>(originalTitle.size())), WEX::Common::String(pwszTitle));
}

static void s_AdjustOutputWait(const bool fShouldBlock)
Expand Down
4 changes: 2 additions & 2 deletions src/host/ut_host/VtIoTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,9 @@ class MockRenderData : public IRenderData, IUiaData
return false;
}

const std::wstring GetConsoleTitle() const noexcept override
const std::wstring_view GetConsoleTitle() const noexcept override
{
return std::wstring{};
return std::wstring_view{};
}

const bool IsSelectionActive() const override
Expand Down
4 changes: 2 additions & 2 deletions src/inc/til/bitmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,15 +263,15 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return const_iterator(_bits, _sz, _sz.area());
}

const std::vector<til::rectangle, run_allocator_type>& runs() const
const gsl::span<const til::rectangle> runs() const
{
// If we don't have cached runs, rebuild.
if (!_runs.has_value())
{
_runs.emplace(begin(), end());
}

// Return a reference to the runs.
// Return the runs.
return _runs.value();
}

Expand Down
11 changes: 8 additions & 3 deletions src/interactivity/onecore/BgfxEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,20 @@ BgfxEngine::BgfxEngine(PVOID SharedViewBase, LONG DisplayHeight, LONG DisplayWid
return S_OK;
}

std::vector<til::rectangle> BgfxEngine::GetDirtyArea()
[[nodiscard]] HRESULT BgfxEngine::GetDirtyArea(gsl::span<const til::rectangle>& area) noexcept
{
SMALL_RECT r;
r.Bottom = _displayHeight > 0 ? (SHORT)(_displayHeight - 1) : 0;
r.Top = 0;
r.Left = 0;
r.Right = _displayWidth > 0 ? (SHORT)(_displayWidth - 1) : 0;

return { r };
_dirtyArea = r;

area = { &_dirtyArea,
1 };

return S_OK;
}

[[nodiscard]] HRESULT BgfxEngine::GetFontSize(_Out_ COORD* const pFontSize) noexcept
Expand All @@ -260,7 +265,7 @@ std::vector<til::rectangle> BgfxEngine::GetDirtyArea()
// - newTitle: the new string to use for the title of the window
// Return Value:
// - S_OK
[[nodiscard]] HRESULT BgfxEngine::_DoUpdateTitle(_In_ const std::wstring& /*newTitle*/) noexcept
[[nodiscard]] HRESULT BgfxEngine::_DoUpdateTitle(_In_ const std::wstring_view /*newTitle*/) noexcept
{
return S_OK;
}
5 changes: 3 additions & 2 deletions src/interactivity/onecore/BgfxEngine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,20 @@ namespace Microsoft::Console::Render

[[nodiscard]] HRESULT GetProposedFont(const FontInfoDesired& fiFontInfoDesired, FontInfo& fiFontInfo, int const iDpi) noexcept override;

std::vector<til::rectangle> GetDirtyArea() override;
[[nodiscard]] HRESULT GetDirtyArea(gsl::span<const til::rectangle>& area) noexcept override;
[[nodiscard]] HRESULT GetFontSize(_Out_ COORD* const pFontSize) noexcept override;
[[nodiscard]] HRESULT IsGlyphWideByFont(const std::wstring_view glyph, _Out_ bool* const pResult) noexcept override;

protected:
[[nodiscard]] HRESULT _DoUpdateTitle(_In_ const std::wstring& newTitle) noexcept override;
[[nodiscard]] HRESULT _DoUpdateTitle(_In_ const std::wstring_view newTitle) noexcept override;

private:
ULONG_PTR _sharedViewBase;
SIZE_T _runLength;

LONG _displayHeight;
LONG _displayWidth;
til::rectangle _dirtyArea;

COORD _fontSize;

Expand Down
Loading

0 comments on commit 525be22

Please sign in to comment.