Skip to content

Commit

Permalink
Optimize hot path in textBufferCellIterator (#10621)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

The `+=` operator is an extremely hot path under heavily output load. This PR aims to optimize its speed.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Supports #10563
* [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
  • Loading branch information
skyline75489 authored and DHowett committed Aug 25, 2021
1 parent ccf87cd commit 5235d2d
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 10 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,7 @@ IAction
IApi
IApplication
IBase
ICache
icacls
iccex
icch
Expand Down
17 changes: 16 additions & 1 deletion src/buffer/out/OutputCellView.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Module Name:
- OutputCellView.hpp
Abstract:
- Read-only view into a single cell of data that someone is attempting to write into the output buffer.
- Read view into a single cell of data that someone is attempting to write into the output buffer.
- This is done for performance reasons (avoid heap allocs and copies).
Author:
Expand Down Expand Up @@ -36,6 +36,21 @@ class OutputCellView
TextAttribute TextAttr() const noexcept;
TextAttributeBehavior TextAttrBehavior() const noexcept;

void UpdateText(const std::wstring_view& view) noexcept
{
_view = view;
};

void UpdateDbcsAttribute(const DbcsAttribute& dbcsAttr) noexcept
{
_dbcsAttr = dbcsAttr;
}

void UpdateTextAttribute(const TextAttribute& textAttr) noexcept
{
_textAttr = textAttr;
}

bool operator==(const OutputCellView& view) const noexcept;
bool operator!=(const OutputCellView& view) const noexcept;

Expand Down
106 changes: 97 additions & 9 deletions src/buffer/out/textBufferCellIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,20 +94,93 @@ bool TextBufferCellIterator::operator!=(const TextBufferCellIterator& it) const
// - Reference to self after movement.
TextBufferCellIterator& TextBufferCellIterator::operator+=(const ptrdiff_t& movement)
{
// Note that this method is called intensively when the terminal is under heavy load.
// We need to aggressively optimize it, comparing to the -= operator.
ptrdiff_t move = movement;
auto newPos = _pos;
while (move > 0 && !_exceeded)
if (move < 0)
{
_exceeded = !_bounds.IncrementInBounds(newPos);
// Early branching to leave the rare case to -= operator.
// This helps reducing the instruction count within this method, which is good for instruction cache.
return *this -= -move;
}

// The remaining code in this function is functionally equivalent to:
// auto newPos = _pos;
// while (move > 0 && !_exceeded)
// {
// _exceeded = !_bounds.IncrementInBounds(newPos);
// move--;
// }
// _SetPos(newPos);
//
// _SetPos() necessitates calling _GenerateView() and thus the construction
// of a new OutputCellView(). This has a high performance impact (ICache spill?).
// The code below inlines _bounds.IncrementInBounds as well as SetPos.
// In the hot path (_pos.Y doesn't change) we modify the _view directly.

// Hoist these integers which will be used frequently later.
const auto boundsRightInclusive = _bounds.RightInclusive();
const auto boundsLeft = _bounds.Left();
const auto boundsBottomInclusive = _bounds.BottomInclusive();
const auto boundsTop = _bounds.Top();
const auto oldX = _pos.X;
const auto oldY = _pos.Y;

// Under MSVC writing the individual members of a COORD generates worse assembly
// compared to having them be local variables. This causes a performance impact.
auto newX = oldX;
auto newY = oldY;

while (move > 0)
{
if (newX == boundsRightInclusive)
{
newX = boundsLeft;
newY++;
if (newY > boundsBottomInclusive)
{
newY = boundsTop;
_exceeded = true;
break;
}
}
else
{
newX++;
_exceeded = false;
}
move--;
}
while (move < 0 && !_exceeded)

if (_exceeded)
{
_exceeded = !_bounds.DecrementInBounds(newPos);
move++;
// Early return because nothing needs to be done here.
return *this;
}
_SetPos(newPos);
return (*this);

if (newY == oldY)
{
// hot path
const auto diff = gsl::narrow_cast<ptrdiff_t>(newX) - gsl::narrow_cast<ptrdiff_t>(oldX);
_attrIter += diff;
_view.UpdateTextAttribute(*_attrIter);

const CharRow& charRow = _pRow->GetCharRow();
_view.UpdateText(charRow.GlyphAt(newX));
_view.UpdateDbcsAttribute(charRow.DbcsAttrAt(newX));
_pos.X = newX;
}
else
{
// cold path (_GenerateView is slow)
_pRow = s_GetRow(_buffer, { newX, newY });
_attrIter = _pRow->GetAttrRow().cbegin() + newX;
_pos.X = newX;
_pos.Y = newY;
_GenerateView();
}

return *this;
}

// Routine Description:
Expand All @@ -118,7 +191,22 @@ TextBufferCellIterator& TextBufferCellIterator::operator+=(const ptrdiff_t& move
// - Reference to self after movement.
TextBufferCellIterator& TextBufferCellIterator::operator-=(const ptrdiff_t& movement)
{
return this->operator+=(-movement);
ptrdiff_t move = movement;
if (move < 0)
{
return (*this) += (-move);
}

auto newPos = _pos;
while (move > 0 && !_exceeded)
{
_exceeded = !_bounds.DecrementInBounds(newPos);
move--;
}
_SetPos(newPos);

_GenerateView();
return (*this);
}

// Routine Description:
Expand Down

0 comments on commit 5235d2d

Please sign in to comment.