Optimize rendering runs of spaces when there is no visual change #4877

DHowett-MSFT · 2020-03-10T22:30:27Z

cmatrix is somewhat of a pathological case for our infrastructure: it
prints out a bunch of green and white characters and then updates them a
million times a second.

It also maintains a column of space between every green character. When
it prints this column, it prints it in "default" or "white". This ends
up making runs of text that look like this:

(def: G=green B=bright white W=white *=matrix char =space)

G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W
G W G W G W G W G W G W G W G W

As characters trickle in:

G*W G*W G*W G*W G*W G*W G*W B*W
G*W G*W G*W G*W G*W G*W G*W G W
G*W G*W G*W B*W G*W G*W G*W G W
G*W B*W G*W G W G*W G*W G*W G*W
G*W G W G*W G W G*W B*W G*W G*W
B*W G W G*W G W G*W G W B*W G*W
G W G W G*W G W G*W G W G W B*W
G W G W B*W G W G*W G W G W G W

Every one of those color transitions causes us to break up the run of
text and start rendering it again. This impacts GDI, Direct2D and
ConPTY. In the example above, there are 120 runs.

The problem is, printing a space doesn't use the foreground color!

This commit introduces an optimization. When we're about to break a text
cluster because its attributes changed, we make sure that it's not just
filled with spaces and doesn't differ in any visually-meaningful way
(like underline or strikethrough, considering global invert state).

This lets us optimize both the rendering and the PTY output to look
like this:

G*   *   *   *   *   *   *  B*G
G*   *   *   *   *   *   *
G*   *   *  B*G  *   *   *
G*  B*G  *       *   *   *   *
G*       *       *  B*G  *   *
B*G      *       *      B*G  *
G        *       *          B*G
G       B*G      *

Text will be printed at best line-by-line and at worst only when the
visible properties of the screen actually change. In the example
above, there are only 21 runs.

This speeds up cmatrix remarkably.

PR Checklist

Related to Terminal output not speed enough #1064
CLA signed
Tests added/passed (I haven't run these yet.)
Requires documentation to be updated
Core contributee

j4james · 2020-03-10T22:58:18Z

I haven't had a chance to try your patch, but I suspect the reverse video attribute might cause problems with this optimisation. Consider the difference between this sequence...

printf "\e[31m     \e[32m     \e[m"

and this...

printf "\e[7;31m     \e[32m     \e[m"

In both case it's just a run of spaces with a foreground change, but the second sequence would actually require a cluster split wouldn't it?

And if that is the case, I assume the DECSCNM mode would cause similar problems.

DHowett-MSFT · 2020-03-10T22:59:11Z

YES, thank you. That's very helpful.

DHowett-MSFT · 2020-03-10T23:00:51Z

It won't change the mergeability of this PR, but would you [request changes] because of that?
Thanks!

DHowett-MSFT · 2020-03-10T23:13:56Z

So, an attribute knows whether it's been reversed. This is fine for reverse video, but not for global DECSCNM. I'd hate to round trip through the thing that knows the colors just to make this determination...

j4james

As discussed, this needs to take into account the reverse video attribute, and potentially the DECSCNM mode.

j4james · 2020-03-10T23:48:14Z

Regarding the DECSCNM mode, remember that that's a global attribute, so you should only need to look it up once at the start of the paint. If you can't access the global settings from the renderer, then I suppose you'd have to pass in a bool somewhere along the line. Otherwise maybe we should be updating that state in the renderer itself every time the mode is changed?

Edit: If you want to go the latter route, it looks like it should be an easy change to add something like a SetScreenReversed call to the renderer here:

terminal/src/host/getset.cpp

Lines 1308 to 1313 in a13ccfd

    
           gci.SetScreenReversed(reverseMode); 
        
           if (g.pRender) 
        
           { 
        
               g.pRender->TriggerRedrawAll(); 
        
           }

cmatrix is somewhat of a pathological case for our infrastructure: it prints out a bunch of green and white characters and then updates them a million times a second. It also maintains a column of space between every green character. When it prints this column, it prints it in "default" or "white". This ends up making runs of text that look like this: (def: G=green B=bright white W=white *=matrix char =space) G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W G W As characters trickle in: G*W G*W G*W G*W G*W G*W G*W B*W G*W G*W G*W G*W G*W G*W G*W G W G*W G*W G*W B*W G*W G*W G*W G W G*W B*W G*W G W G*W G*W G*W G*W G*W G W G*W G W G*W B*W G*W G*W B*W G W G*W G W G*W G W B*W G*W G W G W G*W G W G*W G W G W B*W G W G W B*W G W G*W G W G W G W Every one of those color transitions causes us to break up the run of text and start rendering it again. This impacts GDI, Direct2D *and* ConPTY. The problem is, printing a space doesn't **use** the foreground color! This commit introduces an optimization. When we're about to break a text cluster becuase its attributes changed, we make sure that it's not just filled with spaces and differs only in the foreground parameter. This lets us optimize both the rendering _and_ the PTY output to look like this: G* * * * * * * B* G* * * * * * * G* * * B* * * * G* B* * * * * * G* * * B* * * B* * * B* * G * * B* G B* * Text will be printed at best line-by-line and at worst only when the visible properties of the screen actually change. This speeds up cmatrix remarkably.

DHowett-MSFT · 2020-03-12T18:44:15Z

This works for both normal and invert video with both yes and no DECSCNM

j4james

You're going to hate me, but I think there's another case you need to deal with. What happens when one of the underline attributes is set? For example:

printf "\e[4;31m     \e[32m     \e[m"

You've got a run of spaces with only the foreground changing, but that still requires two clusters to handle the change in underline color doesn't it?

src/renderer/base/renderer.cpp

zadjii-msft · 2020-03-12T20:14:48Z

I'm clearing my approval in response to @j4james's comment that happened while I was reviewing

DHowett-MSFT · 2020-03-12T20:17:32Z

hate me

oh heck no 😄 i love it

j4james · 2020-03-12T20:46:36Z

src/buffer/out/TextAttribute.hpp

+
+    constexpr bool IsAnyGridLineEnabled() const noexcept
+    {
+        return WI_IsAnyFlagSet(_wAttrLegacy, COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_UNDERSCORE);


Do we want to future proof this by checking the ExtendedAttributes too (CrossedOut, Underlined, etc)? It's a bit open ended, because there are all sorts of these attributes that we may want to implement one day, so I don't know. I just thought it worth mentioning.

you're not wrong, I just keep acting like i've never written code before. 😄

I love this thread you guys

he said he dismissed it!

DHowett-MSFT · 2020-03-12T21:24:50Z

Okay, this is as comprehensive as I think I can make it -- i've renamed the function to compensate.

zadjii-msft

okay.

DHowett-MSFT · 2020-03-12T21:31:05Z

My most recent test case:

printf "\e[31m     \e[32m     \e[m\n\e[7;31m     \e[32m     \e[m\n\e[4;31m     \e[32m     \e[m\n\e[21;31m     \e[32m     \e[m\n\e[9;31m     \e[32m     \e[m\n"

FG1, FG2
Inverse video, FG1, FG2
Underline, FG1, FG2
Double underline, FG1, FG2
Strikethrough, FG1, FG2

I then toggled DECSCNM on and off with that in the buffer.

zadjii-msft · 2020-03-12T21:33:13Z

I'm not sure how you could write a test for that, which is why I'm okay with this not being tested. We don't really have a RendererTest with a mock engine or anything like that (unfortunately)

zadjii-msft · 2020-03-12T21:33:42Z

@msftbot make sure @miniksa signs off on this

ghost · 2020-03-12T21:33:49Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett-MSFT · 2020-03-12T23:35:48Z

Ugh, I explicitly code-formatted this.

DHowett-MSFT · 2020-03-12T23:37:04Z

Oh, I see. Unformatted code merged into master. Ugh.

DHowett-MSFT · 2020-03-13T00:24:30Z

/azp run

azure-pipelines · 2020-03-13T00:24:40Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2020-03-13T00:46:09Z

cannot clear olf dtatus

miniksa

Yep, good enough.

j4james suggested changes Mar 10, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2020

zadjii-msft added the Product-Terminal The new Windows Terminal. label Mar 11, 2020

DHowett-MSFT force-pushed the dev/duhowett/cmatrix branch from d7b39c8 to a857291 Compare March 12, 2020 18:13

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2020

Take global and local inversion into account

621581d

j4james suggested changes Mar 12, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2020

zadjii-msft previously approved these changes Mar 12, 2020

View reviewed changes

src/renderer/base/renderer.cpp Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2020

zadjii-msft self-requested a review March 12, 2020 20:14

don't forget grid lines

04c65ec

j4james reviewed Mar 12, 2020

View reviewed changes

okay.

f9150c2

DHowett-MSFT changed the title ~~Optimize rendering runs of spaces when only the foreground changes~~ Optimize rendering runs of spaces when there is no visual change Mar 12, 2020

zadjii-msft approved these changes Mar 12, 2020

View reviewed changes

zadjii-msft added the Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues label Mar 12, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 12, 2020

ghost requested review from miniksa, carlos-zamora and leonMSFT March 12, 2020 21:33

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

j4james approved these changes Mar 12, 2020

View reviewed changes

miniksa approved these changes Mar 13, 2020

View reviewed changes

DHowett-MSFT merged commit f919a46 into master Mar 13, 2020

DHowett-MSFT deleted the dev/duhowett/cmatrix branch March 13, 2020 00:54

miniksa mentioned this pull request Mar 20, 2020

Move ConPTY to use til::bitmap #5024

Merged

4 tasks

j4james mentioned this pull request Jul 7, 2020

Add support for DECSCNM in Windows Terminal #6809

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize rendering runs of spaces when there is no visual change #4877

Optimize rendering runs of spaces when there is no visual change #4877

DHowett-MSFT commented Mar 10, 2020 •

edited

Loading

j4james commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

j4james left a comment

j4james commented Mar 10, 2020 •

edited

Loading

DHowett-MSFT commented Mar 12, 2020

j4james left a comment

zadjii-msft commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

j4james Mar 12, 2020

DHowett-MSFT Mar 12, 2020

zadjii-msft Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

zadjii-msft left a comment

DHowett-MSFT commented Mar 12, 2020

zadjii-msft commented Mar 12, 2020

zadjii-msft commented Mar 12, 2020

ghost commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

DHowett-MSFT commented Mar 13, 2020

azure-pipelines bot commented Mar 13, 2020

DHowett-MSFT commented Mar 13, 2020

miniksa left a comment

Optimize rendering runs of spaces when there is no visual change #4877

Optimize rendering runs of spaces when there is no visual change #4877

Conversation

DHowett-MSFT commented Mar 10, 2020 • edited Loading

PR Checklist

j4james commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

DHowett-MSFT commented Mar 10, 2020

j4james left a comment

Choose a reason for hiding this comment

j4james commented Mar 10, 2020 • edited Loading

DHowett-MSFT commented Mar 12, 2020

j4james left a comment

Choose a reason for hiding this comment

zadjii-msft commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

j4james Mar 12, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 12, 2020

Choose a reason for hiding this comment

zadjii-msft Mar 12, 2020

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 12, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 12, 2020

zadjii-msft commented Mar 12, 2020

zadjii-msft commented Mar 12, 2020

ghost commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

DHowett-MSFT commented Mar 12, 2020

DHowett-MSFT commented Mar 13, 2020

azure-pipelines bot commented Mar 13, 2020

DHowett-MSFT commented Mar 13, 2020

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 10, 2020 •

edited

Loading

j4james commented Mar 10, 2020 •

edited

Loading