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.

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

Eliminate more transient allocations: Titles and invalid rectangles and bitmap runs and utf8 conversions #8621

Merged
23 commits merged into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
60bbf49
We never look at just the prefix. So store Title+Prefix concatenated …
miniksa Dec 19, 2020
be5da8d
Finish fixing title. Change all Dirty Areas to be stored in the engin…
miniksa Dec 19, 2020
f66d51e
Make it use wstring_view
miniksa Jan 7, 2021
2983b9d
These should be string views to match.
miniksa Jan 7, 2021
b2e6d6d
Code format!
miniksa Jan 7, 2021
50e8ee8
static analysis
miniksa Jan 7, 2021
141bbd9
fix test things too.
miniksa Jan 7, 2021
ead8fab
Merge branch 'main' into dev/miniksa/perf_title_and_dirty
miniksa Jan 7, 2021
00f80e4
Add held-buffer converter helpers for hot paths.
miniksa Jan 8, 2021
d8cbaed
Hold a conversion buffer string on the VtEngine to prevent hundreds o…
miniksa Jan 8, 2021
1882623
use PMR pool for the vector in the bitmap so we're not alloc/freeing …
miniksa Jan 8, 2021
f378785
Revert "Hold a conversion buffer string on the VtEngine to prevent hu…
miniksa Jan 8, 2021
0f4d33b
Use u16u8 instead of reinventing the wheel on out parameters for conv…
miniksa Jan 8, 2021
1d977b7
code format!'
miniksa Jan 8, 2021
8221b22
Validated in razzle. Forgot to declare the rectangle in bgfx engine.
miniksa Jan 8, 2021
9fefabf
Spell check. codepath --> code path.
miniksa Jan 8, 2021
fa1e14a
missed a noexcept for audit mode.
miniksa Jan 15, 2021
ebbda64
Don't bother adding these functions to Convert since I didn't use 'em…
miniksa Jan 15, 2021
1929b32
Fixed it I think.
miniksa Jan 20, 2021
7662ea8
Merge branch 'main' into dev/miniksa/perf_title_and_dirty
miniksa Feb 12, 2021
c6432ae
ensure that PMR is being used now that it's not strictly a part of th…
miniksa Feb 12, 2021
a6f1b49
Reorder constructors for bitmap how they are in main. Use the three-p…
miniksa Feb 13, 2021
b308974
CODE FORMAT!!!!
miniksa Feb 16, 2021
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
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,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
21 changes: 17 additions & 4 deletions src/inc/til/bitmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,15 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return const_iterator(_bits, _sz, _sz.area());
}

const std::vector<til::rectangle>& 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());
_runs.emplace(begin(), end(), &_getPool());
}

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

Expand Down Expand Up @@ -454,7 +454,20 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
til::rectangle _rc;
dynamic_bitset<> _bits;

mutable std::optional<std::vector<til::rectangle>> _runs;
// Memory pooling to save alloc/free work to the OS.
// The optional below will destroy the internal vector when it is reset.
// But that will do a whole free to the OS when we know we're probably going to rebuild
// it very shortly for the new set of runs. We could make a pair bool/vector
// or continue to use the optional with a PMR vector inside.
// It's static because we only need one pool to manage memory
// for all vectors of rectangles no matter which bitmap instance is making them.
miniksa marked this conversation as resolved.
Show resolved Hide resolved
static std::pmr::unsynchronized_pool_resource& _getPool() noexcept
Copy link
Member

Choose a reason for hiding this comment

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

this will conflict with #8787; should we converge earlier rather than later?

Copy link
Member

Choose a reason for hiding this comment

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

We decided over Teams to converge in this PR, because there's a conflict here anyway.

{
static std::pmr::unsynchronized_pool_resource pool;
return pool;
}

mutable std::optional<std::pmr::vector<til::rectangle>> _runs;

#ifdef UNIT_TESTING
friend class ::BitmapTests;
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
miniksa marked this conversation as resolved.
Show resolved Hide resolved
{
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,
miniksa marked this conversation as resolved.
Show resolved Hide resolved
miniksa marked this conversation as resolved.
Show resolved Hide resolved
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;
}
Loading