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

[WIP] Perf: construct & reuse clusters in output thread #3438

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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/buffer/out/AttrRow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ size_t ATTR_ROW::GetNumberOfRuns() const noexcept
return _list.size();
}

TextAttributeRun ATTR_ROW::GetRunByIndex(size_t index) const
{
return _list.at(index);
}

// Routine Description:
// - This routine finds the nth attribute in this ATTR_ROW.
// Arguments:
Expand Down
1 change: 1 addition & 0 deletions src/buffer/out/AttrRow.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class ATTR_ROW final
size_t* const pApplies) const;

size_t GetNumberOfRuns() const noexcept;
TextAttributeRun GetRunByIndex(size_t index) const;

size_t FindAttrIndex(const size_t index,
size_t* const pApplies) const;
Expand Down
49 changes: 49 additions & 0 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ ROW::ROW(const SHORT rowId, const short rowWidth, const TextAttribute fillAttrib
_attrRow{ gsl::narrow<UINT>(rowWidth), fillAttribute },
_pParent{ pParent }
{
RefreshClusters(rowWidth);
}

size_t ROW::size() const noexcept
Expand Down Expand Up @@ -69,6 +70,8 @@ void ROW::SetId(const SHORT id) noexcept
bool ROW::Reset(const TextAttribute Attr)
{
_charRow.Reset();
RefreshClusters(_rowWidth);

try
{
_attrRow.Reset(Attr);
Expand All @@ -90,6 +93,8 @@ bool ROW::Reset(const TextAttribute Attr)
[[nodiscard]] HRESULT ROW::Resize(const size_t width)
{
RETURN_IF_FAILED(_charRow.Resize(width));
RefreshClusters(width);

try
{
_attrRow.Resize(width);
Expand All @@ -98,6 +103,7 @@ bool ROW::Reset(const TextAttribute Attr)

_rowWidth = width;


return S_OK;
}

Expand All @@ -122,6 +128,11 @@ std::wstring ROW::GetText() const
return _charRow.GetText();
}

std::vector<Microsoft::Console::Render::Cluster> ROW::GetClusters() const
{
return _clusters;
}

RowCellIterator ROW::AsCellIter(const size_t startIndex) const
{
return AsCellIter(startIndex, size() - startIndex);
Expand Down Expand Up @@ -157,6 +168,8 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, co
THROW_HR_IF(E_INVALIDARG, limitRight.value_or(0) >= _charRow.size());
size_t currentIndex = index;

size_t clusterIndex = index;

// If we're given a right-side column limit, use it. Otherwise, the write limit is the final column index available in the char row.
const auto finalColumnInRow = limitRight.value_or(_charRow.size() - 1);

Expand Down Expand Up @@ -199,6 +212,19 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, co
{
_charRow.DbcsAttrAt(currentIndex) = it->DbcsAttr();
_charRow.GlyphAt(currentIndex) = it->Chars();

if (it->DbcsAttr().IsDbcs())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is kind tricky but I need it to handle dbcs since I'm in CJK environment myself. A cluster with column set to zero will be ignored for rendering.

{
if (it->DbcsAttr().IsLeading())
{
_clusters.at(clusterIndex).SetColumns(0);
}
if (it->DbcsAttr().IsTrailing())
{
_clusters.at(clusterIndex).SetColumns(2);
}
}

++it;
}

Expand All @@ -220,7 +246,30 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const size_t index, co

// Move to the next cell for the next time through the loop.
++currentIndex;
++clusterIndex;
}

return it;
}

void ROW::RefreshClusters(const size_t width)
{
_clusters.clear();

for (size_t i = 0; i < width; ++i)
{
_clusters.emplace_back(_charRow.GlyphAt(i), _charRow.GlyphAt(i).operator std::basic_string_view<wchar_t>().size());
const auto dbcsAttr = _charRow.DbcsAttrAt(i);
if (dbcsAttr.IsDbcs())
{
if (dbcsAttr.IsLeading())
{
_clusters.at(i).SetColumns(0);
}
if (dbcsAttr.IsTrailing())
{
_clusters.at(i).SetColumns(2);
}
}
}
}
7 changes: 7 additions & 0 deletions src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ Revision History:
#include "RowCellIterator.hpp"
#include "UnicodeStorage.hpp"

#include "../renderer/inc/Cluster.hpp"

class TextBuffer;

class ROW final
Expand All @@ -50,6 +52,7 @@ class ROW final

void ClearColumn(const size_t column);
std::wstring GetText() const;
std::vector<Microsoft::Console::Render::Cluster> GetClusters() const;

RowCellIterator AsCellIter(const size_t startIndex) const;
RowCellIterator AsCellIter(const size_t startIndex, const size_t count) const;
Expand All @@ -66,8 +69,12 @@ class ROW final
#endif

private:

void RefreshClusters(const size_t width);

CharRow _charRow;
ATTR_ROW _attrRow;
std::vector<Microsoft::Console::Render::Cluster> _clusters;
SHORT _id;
size_t _rowWidth;
TextBuffer* _pParent; // non ownership pointer
Expand Down
3 changes: 1 addition & 2 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -791,8 +791,7 @@ void TextBuffer::Reset()

for (auto& row : _storage)
{
row.GetCharRow().Reset();
row.GetAttrRow().Reset(attr);
row.Reset(attr);
}
}

Expand Down
24 changes: 24 additions & 0 deletions src/renderer/base/Cluster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@

using namespace Microsoft::Console::Render;

Cluster::Cluster() :
_text(L" "),
_columns(1)
{
}

// Routine Description:
// - Instantiates a new cluster structure
// Arguments:
Expand Down Expand Up @@ -59,3 +65,21 @@ const size_t Cluster::GetColumns() const noexcept
{
return _columns;
}

Cluster::Cluster(const Cluster& other)
{
_text = other.GetText();
_columns = other.GetColumns();
}

Cluster& Cluster::operator=(const Cluster& other)
{
_text = other.GetText();
_columns = other.GetColumns();
return *this;
}

void Cluster::SetColumns(size_t column)
{
_columns = column;
}
73 changes: 69 additions & 4 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -582,11 +582,11 @@ void Renderer::_PaintBufferOutput(_In_ IRenderEngine* const pEngine)
// This means that we need 14,27 out of the backing buffer to fill in the 1,1 cell of the screen.
const auto screenLine = Viewport::Offset(bufferLine, -view.Origin());

// Retrieve the cell information iterator limited to just this line we want to redraw.
auto it = buffer.GetCellDataAt(bufferLine.Origin(), bufferLine);
//auto it = buffer.GetCellDataAt(bufferLine.Origin(), bufferLine);
//_PaintBufferOutputHelper(pEngine, it, screenLine.Origin());

// Ask the helper to paint through this specific line.
_PaintBufferOutputHelper(pEngine, it, screenLine.Origin());
const auto& rowData = buffer.GetRowByOffset(row);
Copy link
Collaborator Author

@skyline75489 skyline75489 Nov 5, 2019

Choose a reason for hiding this comment

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

This method _PaintBufferOutput is supposed to handle all kinds of dirty rect refreshing. I kind of made it only working for rows, which is basically all that's needed right now. Should make it better, though.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably what ended up causing the duplicate conpty output - by just re-rendering the entire row at the time, that's causing the text to get duplicated on the Terminal side

_PaintBufferLineOutputHelper(pEngine, rowData, screenLine.Origin());
}
}
}
Expand Down Expand Up @@ -664,6 +664,71 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
}
}

void Renderer::_PaintBufferLineOutputHelper(_In_ IRenderEngine* const pEngine,
const ROW& row,
const COORD target)
{
size_t cols = 0;
const ATTR_ROW& attrRow = row.GetAttrRow();
const std::vector<Cluster> rowClusters = row.GetClusters();

const size_t runCount = attrRow.GetNumberOfRuns();
size_t runIndex = 0;
// And hold the point where we should start drawing.
auto screenPoint = target;

size_t runStart = 0;
// This outer loop will continue until we reach the end of the text we are trying to draw.
while (runIndex < runCount)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where magic happens. We have already processed the text buffer and got the clusters we need. All that left is to walk through the row and render each run.

{
const TextAttributeRun run = attrRow.GetRunByIndex(runIndex);
const TextAttribute attr = run.GetAttributes();
const size_t runLength = run.GetLength();

const auto currentRunColor = attr;

// Update the drawing brushes with our color.
THROW_IF_FAILED(_UpdateDrawingBrushes(pEngine, currentRunColor, false));

// Advance the point by however many columns we've just outputted and reset the accumulator.
screenPoint.X += gsl::narrow<SHORT>(cols);
cols = 0;

size_t count = 0;
std::vector<Cluster> clusters;
clusters.reserve(runLength);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reserve here is intended to reduce vector reallocation and it works great. Without reserve, the performance is heavily penalized.

for (auto it = rowClusters.cbegin() + runStart;
cols < runLength && it != rowClusters.cend(); ++it)
{
auto cluster = (*it);
const auto columnCount = cluster.GetColumns();
cols += columnCount;
count++;

if (cluster.GetColumns() <= 0)
{
continue;
}

clusters.emplace_back(cluster);
}

// Do the painting.
// TODO: Calculate when trim left should be TRUE
THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, false));

// If we're allowed to do grid drawing, draw that now too (since it will be coupled with the color data)
if (_pData->IsGridLineDrawingAllowed())
{
// We're only allowed to draw the grid lines under certain circumstances.
_PaintBufferOutputGridLineHelper(pEngine, currentRunColor, cols, screenPoint);
}

runStart += count;
++runIndex;
}
}

// Method Description:
// - Generates a IRenderEngine::GridLines structure from the values in the
// provided textAttribute
Expand Down
4 changes: 4 additions & 0 deletions src/renderer/base/renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ namespace Microsoft::Console::Render
TextBufferCellIterator it,
const COORD target);

void _PaintBufferLineOutputHelper(_In_ IRenderEngine* const pEngine,
const ROW& row,
const COORD target);

static IRenderEngine::GridLines s_GetGridlines(const TextAttribute& textAttribute) noexcept;

void _PaintBufferOutputGridLineHelper(_In_ IRenderEngine* const pEngine,
Expand Down
12 changes: 10 additions & 2 deletions src/renderer/inc/Cluster.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ namespace Microsoft::Console::Render
class Cluster
{
public:
Cluster();

Cluster(const std::wstring_view text, const size_t columns);

const wchar_t GetTextAsSingle() const noexcept;
Expand All @@ -29,11 +31,17 @@ namespace Microsoft::Console::Render

const size_t GetColumns() const noexcept;

Cluster(const Cluster&);

Cluster& operator=(const Cluster&);

void SetColumns(size_t column);

private:
// This is the UTF-16 string of characters that form a particular drawing cluster
const std::wstring_view _text;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I love the const here but I want to reuse existing _text and update the columns. So I had to make it this way.

std::wstring_view _text;

// This is how many columns we're expecting this cluster to take in the display grid
const size_t _columns;
size_t _columns;
};
}