Stop committing the whole buffer when determining if it's empty #15582
Conversation
As a shortcut, GetLastNonSpaceCharacter can start with the last committed row. It's guaranteed that there isn't anything of worth below that point, so why bother checking? Without this, Terminal immediately commits the entire 9031-line buffer on startup while trying to -- get this -- clear the screen!
src/buffer/out/textBuffer.cpp
Outdated
| const auto lastRowOffset = static_cast<ptrdiff_t>((_commitWatermark - 1) - _buffer.get()) / _bufferRowStride; | ||
| return gsl::narrow_cast<size_t>(lastRowOffset); |
There was a problem hiding this comment.
i almost certainly blew up the x86 or audit builds and just haven't bothered to run locally. keep in mind.
| // * 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmmmmm I agree with you, but I probably should have been the one to commit the suggestion ;)
There was a problem hiding this comment.
Ah sorry, I thought this would take some load off your shoulders.
| @@ -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; | |||
There was a problem hiding this comment.
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.
DHowett
changed the title
…osoft#15582) As a shortcut, GetLastNonSpaceCharacter can start with the last committed row. It's guaranteed that there isn't anything of worth below that point, so why bother checking? Without this, Terminal immediately commits the entire 9031-line buffer on startup while trying to--get this!--clear the screen! --------- Co-authored-by: Leonard Hecker <lhecker@microsoft.com>
As a shortcut, GetLastNonSpaceCharacter can start with the last committed row. It's guaranteed that there isn't anything of worth below that point, so why bother checking?
Without this, Terminal immediately commits the entire 9031-line buffer on startup while trying to--get this!--clear the screen!