Move ConPTY to use til::bitmap by miniksa · Pull Request #5024 · microsoft/terminal

miniksa · 2020-03-19T22:20:04Z

Summary of the Pull Request

Moves the ConPTY drawing mechanism (VtRenderer) to use the fine-grained til::bitmap individual-dirty-bit tracking mechanism instead of coarse-grained rectangle unions to improve drawing performance by dramatically reducing the total area redrawn.

PR Checklist

Part of Restore differential drawing to DirectWrite renderer #778 and Terminal output not speed enough #1064
I work here
Tests added and updated.
I'm a core contributor

Detailed Description of the Pull Request / Additional comments

Converted GetDirtyArea() interface from IRenderEngine to use a vector of til::rectangle instead of the SMALL_RECT to banhammer inclusive rectangles.
VtEngine now holds and operates on the til::bitmap for invalidation regions. All invalidation operation functions that used to be embedded inside VtEngine are deleted in favor of using the ones in til::bitmap.
Updated VtEngine tracing to use new til::bitmap on trace and the new to_string() methods detailed below.
Comparison operators for til::bitmap and complementary tests.
Fixed an issue where the dirty rectangle shortcut in til::bitmap was set to 0,0,0,0 by default which means that |= on it with each set() operation was stretching the rectangle from 0,0. Now it's a std::optional so it has no value after just being cleared and will build from whatever the first invalidated rectangle is. Complementary tests added.
Optional run caching for til::bitmap in the runs() method since both VT and DX renderers will likely want to generate the set of runs at the beginning of a frame and refer to them over and over through that frame. Saves the iteration and creation and caches inside til::bitmap where the chance of invalidation of the underlying data is known best. It is still possible to iterate manually with begin() and end() from the outside without caching, if desired. Complementary tests added.
WEX templates added for til::bitmap and used in tests.
translate() method for til::bitmap which will slide the dirty points in the direction specified by a til::point and optionally back-fill the uncovered area as dirty. Complementary tests added.
Moves all string generation for til types size, point, rectangle, and some into a to_string method on each object such that it can be used in both ETW tracing scenarios AND in the TAEF templates uniformly. Adds a similar method for bitmap.
Add tagging to _bitmap_const_iterator such that it appears as a valid Input Iterator to STL collections and can be used in a std::vector constructor as a range. Adds and cleans up operators on this iterator to match the theoretical requirements for an Input Iterator. Complementary tests added.
Add loose operators to til which will allow some basic math operations (+, -, *, /) between til::size and til::point and vice versa. Complementary tests added. Complementary tests added.
Adds operators to til::rectangle to allow scaling with basic math operations (+, -, *) versus til::size and translation with basic math operations (+, -) against til::point. Complementary tests added.
In-place variants of some operations added to assorted til objects. Complementary tests added.
Update VT tests to compare invalidation against the new map structure instead of raw rectangles where possible.

Validation Steps Performed

Wrote additional til Unit Tests for all additional operators and functions added to the project to support this operation
Updated the existing VT renderer tests
Ran perf check

…p tests.

…so add associated tests.

…support method for checking if rectangle contains another, add test for that too.

…or iterator. Fix unsigned issues for x86.

…hods of all/any/none. also add convenience one() method in prediction of what conpty will be looking for.

…ve it to details namespace.

…e a bitmap already filled. Generalize the bit checking method in the tests.

…it fits nicely into vector emplace, start using bitmap in ConPTY (Vt renderer).

… printing. Update VT tracing to use the same strings. Roll out bitmap through most of VT.

…s and sizes, translation operator on the bitmap. A ton of tests for all that behavior. Replacing the rest of the VT renderer stuff with the invalidation map.

…idation, run codeformat pass.

miniksa · 2020-03-19T22:43:05Z

Summary of Non-passing Tests:
    Microsoft::Console::Render::VtRendererTest::Xterm256TestInvalidate [Failed]
    Microsoft::Console::Render::VtRendererTest::XtermTestInvalidate [Failed]
    ConptyOutputTests::SimpleWriteOutputTest [Failed]
    ConptyOutputTests::WriteTwoLinesUsesNewline [Failed]
    ConptyOutputTests::WriteAFewSimpleLines [Failed]
    ConptyOutputTests::InvalidateUntilOneBeforeEnd [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::SimpleWriteOutputTest [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::WriteTwoLinesUsesNewline [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::WriteAFewSimpleLines [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::PassthroughClearScrollback [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::PassthroughHardReset [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestWrappingALongString [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestAdvancedWrapping [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestExactWrappingWithoutSpaces [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestExactWrappingWithSpaces [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::MoveCursorAtEOL [Failed]

miniksa · 2020-03-19T23:02:32Z

Resolved by VtEngine::Invalidate correction of inclusive/exclusive mismatch (a.k.a. the always problem):

    ConptyOutputTests::SimpleWriteOutputTest [Failed]
    ConptyOutputTests::WriteTwoLinesUsesNewline [Failed]
    ConptyOutputTests::InvalidateUntilOneBeforeEnd [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::SimpleWriteOutputTest [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::WriteTwoLinesUsesNewline [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::PassthroughClearScrollback [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::PassthroughHardReset [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestWrappingALongString [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestExactWrappingWithoutSpaces [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestExactWrappingWithSpaces [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::MoveCursorAtEOL [Failed]

and introduced

    Microsoft::Console::Render::VtRendererTest::WinTelnetTestInvalidate [Failed]

for remaining set

    Microsoft::Console::Render::VtRendererTest::Xterm256TestInvalidate [Failed]
    Microsoft::Console::Render::VtRendererTest::XtermTestInvalidate [Failed]
    Microsoft::Console::Render::VtRendererTest::WinTelnetTestInvalidate [Failed]
    ConptyOutputTests::WriteAFewSimpleLines [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::WriteAFewSimpleLines [Failed]
    TerminalCoreUnitTests::ConptyRoundtripTests::TestAdvancedWrapping [Failed]

miniksa · 2020-03-19T23:11:51Z

A choice snippet from ConptyOutputTests::WriteAFewSimpleLines():

    // Here, we're going to emit 3 spaces. The region that got invalidated was a
    // rectangle from 0,0 to 3,3, so the vt renderer will try to render the
    // region in between BBB and CCC as well, because it got included in the
    // rectangle Or() operation.
    // This behavior should not be seen as binding - if a future optimization
    // breaks this test, it wouldn't be the worst.

Yep, that's what I broke/fixed.

miniksa · 2020-03-19T23:22:10Z

A choice snippet from ConptyOutputTests::WriteAFewSimpleLines():

    // Here, we're going to emit 3 spaces. The region that got invalidated was a
    // rectangle from 0,0 to 3,3, so the vt renderer will try to render the
    // region in between BBB and CCC as well, because it got included in the
    // rectangle Or() operation.
    // This behavior should not be seen as binding - if a future optimization
    // breaks this test, it wouldn't be the worst.

Yep, that's what I broke/fixed.

Updating the code as follows, fixes this one and the ConptyRoundtripTests copy:

    expectedOutput.push_back("AAA");
    expectedOutput.push_back("\r\n");
    expectedOutput.push_back("BBB");
    // Jump down to the fourth line because emitting spaces didn't do anything
    // and we will skip to emitting the CCC segment.
    expectedOutput.push_back("\x1b[4;1H");
    expectedOutput.push_back("CCC");

    // Cursor goes back on.
    expectedOutput.push_back("\x1b[?25h");

@DHowett-MSFT, @zadjii-msft, the only thing I didn't expect was the cursor to go back on. It wasn't there before in this test. Is it expected for the cursor to go back on when drawing is done? Under what circumstances should it not do that?

miniksa · 2020-03-19T23:34:29Z

This one TerminalCoreUnitTests::ConptyRoundtripTests::TestAdvancedWrapping [Failed]:

It was looking for

    // Clear the rest of row 1
    expectedOutput.push_back("\x1b[K");

and

    // and clear everything after the text, because the buffer is empty.
    expectedOutput.push_back("\x1b[K");

Which no longer happen because the invalid region isn't intersecting rectangles anymore.

…les (inclusive/exclusive bug.) Fix one of the tests to not be comparing to internal details based on a 0-size exclusive circumstance that only worked with or-rect. Fix importing of til for unit tests in the library includes so the WEX/TAEF templates show up appropriately (cascaded to changing renderervt.unittest lib compilation to use shared testing props so it works too.)

miniksa · 2020-03-20T16:39:09Z

Tests all fixed. Still some more inclusive/exclusive juggling (which should be changed, probably in the next PR for DX, to use til objects in the IRenderEngine interfaces). Some changed assumptions about scrolling lines up and down. And fixing the import of the TIL files in unit tests so the templates show up in all the projects now (hopefully.)

Setting this for real review. I will reply with the perf screenshots in a bit.

zadjii-msft

Yea I don't think I have anything to block here. Lemme see those perf screenshots :)

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

src/inc/til/rectangle.h

src/renderer/vt/XtermEngine.cpp

src/til/ut_til/til.unit.tests.vcxproj

DHowett-MSFT

Love it. Seems totally cromulent to me

src/inc/til/bitmap.h

src/inc/til/operators.h

src/inc/til/rectangle.h

src/renderer/base/renderer.cpp

miniksa · 2020-03-20T20:37:34Z

Old: https://thumbs.gfycat.com/FrigidVictoriousIcelandichorse-mobile.mp4
New: https://thumbs.gfycat.com/PowerlessOldfashionedAmericangoldfinch-mobile.mp4

Not super dramatic anymore now that #4877 is merged...

I'll follow up with some WPRs to try to confirm that it's doing less.

I also expect we'll need to land the DX side to really see more smoothness here.

miniksa · 2020-03-20T21:01:13Z

Before:

After:

miniksa · 2020-03-20T21:09:05Z

@DHowett-MSFT had a follow up question about start/endpaint.

After this change, more time is spent on StartPaint to detect the runs out of the bitmap. But it seems worth it for the significant _PaintBufferOutput savings.

Before:

After:

…ments. Taking out the MUL for scaling since it's unused and confusing.

src/til/ut_til/sources

…ompiled this before the last push and it didn't fail.

ghost · 2020-03-23T15:57:28Z

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This reverts commit ca33d89.

Correct scrolling invalidation region for tmux in pty w/ bitmap Add tracing for circling and scrolling operations. Fix improper invalidation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled. ## References - Introduced with #5024 ## Detailed Description of the Pull Request / Additional comments - This occurs when there is a scroll region restriction applied and a newline operation is performed to attempt to spin the contents of just the scroll region. This is a frequent behavior of tmux. - Right now, the Terminal doesn't support any sort of "scroll content" operation, so what happens here generally speaking is that the PTY in the ConHost will repaint everything when this happens. - The PTY when doing `AdjustCursorPosition` with a scroll region restriction would do the following things: 1. Slide literally everything in the direction it needed to go to take advantage of rotating the circular buffer. (This would force a repaint in PTY as the PTY always forces repaint when the buffer circles.) 2. Copy the lines that weren't supposed to move back to where they were supposed to go. 3. Backfill the "revealed" region that encompasses what was supposed to be the newline. - The invalidations for the three operations above were: 1. Invalidate the number of rows of the delta at the top of the buffer (this part was wrong) 2. Invalidate the lines that got copied back into position (probably unnecessary, but OK) 3. Invalidate the revealed/filled-with-spaces line (this is good). - When we were using a simple single rectangle for invalidation, the union of the top row of the buffer from 1 and the bottom row of the buffer from 2 (and 3 was irrelevant as it was already unioned it) resulted in repainting the entire buffer and all was good. - When we switched to a bitmap, it dutifully only repainted the top line and the bottom two lines as the middle ones weren't a consequence of intersect. - The logic was wrong. We shouldn't be invalidating rows-from-the-top for the amount of the delta. The 1 part should be invalidating everything BUT the lines that were invalidated in parts 2 and 3. (Arguably part 2 shouldn't be happening at all, but I'm not optimizing for that right now.) - So this solves it by restoring an entire screen repaint for this sort of slide data operation by giving the correct number of invalidated lines to the bitmap. ## Validation Steps Performed - Manual validation with the steps described in #5104 - Automatic test `ConptyRoundtripTests::ScrollWithMargins`. Closes #5104

ghost · 2020-04-22T17:12:06Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

miniksa added 19 commits March 16, 2020 17:17

bring bitmap and supporting rectangle mods over, implement more bitma…

d637178

…p tests.

Make bitmap tests. Add supplemental helpers to rectangle and some, al…

70567d4

…so add associated tests.

Add tests for setting/resetting points/rectangles out of bounds. Add …

69b44ae

…support method for checking if rectangle contains another, add test for that too.

code format pass.

fca07fa

Don't include stuff at the top of bitmap. Don't use reserved naming f…

79d1f33

…or iterator. Fix unsigned issues for x86.

Fix comment

81af5aa

SA fixes.

09139c4

Change the entire interface to just use til::rectangle.

43f952c

make bitmap track a rectangle inside. remove empty and add bitset met…

de20c5a

…hods of all/any/none. also add convenience one() method in prediction of what conpty will be looking for.

Some frees members, some comments about the rectangle iterator and mo…

a86b8fa

…ve it to details namespace.

Make resize work and add tests. Add constructors that let you allocat…

c4df795

…e a bitmap already filled. Generalize the bit checking method in the tests.

Static analysis pass.

824657f

Merge branch 'master' into dev/miniksa/til_bitmap

e3fe53d

Merge branch 'dev/miniksa/til_bitmap' into dev/miniksa/til_bitmap_pty

d7cf7d6

Add run caching to bitmap, make iterator into real input iterator so …

83e28f9

…it fits nicely into vector emplace, start using bitmap in ConPTY (Vt renderer).

Set up string printing for til types, use that inside the TAEF string…

d705527

… printing. Update VT tracing to use the same strings. Roll out bitmap through most of VT.

Operators on rectangle with points and sizes, operators between point…

95b9b85

…s and sizes, translation operator on the bitmap. A ton of tests for all that behavior. Replacing the rest of the VT renderer stuff with the invalidation map.

finish xterm engine changes, make tests work again with the new inval…

31eadef

…idation, run codeformat pass.

Merge branch 'master' into dev/miniksa/til_bitmap_pty

873a612

SA pass and code format.

0651df1

inclusive/exclusive mismatch! *shakes fist at sky*

cf06e97

miniksa added 2 commits March 19, 2020 16:22

Update simple tests to compensate for optimized invalidation rectangle.

59bfce1

Remove testing for clear/paint of non-dirty regions (per new algorithm)

a48919e

miniksa added 2 commits March 20, 2020 09:37

Format pass.

8647ae0

miniksa marked this pull request as ready for review March 20, 2020 16:39

miniksa self-assigned this Mar 20, 2020

miniksa added Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty labels Mar 20, 2020

miniksa requested review from DHowett-MSFT, carlos-zamora, leonMSFT and zadjii-msft March 20, 2020 19:38

zadjii-msft approved these changes Mar 20, 2020

View reviewed changes

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp Show resolved Hide resolved

src/inc/til/rectangle.h Outdated Show resolved Hide resolved

src/renderer/vt/XtermEngine.cpp Outdated Show resolved Hide resolved

src/til/ut_til/til.unit.tests.vcxproj Show resolved Hide resolved

DHowett-MSFT approved these changes Mar 20, 2020

View reviewed changes

PR feedback. Restoring non-optional dirty. Removing some leftover com…

0fc48b9

…ments. Taking out the MUL for scaling since it's unused and confusing.

DHowett-MSFT reviewed Mar 20, 2020

View reviewed changes

src/til/ut_til/sources Outdated Show resolved Hide resolved

miniksa added 2 commits March 20, 2020 16:11

tabs to spaces

7a5a3d3

fix the til bitmap and tests for removing the optional... I swear I c…

91c11d9

…ompiled this before the last push and it didn't fail.

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 23, 2020

ghost merged commit ca33d89 into master Mar 23, 2020

ghost deleted the dev/miniksa/til_bitmap_pty branch March 23, 2020 15:58

DHowett-MSFT pushed a commit that referenced this pull request Mar 24, 2020

Revert "Move ConPTY to use til::bitmap (#5024)"

d6300b7

This reverts commit ca33d89.

miniksa mentioned this pull request Mar 25, 2020

Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

Merged

lhecker mentioned this pull request Jun 19, 2021

Extremely slow performance when processing virtual terminal sequences #10462

Open

This pull request was closed.

Conversation

miniksa commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

miniksa commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniksa commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniksa commented Mar 19, 2020

Uh oh!

miniksa commented Mar 19, 2020

Uh oh!

miniksa commented Mar 19, 2020

Uh oh!

miniksa commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa commented Mar 20, 2020

Uh oh!

miniksa commented Mar 20, 2020

Uh oh!

miniksa commented Mar 20, 2020

Uh oh!

Uh oh!

ghost commented Mar 23, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miniksa commented Mar 19, 2020 •

edited

Loading

miniksa commented Mar 19, 2020 •

edited

Loading

miniksa commented Mar 19, 2020 •

edited

Loading

miniksa commented Mar 20, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.