[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.
Mon Aug 31 09:47:55 GMT 2020
Thank you for checking the patch.
On Mon, 31 Aug 2020 10:16:51 +0200
Corinna Vinschen wrote:
> Hi Takashi,
> On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:
> > Hi Corinna,
> > On Sun, 30 Aug 2020 14:49:25 +0200
> > Corinna Vinschen wrote:
> > > I like the idea in general, but isn't there a noticable perfomance hit?
> > I have measured the startup time of non-cygwin process
> > with v2 and v8 patch.
> > mintty with v2 : 92.7ms
> > emacs-dumb with v2: 22.8ms
> > mintty with v8 : 94.6ms
> > emacs-dumb with v8: 22.7ms
> > There is not noticeable difference more than measurement
> > error.
> > By the way, I have implemented timeout strategy for CSI6n, you
> > mentioned in the previous comment, for a test. This check is
> > done only on the first execution of non-cygwin apps in the pty.
> > With this patch, first checks if the terminfo has cursor_home
> > (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> > supposed to be supported. In this case, I expect not to display
> > garbage "^[[6n" even if CSI6n is sent because the parser ignores
> > unsupported CSI sequences.
> > With this implementation, pseudo console works in tmux as well
> > even if TERM=screen.
> > Please have a look v9 patch attached.
> > The performance of v9 is also checked.
> > mintty with v9 : 93.9ms
> > emacs-dumb with v9: 22.8ms
> > ANSI without CSI6n: 22.8ms
> > [the first time in v9]
> > mintty : 94.8ms
> > emacs-dumb : 22.5ms
> > ANSI without CSI6n: 63.5ms
> > Most of the results are the same as v2 and v8 except for the
> > first execution of non-cygwin apps in ansi terminal without
> > CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> > in ANSI terminal without CSI6n support.
> > However, this causes only on the first execution of non-cygwin
> > apps in pty.
> > I think this is the most reasonable one I have ever proposed.
> This looks good, but I have a few nits:
> - Don't use GetTickCount(). Use GetTickCount64(). Otherwise the code
> is prone to the 49 days tick counter overflow problem(*).
This does not matter because the code is
if (GetTickCount() - t0 > 40)
if (GetTickCount() > t0 + 40)
and because DWORD is 32bit unsigned integer.
If the overflow occurs within the timeout period, for example,
t0 = 0xfffffff0 and GetTickCount() becomes 0x00000002,
GetTickCount() - t0 equals to 18 (0x12) as expected.
If the code is
if (GetTickCount() > t0 + 40)
and t0 = 0xfffffff0 and GetTickCount() is 0xfffffff1,
t0 + 40 equals to 24 (0x18)
so GetTickCount() > t0 + 40 is true against expectation.
> - get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().
pcon_start is cleared to false in master write() if responce to CSI6n
is sent. Therefore, it is necessary to set again after the responce.
I will added the comment in the source.
> - Following the maybe_dumb label, you're setting has_csi6n and
> has_set_title to false, but these are the default values anyway.
> Also, setting has_set_title to false in the preceeding else branch
> seems unnecessary. Do you want that for clarity? If not I'd drop
You are right. I will submit the v10 patch.
> With these minor problems fixed, I'm happy to push the change.
> (*) I just noticed belatedly that GetTickCount() is used in the tty code
> already. Can you please change pcon_last_time to ULONGLONG and use
> GetTickCount64() instead?
Same here. That does not matter.
Takashi Yano <email@example.com>
More information about the Cygwin-patches