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

Stop committing the whole buffer when determining if it's empty #15582

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Stop committing the whole buffer when determining if it's empty #15582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7fb2e9
Avoid committing the entire buffer when figuring out if it's empty
DHowett Jun 20, 2023
60b6338
ooga booga
DHowett Jun 20, 2023
adabfd5
Leonard for the save
DHowett Jun 20, 2023
07e33d0
Deduplicate redundant offset subtraction
lhecker Jun 21, 2023
a8201c6
Avoid committing empty buffers on reflow
lhecker Jun 21, 2023
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
17 changes: 15 additions & 2 deletions src/buffer/out/textBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@ ROW& TextBuffer::_getRowByOffsetDirect(size_t offset)
return *reinterpret_cast<ROW*>(row);
}

// Returns the "user-visible" index of the last committed row, which can be used
// to short-circuit some algorithms that try to scan the entire buffer.
// Returns 0 if no rows are committed in.
til::CoordType TextBuffer::_estimateOffsetOfLastCommittedRow() const noexcept
{
const auto lastRowOffset = (_commitWatermark - _buffer.get()) / _bufferRowStride;
// This subtracts 2 from the offset to account for the:
// * scratchpad row at offset 0, whereas regular rows start at offset 1.
// * fact that _commitWatermark points _past_ the last committed row,
// but we want to return an index pointing at the last row.
return std::max(0, gsl::narrow_cast<til::CoordType>(lastRowOffset - 2));
Copy link
Member

Choose a reason for hiding this comment

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

I realized that the

const auto y = (_commitWatermark - _buffer.get() - 1) / _bufferRowStride;

calculation is effectively the same as

const auto x = (_commitWatermark - _buffer.get()) / _bufferRowStride;
const auto y = x - 1;

and so it kind of merges with the other -1 we already had to form -2.
I also felt like it's neat because now it's only 2 lines of code and the std::max ensures it's impossible to return negative indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmmm I agree with you, but I probably should have been the one to commit the suggestion ;)

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I thought this would take some load off your shoulders.

}

// Retrieves a row from the buffer by its offset from the first row of the text buffer
// (what corresponds to the top row of the screen buffer).
const ROW& TextBuffer::GetRowByOffset(const til::CoordType index) const
Expand Down Expand Up @@ -807,7 +820,7 @@ til::point TextBuffer::GetLastNonSpaceCharacter(std::optional<const Microsoft::C

til::point coordEndOfText;
// Search the given viewport by starting at the bottom.
coordEndOfText.y = viewport.BottomInclusive();
coordEndOfText.y = std::min(viewport.BottomInclusive(), _estimateOffsetOfLastCommittedRow());

const auto& currRow = GetRowByOffset(coordEndOfText.y);
// The X position of the end of the valid text is the Right draw boundary (which is one beyond the final valid character)
Expand Down Expand Up @@ -2622,7 +2635,7 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
// printable char gets reset. See GH #12567
auto newRowY = newCursor.GetPosition().y + 1;
const auto newHeight = newBuffer.GetSize().Height();
const auto oldHeight = oldBuffer.GetSize().Height();
const auto oldHeight = oldBuffer._estimateOffsetOfLastCommittedRow() + 1;
Copy link
Member

Choose a reason for hiding this comment

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

I added this, because it's a simple change, but speeds up reflow of empty buffers quite a lot. It needs +1 because _estimateOffsetOfLastCommittedRow returns the offset of the last row and not the height (= past the end) of the buffer.

for (;
iOldRow < oldHeight && newRowY < newHeight;
iOldRow++)
Expand Down
1 change: 1 addition & 0 deletions src/buffer/out/textBuffer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ class TextBuffer final
void _construct(const std::byte* until) noexcept;
void _destroy() const noexcept;
ROW& _getRowByOffsetDirect(size_t offset);
til::CoordType _estimateOffsetOfLastCommittedRow() const noexcept;

void _SetFirstRowIndex(const til::CoordType FirstRowIndex) noexcept;
til::point _GetPreviousFromCursor() const;
Expand Down