Re: Performance regression in cygwin 3.4.0

Re: Performance regression in cygwin 3.4.0

[Takashi Yano]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

Hi, Corinna, Ken, and other cygwin developers,

I tried to fix the following performance problem of
non-cygwin pipe and succeeded by applying the patch
attached.

Although it is not smart enough, it works as far as
I tested.

What do you think of this patch? Any other idea?

Comments or suggestions will be appreciated.

On Thu, 8 Dec 2022 18:46:56 +0900
Takashi Yano wrote:
> On Wed, 07 Dec 2022 19:38:19 +0000
> tryandbuy wrote:
> > Reading from stdin is very slow when a process runs outside of Cygwin Terminal.
> > 
> > Steps to reproduce:
> > 1. Create "test.txt" file using python code:
> > long_text = "10" * 2500
> > with open(r'test.txt', 'w') as f:
> >     for i in range(5000):
> >         f.write('KEY%03d: %d => %s\n' % (i % 100, i, long_text))
> > 
> > 2. Install "cygwin" package version > "3.3.6-1"
> > 3. Open windows command prompt (cmd.exe)
> > 4. Enter command:
> > type test.txt | c:\cygwin64\bin\wc.exe -l
> > 
> > When running the same command (use cat instead of type) on the Cygwin Terminal, no performance issues observed.
> > 
> > The last version of cygwin package without issues: 3.3.6-1
> > Versions of cygwin package with issues: 3.4.x - 3.5.x
> > 
> > P.S. I tested this combinations of pipings:
> > cygwin_prog | win_prog    # no issues
> > win_prog | cygwin_prog    # has issues when running in windows command prompt, no issues in Cygwin Terminal
> > cygwin_prog | cygwin_prog # has issues  when running in windows command prompt, no issues in Cygwin Terminal
> 
> I confirmed the problem. I also identified the code difference,
> which causes the problem, between cygwin 3.3.x and 3.4.0.
> 
> Let us consider how to fix that.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: 0001-Cygwin-pipe-Fix-performance-degradation-for-non-cygw.patch --]
[-- Type: application/octet-stream, Size: 2681 bytes --]

From eb11d098dd8d0bf9c52291c5b5106715d5e3128d Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Thu, 8 Dec 2022 22:02:43 +0900
Subject: [PATCH] Cygwin: pipe: Fix performance degradation for non-cygwin
 pipe.

- After the commit 9e4d308cd592fe383dec58ea6523c1b436888ef8, the
  performance of read from non-cygwin pipe has been degraded. This
  is because select_sem mechanism does not work for non-cygwin pipe.
  This patch fixes the issue.

  Addresses:
    https://cygwin.com/pipermail/cygwin/2022-December/252628.html
---
 winsup/cygwin/fhandler/pipe.cc | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 720e4efd3..654e94ebd 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -281,6 +281,9 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
   size_t nbytes = 0;
   NTSTATUS status = STATUS_SUCCESS;
   IO_STATUS_BLOCK io;
+  DWORD t0 = GetTickCount (); /* Init timer */
+  const DWORD t0_threshold = 20;
+  const DWORD t0_ovr = 100;
 
   if (!len)
     return;
@@ -312,6 +315,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
     {
       ULONG_PTR nbytes_now = 0;
       ULONG len1 = (ULONG) (len - nbytes);
+      DWORD select_sem_timeout = 0;
 
       FILE_PIPE_LOCAL_INFORMATION fpli;
       status = NtQueryInformationFile (get_handle (), &io,
@@ -338,6 +342,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
 	  nbytes += nbytes_now;
 	  if (select_sem && nbytes_now > 0)
 	    release_select_sem ("raw_read");
+	  t0 = GetTickCount (); /* Reset timer */
 	}
       else
 	{
@@ -358,7 +363,21 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
 		  nbytes = (size_t) -1;
 		  break;
 		}
-	      waitret = cygwait (select_sem, 1);
+	      /* Prevent timer from overflow */
+	      if (GetTickCount () - t0 > t0_ovr)
+		t0 += t0_ovr - t0_threshold;
+	      /* If the pipe is a non-cygwin pipe, select_sem trick
+		 does not work. As a result, the following cygwait()
+		 will return only after timeout occurs. This causes
+		 performance degradation. However, setting timeout
+		 to zero causes high CPU load. So, set timeout to
+		 non-zero only when select_sem is valid or pipe is
+		 not ready to read for more than t0_threshold.
+		 This prevents both the performance degradation and
+		 the high CPU load. */
+	      if (select_sem || GetTickCount () - t0 > t0_threshold)
+		select_sem_timeout = 1;
+	      waitret = cygwait (select_sem, select_sem_timeout);
 	      if (waitret == WAIT_CANCELED)
 		pthread::static_cancel_self ();
 	      else if (waitret == WAIT_SIGNALED)
-- 
2.38.1

Re: Performance regression in cygwin 3.4.0

[Corinna Vinschen]

Hi Takashi,

On Dec  8 23:54, Takashi Yano wrote:
> Hi, Corinna, Ken, and other cygwin developers,
> 
> I tried to fix the following performance problem of
> non-cygwin pipe and succeeded by applying the patch
> attached.
> 
> Although it is not smart enough, it works as far as
> I tested.
> 
> What do you think of this patch? Any other idea?
> 
> Comments or suggestions will be appreciated.

> From eb11d098dd8d0bf9c52291c5b5106715d5e3128d Mon Sep 17 00:00:00 2001
> From: Takashi Yano <takashi.yano@nifty.ne.jp>
> Date: Thu, 8 Dec 2022 22:02:43 +0900
> Subject: [PATCH] Cygwin: pipe: Fix performance degradation for non-cygwin
>  pipe.
> 
> - After the commit 9e4d308cd592fe383dec58ea6523c1b436888ef8, the

I wrote about that on cygwin-patches a while back.  Can we follow Linux
kernel commit messages and use standard tags per
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html,
please?

I mean especially the "Fixes:" tag as well as the "Signed-off-by:"
tag per
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
and
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

The commit id in the Fixes tag is typically 12 digits, i.e.:

>   performance of read from non-cygwin pipe has been degraded. This
>   is because select_sem mechanism does not work for non-cygwin pipe.
>   This patch fixes the issue.
> 
>   Addresses:
>     https://cygwin.com/pipermail/cygwin/2022-December/252628.html

    Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT
    flag for read pipe.")
    Signed-off-by: <developer email address>

If we do this from now on, that would be great.

As for the code, I have just one nit.  Don't use GetTickCount.  Use
GetTickCount64().  I just noticed you're using it in tty::wait_fwd as
well.  Please use GetTickCount64() there, too.  This way you can
get rid of the overflow handling.

Ken?  Any more input?


Thanks,
Corinna

>    if (!len)
>      return;
> @@ -312,6 +315,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>      {
>        ULONG_PTR nbytes_now = 0;
>        ULONG len1 = (ULONG) (len - nbytes);
> +      DWORD select_sem_timeout = 0;
>  
>        FILE_PIPE_LOCAL_INFORMATION fpli;
>        status = NtQueryInformationFile (get_handle (), &io,
> @@ -338,6 +342,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>  	  nbytes += nbytes_now;
>  	  if (select_sem && nbytes_now > 0)
>  	    release_select_sem ("raw_read");
> +	  t0 = GetTickCount (); /* Reset timer */
>  	}
>        else
>  	{
> @@ -358,7 +363,21 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>  		  nbytes = (size_t) -1;
>  		  break;
>  		}
> -	      waitret = cygwait (select_sem, 1);
> +	      /* Prevent timer from overflow */
> +	      if (GetTickCount () - t0 > t0_ovr)
> +		t0 += t0_ovr - t0_threshold;
> +	      /* If the pipe is a non-cygwin pipe, select_sem trick
> +		 does not work. As a result, the following cygwait()
> +		 will return only after timeout occurs. This causes
> +		 performance degradation. However, setting timeout
> +		 to zero causes high CPU load. So, set timeout to
> +		 non-zero only when select_sem is valid or pipe is
> +		 not ready to read for more than t0_threshold.
> +		 This prevents both the performance degradation and
> +		 the high CPU load. */
> +	      if (select_sem || GetTickCount () - t0 > t0_threshold)
> +		select_sem_timeout = 1;
> +	      waitret = cygwait (select_sem, select_sem_timeout);
>  	      if (waitret == WAIT_CANCELED)
>  		pthread::static_cancel_self ();
>  	      else if (waitret == WAIT_SIGNALED)
> -- 
> 2.38.1
> 

Re: Performance regression in cygwin 3.4.0

[Ken Brown]

On 12/8/2022 10:30 AM, Corinna Vinschen wrote:
> Hi Takashi,
> As for the code, I have just one nit.  Don't use GetTickCount.  Use
> GetTickCount64().  I just noticed you're using it in tty::wait_fwd as
> well.  Please use GetTickCount64() there, too.  This way you can
> get rid of the overflow handling.
> 
> Ken?  Any more input?

LGTM.

Ken

Re: Performance regression in cygwin 3.4.0

[Takashi Yano]

On Thu, 8 Dec 2022 15:56:43 -0500
Ken Brown wrote:
> On 12/8/2022 10:30 AM, Corinna Vinschen wrote:
> > Hi Takashi,
> > As for the code, I have just one nit.  Don't use GetTickCount.  Use
> > GetTickCount64().  I just noticed you're using it in tty::wait_fwd as
> > well.  Please use GetTickCount64() there, too.  This way you can
> > get rid of the overflow handling.
> > 
> > Ken?  Any more input?
> 
> LGTM.

Thanks for reviewing. I will submit a revised patch
to cygwin-patches@cygwin.com.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>