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

Jump to bottom

Cache VT buffer line string to avoid (de)alloc on every paint #6840

Merged
merged 2 commits into from
Jul 8, 2020
+15 −12
Merged

Cache VT buffer line string to avoid (de)alloc on every paint #6840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cdb7be3
Hold the buffer line string in the object so we save all the alloc/de…
miniksa Jul 1, 2020
874f222
Code format! *shakes fist*
miniksa Jul 8, 2020
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
21 changes: 10 additions & 11 deletions src/renderer/vt/paint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,17 +334,17 @@ using namespace Microsoft::Console::Types;
{
RETURN_IF_FAILED(_MoveCursor(coord));

std::wstring wstr;
wstr.reserve(clusters.size());
_bufferLine.clear();
_bufferLine.reserve(clusters.size());

short totalWidth = 0;
for (const auto& cluster : clusters)
{
wstr.append(cluster.GetText());
_bufferLine.append(cluster.GetText());
RETURN_IF_FAILED(ShortAdd(totalWidth, gsl::narrow<short>(cluster.GetColumns()), &totalWidth));
}

RETURN_IF_FAILED(VtEngine::_WriteTerminalAscii(wstr));
RETURN_IF_FAILED(VtEngine::_WriteTerminalAscii(_bufferLine));

// Update our internal tracker of the cursor's position
_lastText.X += totalWidth;
Expand All @@ -371,21 +371,21 @@ using namespace Microsoft::Console::Types;
return S_OK;
}

std::wstring unclusteredString;
unclusteredString.reserve(clusters.size());
_bufferLine.clear();
_bufferLine.reserve(clusters.size());
short totalWidth = 0;
for (const auto& cluster : clusters)
{
unclusteredString.append(cluster.GetText());
_bufferLine.append(cluster.GetText());
RETURN_IF_FAILED(ShortAdd(totalWidth, static_cast<short>(cluster.GetColumns()), &totalWidth));
}
const size_t cchLine = unclusteredString.size();
const size_t cchLine = _bufferLine.size();

bool foundNonspace = false;
size_t lastNonSpace = 0;
for (size_t i = 0; i < cchLine; i++)
{
if (unclusteredString.at(i) != L'\x20')
if (_bufferLine.at(i) != L'\x20')
{
lastNonSpace = i;
foundNonspace = true;
Expand Down Expand Up @@ -479,8 +479,7 @@ using namespace Microsoft::Console::Types;
RETURN_IF_FAILED(_MoveCursor(coord));

// Write the actual text string
std::wstring wstr = std::wstring(unclusteredString.data(), cchActual);
RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr));
RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8({ _bufferLine.data(), cchActual }));
Copy link
Member

Choose a reason for hiding this comment

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

is it not possible to pass the wstring directly as a view? bah.

were we literally making a whole copy of the wstring to a wstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why the copy was there. It's probably because the _WriteTerminalUtf8 function converted to using std::wstring_view from std::wstring at a point in time after this paint function was last updated and this became a mechanical replace.

As for why we don't pass _bufferLine directly, the cchActual may be less than the length of the full buffer depending on trimming calculations performed above.


// GH#4415, GH#5181
// If the renderer told us that this was a wrapped line, then mark
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/vt/state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
_newBottomLine{ false },
_deferredCursorPos{ INVALID_COORDS },
_inResizeRequest{ false },
_trace{}
_trace{},
_bufferLine{}
{
#ifndef UNIT_TESTING
// When unit testing, we can instantiate a VtEngine without a pipe.
Expand Down
3 changes: 3 additions & 0 deletions src/renderer/vt/vtrenderer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,9 @@ namespace Microsoft::Console::Render

bool _WillWriteSingleChar() const;

// buffer space for these two functions to build their lines
// so they don't have to alloc/free in a tight loop
std::wstring _bufferLine;
Copy link
Member

Choose a reason for hiding this comment

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

do we want to til::manage?

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed this a bit in Teams. This should max out at approximately (buffer width * sizeof(wchar_t) * 2). We could try to shrink it back down when we start getting small regions, but it would probably grow again when more complicated painting follows up immediately after. The amount of memory left around is arguably small for how much benefit we get from not doing the alloc/dealloc.

I could come up with some sort of time-based decay of the size of the allocation (if someone was doing big draws and has now been doing little draws for a while, it could release it back.... but I don't really have a justification to do all that to pay back ~240 bytes of memory.)

Also, we only have til::manage_vector not til::manage_string at this time. So we'd have to write something for that.

We decided together that this is a big MEH.

[[nodiscard]] HRESULT _PaintUtf8BufferLine(std::basic_string_view<Cluster> const clusters,
const COORD coord,
const bool lineWrapped) noexcept;
Expand Down