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

Reimplement TextBuffer::Reflow #15701

Merged
merged 16 commits into from
Sep 26, 2023
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 12, 2023

Subjectively speaking, this commit makes 3 improvements:

  • Most importantly, it now would work with arbitrary Unicode text.
    (No more IsGlyphFullWidth or DBCS handling during reflow.)
  • Due to the simpler implementation it hopefully makes review of
    future changes and maintenance simpler. (~3x less LOC.)
  • It improves perf. by 1-2 orders of magnitude.
    (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

Validation Steps Performed

  • Unit tests ✅
  • Feature tests ✅
  • Reflow with a scrollback ✅
  • Reflowing the cursor cell causes a forced line-wrap ✅
    (Even at the end of the buffer. ✅)
  • color 8f and reflowing retains the background color ✅
  • Enter alt buffer, Resize window, Exit alt buffer ✅

@lhecker lhecker force-pushed the dev/lhecker/8000-TextBuffer-reflow branch from a009e0d to 3d463f1 Compare August 15, 2023 14:00
@lhecker lhecker marked this pull request as ready for review August 15, 2023 14:00
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Performance Performance-related issue Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. labels Aug 15, 2023
@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2023

I fixed all the bugs that were obvious enough that I could find them. It'd be super helpful if anyone else could give this PR/branch a look. But it might be easiest to just put it into our test builds.

};
CopyTextFrom(state);

TransferAttributes(source.Attributes(), _columnCount);
Copy link
Member Author

Choose a reason for hiding this comment

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

This change ensures that we take the line rendition into account when determining the capacity of the row in CopyTextFrom.

@@ -284,7 +289,17 @@ til::CoordType ROW::NavigateToPrevious(til::CoordType column) const noexcept
// Returns the row width if column is beyond the width of the row.
til::CoordType ROW::NavigateToNext(til::CoordType column) const noexcept
{
return _adjustForward(_clampedColumn(column + 1));
return _adjustForward(_clampedColumnInclusive(column + 1));
Copy link
Member Author

Choose a reason for hiding this comment

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

This will allow us to "navigate" the cursor into the past-the-end column.

// We _are_ using span. But C++ decided that string_view and span aren't convertible.
// _chars is a std::span for performance and because it refers to raw, shared memory.
#pragma warning(suppress : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1).
chars = { source._chars.data() + charsOffset, source._chars.size() - charsOffset };
chars = { source._chars.data() + beg, end - beg };
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a h.charsConsumed == chars.size() check at the end of this function. That check only works if the chars.size() is correct, which this change now fixes. The string will now contain the exact amount of text we're allowed to copy.

@@ -159,7 +159,7 @@ class TextBuffer final
// Scroll needs access to this to quickly rotate around the buffer.
void IncrementCircularBuffer(const TextAttribute& fillAttributes = {});

til::point GetLastNonSpaceCharacter(std::optional<const Microsoft::Console::Types::Viewport> viewOptional = std::nullopt) const;
til::point GetLastNonSpaceCharacter(const Microsoft::Console::Types::Viewport* viewOptional = nullptr) const;
Copy link
Member Author

Choose a reason for hiding this comment

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

While I like std::optional, we shouldn't optimally pass Viewport by value. Also, this pointer-style fits much better with Reflow() which has 2 optional parameters one of which is an explicit optional reference and why write

std::optional<std::reference_wrapper<PositionInformation>>

if you can also

PositionInformation*

Copy link
Member

Choose a reason for hiding this comment

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

lmao

_altBufferSize = viewportSize;
_altBuffer->TriggerRedrawAll();
Copy link
Member Author

Choose a reason for hiding this comment

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

This was missing? I think that's a bug.

Copy link
Member

Choose a reason for hiding this comment

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

i'd believe it.

Comment on lines -245 to -250
_altBuffer->GetCursor().StartDeferDrawing();
// we're capturing `this` here because when we exit, we want to EndDefer on the (newly created) active buffer.
auto endDefer = wil::scope_exit([this]() noexcept { _altBuffer->GetCursor().EndDeferDrawing(); });
Copy link
Member Author

Choose a reason for hiding this comment

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

There'll be no need for DeferDrawing anymore, since the reflow/resize functions don't use the cursor.

}

int Terminal::_VisibleEndIndex() const noexcept
{
return _inAltBuffer() ? ViewEndIndex() :
std::max(0, ViewEndIndex() - _scrollOffset);
return _inAltBuffer() ? _altBufferSize.height - 1 : std::max(0, _mutableViewport.BottomInclusive() - _scrollOffset);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I should revert these changes... I felt like the extra _inAltBuffer() check in the nested calls is redundant and inlining the code would make it clearer, but realistically this doesn't make any difference, right? Idk...

Copy link
Member

Choose a reason for hiding this comment

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

jeez, yeah, i don't get why these were the way they are. go for it.

const auto lastNonSpaceRow = newTextBuffer->GetLastNonSpaceCharacter().y;
const auto estimatedBottom = cursorRow + cursorDistanceFromBottom;
const auto viewportBottom = _viewport.Height() - 1;
_virtualBottom = std::max({ lastNonSpaceRow, estimatedBottom, viewportBottom });
Copy link
Member Author

Choose a reason for hiding this comment

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

(Indentation changes only. -> Suppress whitespace changes.)

const auto cursorRow = GetCursor().GetPosition().y;
const auto copyableRows = std::min<til::CoordType>(_height, newSize.height);
til::CoordType srcRow = 0;
til::CoordType dstRow = 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

(Indentation changes only. -> Suppress whitespace changes.)

newRow.SetLineRendition(row.GetLineRendition());
}
const auto oldCursorPos = oldCursor.GetPosition();
til::point newCursorPos;
Copy link
Member Author

Choose a reason for hiding this comment

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

This function may be easier to review without a diff.

src/cascadia/TerminalCore/Terminal.cpp Outdated Show resolved Hide resolved
}

int Terminal::_VisibleEndIndex() const noexcept
{
return _inAltBuffer() ? ViewEndIndex() :
std::max(0, ViewEndIndex() - _scrollOffset);
return _inAltBuffer() ? _altBufferSize.height - 1 : std::max(0, _mutableViewport.BottomInclusive() - _scrollOffset);
Copy link
Member

Choose a reason for hiding this comment

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

jeez, yeah, i don't get why these were the way they are. go for it.

src/host/screenInfo.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Aug 24, 2023

Unfortunately, I'm not confident that the new code replicates the old code exactly,

Well, it sure doesn't! All the reflow tests are failing :D

Some of them have been marked buggy, but I tried to capture as much existing behavior as i could when i wrote them.

@lhecker
Copy link
Member Author

lhecker commented Aug 25, 2023

Well, it sure doesn't! All the reflow tests are failing :D

I thought that this means all is good:
image

I didn't consider that...
image

...they might just not work in VS.

Comment on lines -1518 to -1527
if (gci.IsInVtIoMode())
{
gci.GetVtIo()->BeginResize();
}
auto endResize = wil::scope_exit([&] {
if (gci.IsInVtIoMode())
{
gci.GetVtIo()->EndResize();
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

While working on another PR I realized that this special behavior is only here for TextBuffer::Reflow, because that one used to use NewlineCursor which calls IncrementCircularBuffer which calls Renderer::TriggerFlush which calls VtEngine::InvalidateFlush which then set pForcePaint = false if BeginResize was called.

In short: This block prevents TextBuffer::Reflow from setting pForcePaint to true.

...and the new code doesn't call IncrementCircularBuffer anymore so that's moot.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

(just trying to post a comment I think GH lost)

Comment on lines 295 to 303
til::CoordType ROW::AdjustBackward(til::CoordType column) const noexcept
{
return _adjustBackward(_clampedColumn(column));
}

til::CoordType ROW::AdjustForward(til::CoordType column) const noexcept
{
return _adjustForward(_clampedColumnInclusive(column));
}
Copy link
Member

Choose a reason for hiding this comment

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

These might warrant a comment as to why they're different than the Navigate* versions. If I'm reading this right:

  • the Navigate ones act like "move one cell forward/backward, then find the end/start of the glyph in that cell"
  • while the Adjust ones are just "find the end/start of the glyph in THIS cell"

correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

I've removed AdjustForward (it's unused) and renamed AdjustBackward to AdjustToGlyphStart and added a comment. That should hopefully make it easier to read the code.

@@ -867,6 +883,16 @@ til::CoordType ROW::MeasureLeft() const noexcept

til::CoordType ROW::MeasureRight() const noexcept
{
if (_wrapForced)
Copy link
Member

Choose a reason for hiding this comment

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

brilliant

Comment on lines 2551 to 2557
if (oldY == oldCursorPos.y && oldCursorPos.x >= oldX)
{
cNewCursorPos = newCursor.GetPosition();
fFoundCursorPos = true;
// In theory AdjustBackward ensures we don't put the cursor on a trailing wide glyph.
// In practice I don't think that this can possibly happen. Better safe than sorry.
newCursorPos = { newRow.AdjustBackward(oldCursorPos.x - oldX + newX), newY };
newYLimit = newY + newHeight;
}
Copy link
Member

Choose a reason for hiding this comment

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

if the old cursor was on this row, and the old cursor was beyond this character,

  • the new cursor is on this row, adjusted for how many glyphs are in this new row
  • don't keep copying beyond this new row + the height of the new buffer

that feels weird? I don't think I understand the use of `newYLimit`` here

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be sure:

if the old cursor was on this row, and the old cursor was beyond this character the start of the text we've copied

...because this implies that we've probably just copied the text where the old cursor was on. I've written the if condition that way because it then matches the oldCursorPos.x - oldX. It allows you (kind of) to see how oldCursorPos.x >= oldX ensures that oldCursorPos.x - oldX doesn't go negative and how this results in the new cursor always being at some X position that is >= 0.


that feels weird? I don't think I understand the use of newYLimit here

Imagine that the terminal has no scrollback (= TextBuffer is as tall as the viewport). If we make the window significantly less taller (for instance 1/3rd), we need to ensure we don't continue copying so much text that it overwrites the row the cursor was on. In other words, newYLimit ensures that the row with the cursor remains visible.

I've added that as a comment to this line.

Copy link
Member

Choose a reason for hiding this comment

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

what if like,

  • the buffer starts 30 tall
  • the cursor is on line 5
  • we resize to 20 tall

Does that mean we'd only end up copying the first 25 rows? (newYLimit = 5 + 20)?

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 the last question I have before I s/o)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly that. I did it for this test specifically:

TestBuffer{
{ 6, 5 },
{
{ L"ABCDEF", false },
{ L"$ ", false },
{ L"GHIJKL", false },
{ L"MNOPQR", false },
{ L"STUVWX", false },
},
{ 0, 1 } // cursor on $
},
TestBuffer{
{ 5, 5 }, // reduce width by 1
{
{ L"F$ ", false },
{ L"GHIJK", true }, // [BUG] We should see GHIJK\n L\n MNOPQ\n R\n
{ L"LMNOP", true }, // The wrapping here is irregular
{ L"QRSTU", true },
{ L"VWX ", false },
},
{ 1, 1 } // [BUG] cursor moves to 1,1 instead of sticking with the $
},

$ is the cursor. false in each line means that it has an explicit newline (true = force wrap).
It has a lot of comments about bugs in the old implementation, which is why the entire text past the cursor fits into the new buffer. In the new, fixed implementation the false = explicit newlines are correctly handled and so it doesn't fit anymore:

TestBuffer{
{ 6, 5 },
{
{ L"ABCDEF", false },
{ L"$ ", false },
{ L"GHIJKL", false },
{ L"MNOPQR", false },
{ L"STUVWX", false },
},
{ 0, 1 }, // cursor on $
},
TestBuffer{
{ 5, 5 }, // reduce width by 1
{
{ L"$ ", false },
{ L"GHIJK", true },
{ L"L ", false },
{ L"MNOPQ", true },
{ L"R ", false },
},
{ 0, 0 },
},

Basically I had to make a choice over whether I:

  • continue copying until all text in the old buffer has been consumed.
    If the new cursor position is now somewhere at a negative Y, we just put it at (0,0).
  • stop copying at some point to prevent the cursor from being lost.

I chose the latter. What do you think we should do?

Copy link
Member

Choose a reason for hiding this comment

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

continue copying until all text in the old buffer has been consumed.
If the new cursor position is now somewhere at a negative Y, we just put it at (0,0).

Probably that one, right? Like, the priority should be to keep the newest text, right?

(/cc @DHowett since this is currently a 1.19 candidate. I'm okay to say merge as is and iterate in the first hotfix since

  1. we're out of time and I'm about to log out for the day
  2. this is a RARE edge case
  3. this is more goodness than badness)

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 an interesting case. I'm honestly not sure how to handle it -- we should look at how other terminals that do reflow handle it?

@zadjii-msft
Copy link
Member

I think I'm inclined to s/o on this, though I really don't think I get the newYLimit thing.

Though, there are a TON of reflow roundtrip tests, so if those still pass, then I'm inclined to say this works.

@github-actions

This comment has been minimized.

DHowett added a commit that referenced this pull request Sep 22, 2023
@DHowett
Copy link
Member

DHowett commented Sep 26, 2023

I'm okay to say merge as is and iterate in the first hotfix since

I am taking this as a sign-off for 1.19, plus fixes.

Mike, thanks for reviewing. Sorry if I override you and am incorrect. 😄

@DHowett DHowett merged commit 7474839 into main Sep 26, 2023
16 checks passed
@DHowett DHowett deleted the dev/lhecker/8000-TextBuffer-reflow branch September 26, 2023 00:28
DHowett pushed a commit that referenced this pull request Sep 26, 2023
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅

(cherry picked from commit 7474839)
Service-Card-Id: 90642727
Service-Version: 1.19
DHowett pushed a commit that referenced this pull request Jan 29, 2024
#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect
the `ROW::_wrapForced` flag anymore. This change in behavior was not
noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where
rows of only whitespace text would always be treated as empty.
This would then affect `AdaptDispatch::_EraseAll` to accidentally
correctly guess the last row with text despite the `_FillRect` change.

#15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing
`ROW::MeasureRight` which now made the previous change apparent.
`_EraseAll` would now guess the last row of text incorrectly,
because it would find the rows that `_FillRect` cleared but still
had `_wrapForced` set to `true`.

This PR fixes the issue by replacing the `_FillRect` usage to clear
rows with direct calls to `ROW::Reset()`. In the future this could be
extended by also `MEM_DECOMMIT`ing the now unused underlying memory.

Closes #16603

## Validation Steps Performed
* Enter WSL and resize the window to <40 columns
* Execute
  ```sh
  cd /bin
  ls -la
  printf "\e[3J"
  ls -la
  printf "\e[3J"
  printf "\e[2J"
  ```
* Only one viewport-height-many lines of whitespace exist between the
  current prompt line and the previous scrollback contents ✅
DHowett pushed a commit that referenced this pull request Jan 29, 2024
#15541 changed `AdaptDispatch::_FillRect` which caused it to not affect
the `ROW::_wrapForced` flag anymore. This change in behavior was not
noticeable as `TextBuffer::GetLastNonSpaceCharacter` had a bug where
rows of only whitespace text would always be treated as empty.
This would then affect `AdaptDispatch::_EraseAll` to accidentally
correctly guess the last row with text despite the `_FillRect` change.

#15701 then fixed `GetLastNonSpaceCharacter` indirectly by fixing
`ROW::MeasureRight` which now made the previous change apparent.
`_EraseAll` would now guess the last row of text incorrectly,
because it would find the rows that `_FillRect` cleared but still
had `_wrapForced` set to `true`.

This PR fixes the issue by replacing the `_FillRect` usage to clear
rows with direct calls to `ROW::Reset()`. In the future this could be
extended by also `MEM_DECOMMIT`ing the now unused underlying memory.

Closes #16603

## Validation Steps Performed
* Enter WSL and resize the window to <40 columns
* Execute
  ```sh
  cd /bin
  ls -la
  printf "\e[3J"
  ls -la
  printf "\e[3J"
  printf "\e[2J"
  ```
* Only one viewport-height-many lines of whitespace exist between the
  current prompt line and the previous scrollback contents ✅

(cherry picked from commit 5f71cf3)
Service-Card-Id: 91707937
Service-Version: 1.19
github-merge-queue bot pushed a commit that referenced this pull request Jul 12, 2024
STL iterators have a significant overhead. This improves performance
of `GetLastNonSpaceColumn` by >100x (it's too large to measure),
and reflow by ~15x in debug builds. This makes text reflow in debug
builds today ~10x faster than it used to be in release builds before
the large rewrites in #15701 and #13626.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Performance Performance-related issue Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
3 participants