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

Cursor flicker in Cygwin irssi #18264

Closed
pragma- opened this issue Nov 30, 2024 · 18 comments
Closed

Cursor flicker in Cygwin irssi #18264

pragma- opened this issue Nov 30, 2024 · 18 comments
Assignees
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@pragma-
Copy link

pragma- commented Nov 30, 2024

Windows Terminal version

1.22.3232.0

Windows build number

10.0.19045.0

Other Software

Cygwin

Steps to reproduce

  1. Open Windows Terminal
  2. Open Task Manager with Page Faults displayed (Details tab, right-click columns, select Page fault delta column)
  3. Note zero page faults in OpenConsole.exe
  4. Run Cygwin.bat
  5. Note 800-2000 page faults per second in OpenConsole.exe
  6. Exit Cygwin
  7. Note page faults returns to 0 in OpenConsole.exe

Expected Behavior

Page faults to remain at 0 or low in OpenConsole.exe while Cygwin is running.

Actual Behavior

800-2000 page faults per second in OpenConsole.exe while Cygwin is running. See video below:

10-24-34.mp4
@pragma- pragma- added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 30, 2024
@pragma-
Copy link
Author

pragma- commented Nov 30, 2024

Page faults are 0 when starting Cygwin in either Windows' cmd.exe or Cygwin's mintty.exe.

@DHowett
Copy link
Member

DHowett commented Dec 2, 2024

Cygwin is calling console APIs in its steady state.

It looks like it's calling GetConsoleProcessList in a tight loop, which results in the allocation of a new buffer that is returned to their process via condrv's ioctl interface.

I don't think there's anything we can do about that, other than stopping them from doing so

Is there an issue you're troubleshooting here other than a raw increased count of page faults/?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 2, 2024
@pragma-
Copy link
Author

pragma- commented Dec 2, 2024

I am experiencing stutters and visible cursor movement during terminal screen redraws that I do not experience when using cmd.exe or mintty.exe. I will attempt to bring this issue to the Cygwin team's awareness.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 2, 2024
@DHowett
Copy link
Member

DHowett commented Dec 2, 2024

during terminal screen redraws

You're experiencing a graphical issue using the new Windows Terminal which uses the GPU to render things.

I doubt your issue has anything to do with the console server and its soft page faults. 🙂

@lhecker What are our usual troubleshooting steps for things like this?

@BrianInglis
Copy link

BrianInglis commented Dec 2, 2024

The only page thrasher of that magnitude on my system at the moment appears to be MS Windows Defender! ;^>
I've used Cygwin for decades and never used or even looked at Cygwin.bat which seems to be causing these problems.
That command script Cygwin.bat seems just to cd to Cygwin "root" dir using MS Windows path, and last line runs bash as an interactive login shell, so what is the difference from running under cmd?
I expect bash threads to hang for console input and subprocess termination, or run shell functions or builtin commands, and I see 3-6 (Xwindows is 6) for each instance at the moment.

@lhecker
Copy link
Member

lhecker commented Dec 2, 2024

It looks like it's calling GetConsoleProcessList in a tight loop, which results in the allocation of a new buffer that is returned to their process via condrv's ioctl interface.

It would be great if there was a different solution for that. Waiting on a handle seems better than polling to me.

@lhecker What are our usual troubleshooting steps for things like this?

I think a video of the issue would be a good start. Ideally a screen recording. If a video doesn't clear up what exactly is lagging and how and when, I'd need a significantly more detailed description of the issue.

@pragma-
Copy link
Author

pragma- commented Dec 2, 2024

Here is a video of the cursor flickering. On the left side is Windows Terminal without Cygwin. On the right side is Windows Terminal with Cygwin. Both terminals are running a SSH session into my Linux server where I have reattached to a TMUX session that is running WeeChat, an IRC client. Please note the annoyingly visible cursor flickering between the clock and the input location in the right-side Terminal that is running Cygwin. This cursor flickering is not present when Cygwin is running in cmd.exe or mintty.exe.

untitled.mp4

@lhecker
Copy link
Member

lhecker commented Dec 3, 2024

The cursor flickering issue is over here: #6217
I have different theories on why this may be happening. The most likely one at this time is that applications simply aren't controlling their stdout very well. If they don't atomically redraw the entire UI in a single stdout call, then we can randomly render the terminal contents in between two frames.

In the past this was not obvious because after we saw a write we would wait 16ms until we flush our internal buffers. That's also the reason why wezterm regressed: It updated to a newer ConPTY version where that 16ms was rightfully removed.

One solution is to add back a slight delay, but I would prefer solving the issue in another way.

@tyan0
Copy link

tyan0 commented Dec 3, 2024

Cygwin is calling console APIs in its steady state.

It looks like it's calling GetConsoleProcessList in a tight loop, which results in the allocation of a new buffer that is returned to their process via condrv's ioctl interface.

I don't think there's anything we can do about that, other than stopping them from doing so

Do you mean we cannot call Windows API (GetConsoleProcessList()) frequently? Then, how often can we call it?
Since this issue does not happen with Windows Terminal 1.21.3231.0, it sounds like a regression of Windows Terminal for me.

I also wonder why "the allocation of a new buffer" is necessary. The buffer LPDWORD lpdwProcessList is provided by caller side.

@tyan0
Copy link

tyan0 commented Dec 3, 2024

It looks like it's calling GetConsoleProcessList in a tight loop, which results in the allocation of a new buffer that is returned to their process via condrv's ioctl interface.

I doubt that this is the real cause.
The following code does not cause the issue even with version 1.22.3232.0

#include <windows.h>

int main()
{
	DWORD buf[65536];
	for (;;) GetConsoleProcessList(buf, 65536);
	return 0;
}

@DHowett
Copy link
Member

DHowett commented Dec 3, 2024

The buffer LPDWORD lpdwProcessList is provided by caller side.

It's provided by the caller in another process. The console host does not share memory with or map memory from the caller.

FWIW, GetConsoleProcessList is the only thing Cygwin is doing repeatedly when idle. It turns out that tracking page faults is difficult (? or impossible?) without a kernel debugger. Thanks for testing.

Regardless, neither page faults nor this API are the reason @pragma-'s cursor is jumping between irssi's status line (updated every second) and their input line. That's just because we render the intermediate cursor position. The cursor moves to the status line when irssi overwrites it, because that's how overwriting a region of text works.

Usually, it's hidden when it does that. That's almost certainly the actual root cause: somebody isn't hiding it (whether it's irssi, Cygwin's emulated TTY, or us).

@pragma-, can you echo $TERM in the working and broken sessions?

(thanks for sharing the video!)

@DHowett DHowett changed the title High page-faults in OpenConsole.exe when running Cygwin Cursor flicker in Cygwin irssi Dec 3, 2024
@tyan0
Copy link

tyan0 commented Dec 3, 2024

I could have reproduced the page fault with the following simple test case. Regardless of the frequency of API calls, 64 page-fault occurs every loop. Unlike cygwin case, the page fault happens also with version 1.21.3231.0.

#include <windows.h>

int main()
{
	DWORD buf[65536];
	INPUT_RECORD rec;
	DWORD n;
	for (;;) {
		GetConsoleProcessList(buf, 65536);
		Sleep(1000);
		PeekConsoleInput(GetStdHandle(STD_INPUT_HANDLE), &rec, 1, &n);
		Sleep(1000);
	}
	return 0;
}

@pragma-
Copy link
Author

pragma- commented Dec 3, 2024

@DHowett The $TERM is screen-256color in both terminals. By the way, it's weechat rather than irssi. I've customized my weechat theme to look like irssi! :-)

@DHowett
Copy link
Member

DHowett commented Dec 3, 2024

customized

Oh nice!

screen

Oh, uh, what is TERM outside of screen in both cases?

@pragma-
Copy link
Author

pragma- commented Dec 3, 2024

xterm-256color

@lhecker
Copy link
Member

lhecker commented Dec 3, 2024

I figured out why this happens: When we read from the console pipe, we currently need to provide a buffer where the message is copied into, similar to ReadFile. In order to slightly amortize the cost of allocating a buffer we reuse the previous allocation unless the previous one was >16KiB and the next one is less than half the size. This avoids issues where a huge 100MB buffer never gets freed.
This however does not work well for this usage of the API, because the GetConsoleProcessList call needs 128KiB and the PeekConsoleInput call only needs 20 (?) bytes. This causes the buffer to be repeatedly freed and reallocated.

IMO there's 3 things we should do:

  • cygwin should avoid making "large" data requests if there's no reason for that. That DWORD buf[65536]; buffer is 128KiB large and in practice maybe 16 byte of that will ever be used. It could improve the code by only increasing the request size if the return value is equal to the buffer size.
  • We should increase the cutoff size from 16KiB to 128KiB, because that's the size of the cat chunk size anyway.
  • We should switch to a cheap allocator for processing console messages in the future, because malloc is a poor fit for it. But this doesn't fix the issue for large allocations, since we still need to be conservative about our resident set size. This means that even with a custom allocator, cygwin's usage pattern may still be suboptimal.

github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Dec 4, 2024
Currently, the buffer of 128KB is passed to GetConsoleProcessList().
This causes page fault in the select() loop for console due to:
microsoft/terminal#18264
because the previous code calls GetConsoleProcessList() with large
buffer and PeekConsoleInput() with small buffer alternately.
With this patch, the minimum buffer size is used that is determined
by GetConsoleProcessList() with small buffer passed.

Addresses: https://cygwin.com/pipermail/cygwin/2024-December/256841.html
Fixes: 7277014 ("Cygwin: pty: Prevent pty from changing code page of parent console.")
Reported-by: Steven Buehler <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
(cherry picked from commit 1a49c17)
github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Dec 4, 2024
Currently, the buffer of 128KB is passed to GetConsoleProcessList().
This causes page fault in the select() loop for console due to:
microsoft/terminal#18264
because the previous code calls GetConsoleProcessList() with large
buffer and PeekConsoleInput() with small buffer alternately.
With this patch, the minimum buffer size is used that is determined
by GetConsoleProcessList() with small buffer passed.

Addresses: https://cygwin.com/pipermail/cygwin/2024-December/256841.html
Fixes: 7277014 ("Cygwin: pty: Prevent pty from changing code page of parent console.")
Reported-by: Steven Buehler <[email protected]>
Signed-off-by: Takashi Yano <[email protected]>
@carlos-zamora
Copy link
Member

Now that the first two remediations are complete, we're going to mark this bug as a /dup of #6217. Thanks!

Copy link
Contributor

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Attention The core contributors need to come back around and look at this ASAP. labels Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

6 participants