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

The terminal is buffering in continuous printing #12336

Closed
donno2048 opened this issue Feb 2, 2022 · 28 comments · Fixed by #17510
Closed

The terminal is buffering in continuous printing #12336

donno2048 opened this issue Feb 2, 2022 · 28 comments · Fixed by #17510
Labels
Area-Performance Performance-related issue In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty

Comments

@donno2048
Copy link
Contributor

Windows Terminal version

1.11.3471.0

Windows build number

10.0.19043.1466

Other Software

No response

Steps to reproduce

// poc.c
#include <stdio.h>
#include <windows.h>
int main(void) {
    CONSOLE_SCREEN_BUFFER_INFO csbi;
    GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbi);
    int width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
    int height = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
    char f[height][width];
    memset(f, ' ', height * width);
    f[height / 2][width / 2] = '*';
    while(1) {
        puts(*f);
    }
}
>gcc poc.c -o main
>main

Expected Behavior

One "*" character is supposed to appear in the middle of the terminal

Actual Behavior

The terminal is buffering just like I previously mentioned here

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 2, 2022
@eabase

This comment was marked as off-topic.

@donno2048

This comment was marked as off-topic.

@elsaco

This comment was marked as off-topic.

@donno2048
Copy link
Contributor Author

That's odd, using gcc in cmd I get this (expected behavior):

cmd.mp4

And in the terminal this:

terminal.mp4

@eabase

This comment was marked as off-topic.

@donno2048

This comment was marked as off-topic.

@237dmitry

This comment was marked as off-topic.

@donno2048

This comment was marked as off-topic.

@237dmitry

This comment was marked as off-topic.

@donno2048

This comment was marked as off-topic.

@j4james
Copy link
Collaborator

j4james commented Feb 3, 2022

I suspect this is a duplicate of #11794, in which case it should be improved by PR #11890 (once that's released).

@zadjii-msft
Copy link
Member

Alright, I don't know how this thread got so off the rails. Gonna minimize half the comments here.

Alas, this didn't get better with #11890. The key here is that OP is just constantly throwing new lines into the buffer, so conpty needs to flush the new line to the terminal side each newline. IIRC I had tried avoiding that in the initial conpty implementation, but it had some bug that forced the immediate flushing. Something about InvalidateCircling. (I idly wonder if I made that call before deciding that conpty needs to be the same size as the viewport. Maybe that isn't necessary anymore.)

#1173 & #10001 might be a potential solution here. Get ConPTY out of the equation.

OP could probably remove a lot of pain by doing a \e[H at the start of each frame, by returning the cursor to the top of the screen at each frame. That would prevent them from loading up the scrollback with previous frames. There's still the underlying optimization that could be made here, so let's investigate.

cpp version of OPs code, for my reference
#include <stdio.h>
#include <windows.h>

// This wmain exists for help in writing scratch programs while debugging.
int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/)
{
    CONSOLE_SCREEN_BUFFER_INFO csbi;
    GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbi);
    int width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
    int height = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
    char* f = new char [height * width];
    memset(f, ' ', height * width);
    f[(height / 2) * width + (width / 2)] = '*';
    f[height * width - 1] = 0;
    while(1) {
        puts(f);
    }
}

@zadjii-msft zadjii-msft added Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Feb 3, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 3, 2022
@zadjii-msft zadjii-msft added this to the 22H1 milestone Feb 3, 2022
@lhecker
Copy link
Member

lhecker commented Feb 4, 2022

The code posted here has a bug where the null terminator for f is missing when passed to puts which expects one.
Use this instead:

fwrite(f, sizeof(char), height * width, stdout);

The immediate flushing of ConPTY is a significant performance issue. vtebench is a really nice tool and it shows that Windows Terminal is pretty alright in almost all areas... except for newline performance, which is 500-900x slower than other popular Terminals. See #10563.

I suspect that fixing the scrolling performance will simultaneously fix this issue more or less. \e[H, etc. should be used for your CLI application regardless though.

  • At the start of your animation hide your cursor with "\x1b[?25l"
  • Use "\x1b[H" or "\x1b[1;1H" to jump back to the top-left of the viewport each frame

@donno2048
Copy link
Contributor Author

Using any type of Unicode special sequence is problematic when running the code from CMD because then it's actually printed...

@lhecker
Copy link
Member

lhecker commented Feb 5, 2022

@donno2048 Maybe I'm misunderstanding you, but if you're referring to my (and other's) suggestion to use "\x1b[H":
This isn't unicode, but rather a so called "virtual terminal sequence", or "escape sequence".
You can read more about those here: https://en.wikipedia.org/wiki/ANSI_escape_code#Description
\x1b is the ESC character in hexadecimal and the same as \033 in octal. Some applications also accept \e as a synonym.

Modern terminals will parse such escape sequences and not show them on the screen to the user. That way you can for instance tell your terminal to jump anywhere on the screen, or change the text color, with just a bit of hidden text.

In order to use such sequences in CMD you can simply enable the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag. This flag is always enabled when you run applications in Windows Terminal. Here's some example code: https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences#samples

@donno2048
Copy link
Contributor Author

Yeah, I know what an escape sequence I just didn't remember the term 😅 (yeah it isn't Unicode but you know what I mean), in my original issue (microsoft/vscode#142001) I mentioned the possibility to use them to resolve this issue partially, the problem as I said is that when not running in a compatible terminal but rather a "plain" CMD those characters will actually be printed which is problematic

@lhecker
Copy link
Member

lhecker commented Feb 5, 2022

@donno2048 I probably continue to misunderstand you. I'm sorry in advance. 😅
CMD isn't incompatible with escape sequences. It's just disabled by default. I mean maybe you meant to say with "plain CMD" that you explicitly don't want to use escape sequences. Reading your issue on vscode however implies to me that you actually want to use escape sequences, but found that they don't work in CMD, right?

If I'm right, does it work if you try to add this at the very start of your application?

SetConsoleMode(GetStdHandle(STD_OUTPUT_HANDLE), ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING);

ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT are enabled by default, but ENABLE_VIRTUAL_TERMINAL_PROCESSING isn't. This one-liner will enable the flag and allow you to use \x1b[H in CMD. 🙂

@donno2048
Copy link
Contributor Author

SetConsoleMode(GetStdHandle(STD_OUTPUT_HANDLE), ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING);

You just blew my mind

@eabase
Copy link

eabase commented Feb 5, 2022

@lhecker

Use this instead:

I tried that, but I'm getting weird results. What code are you referring to, can you show the whole thing, and with the compiler options?

@donno2048
Copy link
Contributor Author

donno2048 commented Feb 5, 2022

Use this instead:

I tried that, but I'm getting weird results. What code are you referring to, can you show the whole thing, and with the compiler options?

The code is the code from the first comment here.

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 7, 2022
@zadjii-msft zadjii-msft removed this from the 22H1 milestone Aug 3, 2022
@zadjii-msft zadjii-msft added this to the 22H2 milestone Aug 3, 2022
@donno2048
Copy link
Contributor Author

@lhecker

Use this instead:

I tried that, but I'm getting weird results. What code are you referring to, can you show the whole thing, and with the compiler options?

Try this:

#include <stdio.h>
#include <windows.h>
int main(void) {
	int height, width;
	{
		CONSOLE_SCREEN_BUFFER_INFO csbi;
		handle_t h = GetStdHandle(STD_OUTPUT_HANDLE);
		SetConsoleMode(h, ENABLE_PROCESSED_OUTPUT | ENABLE_WRAP_AT_EOL_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING);
		GetConsoleScreenBufferInfo(h, &csbi);
		width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
		height = csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
	}
	char f[height][width];
	memset(f, ' ', height * width);
	f[height / 2][width / 2] = '*';
	while (TRUE) {
		printf("\x1b[H");
		fwrite(f, sizeof(char), height * width, stdout);
	}
}

@zadjii-msft
Copy link
Member

A thought I had: Perhaps I could reuse the "buffer but don't flush" from #14677. Flushing is expensive, buffering... less so.

@zadjii-msft
Copy link
Member

dev/migrie/f/12336-let-it-mellow has an experimentation into that thought, but it's still only 8MB/s (up from ~6MB/s) compared to the like, 98MB/s of conhost. Though, the passthrough that's in main (admittedly busted to heck) does get that up to ~70MB/s (up from ~60)

@zadjii-msft
Copy link
Member

showerthought - does that just queue the frame to get flushed on the render thread, but ultimately, the render thread still ends up blocking on WriteFile. Like, I'm worried that we end up in a place where the flush is still just blocking on I/O instead of properly flushing async.

7418101 had an attempt at just moving the WriteFile into Flush, but we still need to do something faster than that.

@j4james
Copy link
Collaborator

j4james commented Jul 20, 2023

The problem with "buffering" linefeeds is that they destroy the conhost buffer, since that's only the size of the visible viewport. Every linefeed you execute without flushing is going to erase one line of content from the top of the viewport. By the time you eventually flush the buffer, there's potentially nothing left to send to the conpty client. What should have been pushed into their scrollback would now be lost.

@zadjii-msft
Copy link
Member

Oh sorry - to clarify: in the aforementioned branch, when we circle, I'd render the text buffer out to the conpty buffer but not immediately flush the conpty buffer to the end terminal. I'd wait until the normal frame happens to do that. That would get the conpty WriteFile out of the console I/O thread's hot path (writing text).

That would still preserve the text, because the "frame" still happens before we lose the text. It just gets written to the pipe at a later time.

Now, the real issue is: can we speed writing to the pipe up? Is that slow because pipes are slow? Is it slow because Terminal drains the pipe too slow? Something something xproc calls, etc.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 20, 2023

[[nodiscard]] HRESULT VtEngine::InvalidateFlush(_In_ const bool circled, _Out_ bool* const pForcePaint) noexcept
{
// If we're in the middle of a resize request, don't try to immediately start a frame.
if (_inResizeRequest)
{
*pForcePaint = false;
}
else
{
*pForcePaint = true;
// Keep track of the fact that we circled, we'll need to do some work on
// end paint to specifically handle this.
_circled = circled;
}

hmm. Do we need to pForcePaint if

  • we're circling, AND
  • the top line is NOT invalid?

Probably not? That might be a place to speed this up a little. Not sure that'd end up actually optimizing that much out. I'd reckon we're probably outputting to the textbuffer a whole viewport worth of lines faster than a frame anyways

@j4james
Copy link
Collaborator

j4james commented Jul 20, 2023

Oh sorry - to clarify: in the aforementioned branch, when we circle, I'd render the text buffer out to the conpty buffer but not immediately flush the conpty buffer to the end terminal.

OK. That makes perfect sense. I'd forgotten there was a conpty buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Performance Performance-related issue In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants