GNU bug report logs - #66288
29.1; Performance regression using pipe for subprocess

Previous Next

Full log

Message #83 received at 66288 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Chris Hanson <cph <at> chris-hanson.org>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: gerd.moellmann <at> gmail.com, dmitry <at> gutov.dev, gregory <at> heytings.org,
 66288 <at> debbugs.gnu.org
Subject: Re: bug#66288: 29.1; Performance regression using pipe for subprocess
Date: Wed, 04 Oct 2023 09:52:52 +0300

> Date: Tue, 3 Oct 2023 20:33:29 -0400
> Cc: gerd.moellmann <at> gmail.com, 66288 <at> debbugs.gnu.org
> From: Chris Hanson <cph <at> chris-hanson.org>
> 
> I applied patch 0002* as you suggested, and that did the trick!
> 
> It's now very fast again, and measures the same as 28.2.
> 
> Thanks!
> 
> *0002-Remember-the-value-of-read_process_output_max-when-a.patch

Thanks for testing.

However, I'm reluctant to install that patch on the release branch.
First, it includes at least one (mostly harmless) error: the call to
fcntl with F_GETPIPE_SZ should use only 2 arguments, not 3.  More
importantly, it is too invasive to my palate, and is still in the
process of patch review.

Given that this is the culprit (thanks, Gregory, for the bisection!),
I conclude that the problem which caused this issue is because modern
Linux kernels use 16 pages (64KB) as the default pipe capacity,
whereas that fcntl call reduced it to just one page of 4KB.
Therefore, one simple change is for xscheme.el to make the value of
read-process-output-max to 64KB.  Chris, can you test that, using the
unmodified 29.1 sources?

A perhaps better change is the one below, which realizes that the
fcntl call was added to create_process for fixing bug#55737 so as to
allow _enlarging_ the pipe size when very large reads are required; it
was never meant to _reduce_ the default size of the pipe:

diff --git a/src/process.c b/src/process.c
index 67d1d3e..8cffc42 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2189,8 +2189,14 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
       inchannel = p->open_fd[READ_FROM_SUBPROCESS];
       forkout = p->open_fd[SUBPROCESS_STDOUT];
 
-#if defined(GNU_LINUX) && defined(F_SETPIPE_SZ)
-      fcntl (inchannel, F_SETPIPE_SZ, read_process_output_max);
+#if defined(GNU_LINUX) && defined(F_SETPIPE_SZ) && defined(F_GETPIPE_SZ)
+      /* If they requested larger reads than the default system pipe
+         capacity, enlarge the capacity to match the request.  */
+      if (read_process_output_max > fcntl (inchannel, F_GETPIPE_SZ))
+	{
+	  int readmax = clip_to_bounds (1, read_process_output_max. INT_MAX);
+	  fcntl (inchannel, F_SETPIPE_SZ, readmax);
+	}
 #endif
     }
 
Paul, any suggestions or comments?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.