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

Reduce InsertAttrRuns CPU usage #2937

Merged
merged 1 commit into from
Oct 18, 2019
Merged
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
75 changes: 54 additions & 21 deletions src/buffer/out/AttrRow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -278,31 +278,64 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt
{
return S_OK;
}
// .. otherwise if we internally have a list of 2 and we're about to insert a single color
// it's probable that we're just walking left-to-right through the row and changing each
// cell one at a time.
// e.g.
// AAAAABBBBBBB
// AAAAAABBBBBB
// AAAAAAABBBBB
// Check for that circumstance by seeing if we're inserting a single run of the
// left side color right at the boundary and just adjust the counts in the existing
// two elements in our internal list.
else if (_list.size() == 2 && newAttrs.at(0).GetLength() == 1)
// .. otherwise if we internally have a list of 2 or more and we're about to insert a single color
// it's possible that we just walk left-to-right through the row and find a quick exit.
else if (iStart > 0 && iStart == iEnd)
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 condition here is, I think, the same as the previous one. But the performance is a lot better because we don't have to access those vector things. TBH I'm surprised by the slowness of vector, even though all you need is just the size().

@miniksa Is there anything that could possibly be broken by the change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we stick to the old way, we would be using (_list.size() >= 2 && newAttrs.at(0).GetLength() == 1), just to be clear.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as long as it works. I can't think of anything offhand that would affect the behavior here, so as long as the tests pass and you're satisfied with it... we'll take it and see what happens.

{
const auto left = _list.begin();
if (iStart == left->GetLength() && NewAttr == left->GetAttributes())
// First we try to find the run where the insertion happens, using lowerBound and upperBound to track
// where we are curretly at.
size_t lowerBound = 0;
size_t upperBound = 0;
for (size_t i = 0; i < _list.size(); i++)
{
const auto right = left + 1;
left->IncrementLength();
right->DecrementLength();

// If we just reduced the right half to zero, just erase it out of the list.
if (right->GetLength() == 0)
upperBound += _list.at(i).GetLength();
if (iStart >= lowerBound && iStart < upperBound)
{
_list.erase(right);
const auto curr = std::next(_list.begin(), i);

// The run that we try to insert into has the same color as the new one.
// e.g.
// AAAAABBBBBBBCCC
// ^
// AAAAABBBBBBBCCC
//
// 'B' is the new color and '^' represents where iStart is. We don't have to
// do anything.
if (curr->GetAttributes() == NewAttr)
{
return S_OK;
}

// If the insertion happens at current run's lower boundary...
if (iStart == lowerBound)
{
const auto prev = std::prev(curr, 1);
// ... and the previous run has the same color as the new one, we can
// just adjust the counts in the existing two elements in our internal list.
// e.g.
// AAAAABBBBBBBCCC
// ^
// AAAAAABBBBBBCCC
//
// Here 'A' is the new color.
if (NewAttr == prev->GetAttributes())
{
prev->IncrementLength();
curr->DecrementLength();

// If we just reduced the right half to zero, just erase it out of the list.
if (curr->GetLength() == 0)
{
_list.erase(curr);
}

return S_OK;
}
}
}
return S_OK;

// Advance one run in the _list.
lowerBound = upperBound;
}
}
}
Expand Down