Re: Performance regression in cygwin 3.4.0

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

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
>