* Re: Performance regression in cygwin 3.4.0
[not found] ` <20221208184656.ea6b9796eccc503c1e238f0f@nifty.ne.jp>
@ 2022-12-08 14:54 ` Takashi Yano
2022-12-08 15:30 ` Corinna Vinschen
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Yano @ 2022-12-08 14:54 UTC (permalink / raw)
To: cygwin-developers
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Performance regression in cygwin 3.4.0
2022-12-08 14:54 ` Performance regression in cygwin 3.4.0 Takashi Yano
@ 2022-12-08 15:30 ` Corinna Vinschen
2022-12-08 20:56 ` Ken Brown
0 siblings, 1 reply; 4+ messages in thread
From: Corinna Vinschen @ 2022-12-08 15:30 UTC (permalink / raw)
To: cygwin-developers
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Performance regression in cygwin 3.4.0
2022-12-08 15:30 ` Corinna Vinschen
@ 2022-12-08 20:56 ` Ken Brown
2022-12-09 0:48 ` Takashi Yano
0 siblings, 1 reply; 4+ messages in thread
From: Ken Brown @ 2022-12-08 20:56 UTC (permalink / raw)
To: cygwin-developers
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Performance regression in cygwin 3.4.0
2022-12-08 20:56 ` Ken Brown
@ 2022-12-09 0:48 ` Takashi Yano
0 siblings, 0 replies; 4+ messages in thread
From: Takashi Yano @ 2022-12-09 0:48 UTC (permalink / raw)
To: cygwin-developers
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-09 0:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <Abw9_BgkZRll8zEaAWSNVO1Uw0sF_--Amd02G5fpj3r5O6QBhyqjWu2cxV1IctJtM03OGztfZx60TEIYfDi34p3_unluAxKgQDzcsYZASxY=@proton.me>
[not found] ` <20221208184656.ea6b9796eccc503c1e238f0f@nifty.ne.jp>
2022-12-08 14:54 ` Performance regression in cygwin 3.4.0 Takashi Yano
2022-12-08 15:30 ` Corinna Vinschen
2022-12-08 20:56 ` Ken Brown
2022-12-09 0:48 ` Takashi Yano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).