Re: [PATCH] Cygwin: pipe: Fix performance degradation for non-cygwin pipe.

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

Hi Takashi,

both patches LGTM, feel free to push.  Your commit message looks great!


Thanks,
Corinna


On Dec  9 09:49, Takashi Yano wrote:
> https://cygwin.com/pipermail/cygwin/2022-December/252628.html
> 
> After the commit 9e4d308cd592, 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.
> 
> Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT
> flag for read pipe.")
> Reported-by: tryandbuy <tryandbuy@proton.me>
> Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pipe.cc | 16 +++++++++++++++-
>  winsup/cygwin/release/3.4.1    |  3 +++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index 720e4efd3..e23131668 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -281,6 +281,8 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>    size_t nbytes = 0;
>    NTSTATUS status = STATUS_SUCCESS;
>    IO_STATUS_BLOCK io;
> +  ULONGLONG t0 = GetTickCount64 (); /* Init timer */
> +  const ULONGLONG t0_threshold = 20;
>  
>    if (!len)
>      return;
> @@ -312,6 +314,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,
> @@ -358,7 +361,18 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>  		  nbytes = (size_t) -1;
>  		  break;
>  		}
> -	      waitret = cygwait (select_sem, 1);
> +	      /* 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 || GetTickCount64 () - 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)
> diff --git a/winsup/cygwin/release/3.4.1 b/winsup/cygwin/release/3.4.1
> index 432113a55..afe7e86f9 100644
> --- a/winsup/cygwin/release/3.4.1
> +++ b/winsup/cygwin/release/3.4.1
> @@ -8,3 +8,6 @@ Bug Fixes
>  - Fix a backward incompatibility problem in the definition of the
>    base type of the stdio type FILE.
>    Addresses: https://savannah.gnu.org/bugs/index.php?63480
> +
> +- Fix performance degradation of non-cygwin pipe.
> +  Addresses: https://cygwin.com/pipermail/cygwin/2022-December/252628.html
> -- 
> 2.38.1