Re: sort: prefer posix_spawnp to fork and execlp

[Pádraig Brady]

On 24/10/2025 17:05, Pádraig Brady wrote:

On 24/10/2025 13:33, Pádraig Brady wrote:

On 24/10/2025 07:24, Collin Funk wrote:

Here is a patch to make 'sort' use posix_spawnp.

Very tiny performance improvement:

       $ export LC_ALL=C
       $ seq 100000 | shuf > input
       $ perf stat -i --repeat 10 ./src/sort-prev -S 1K \
           --compress-program=gzip input 2>&1 > /dev/null \
           | grep -F 'seconds time elapsed'
                   4.5896 +- 0.0109 seconds time elapsed  ( +-  0.24% )
       $ perf stat -i --repeat 10 ./src/sort -S 1K \
           --compress-program=gzip input 2>&1 > /dev/null \
           | grep -F 'seconds time elapsed'
                  4.53118 +- 0.00142 seconds time elapsed  ( +-  0.03% )

But hopefully it avoids a fork failing with ENOMEM.

Also, I think the error reporting is a bit nicer with this change. Here
is how it behaved previously:

       $ /bin/sort -S 1K --compress-program=missing Makefile 2>&1
       couldn't execute compress program: errno 2
       [...]
       couldn't execute compress program: errno 2
       sort: 'missing' [-d] terminated abnormally
       couldn't execute compress program: errno 2
       couldn't execute compress program: errno 2
       $ /bin/sort -S 1K --compress-program=missing Makefile 2>&1
       couldn't execute compress program: errno 2
       [...]
       couldn't execute compress program: errno 2
       sort: 'missing' [-d] terminated abnormally
       couldn't execute compress program: errno 2

Here it is now:

       $ ./src/sort -S 1K --compress-program=missing Makefile 2>&1
       sort: couldn't create process for 'missing': No such file or directory

That's quite a bit better.

The logic in the patch looks sound.

Actually the fall-back logic in maybe_create_temp() looks wrong.Previously any 
fork failure (usually due to resource contraints)
would result in a fallback to writing to temp file directly.

Now any posix_spawn() failure results in immediate exit.

Should we treat the --compress-program as fully optional
(which it was anyway in the case of resource contraints)
and always fall-back to writing the temp file directly?
(We'd write an error for each spawn failure of course).
I'm inclined to think we should do that.

An untested patch to do that:

diff --git a/src/sort.c b/src/sort.c
index c8df900f6..6a8004904 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1106,8 +1106,8 @@ pipe_child (pid_t *pid, int pipefds[2], int tempfd, bool 
decompress,

       result = posix_spawnp (pid, compress_program, &actions, &attr,
                              (char * const *) argv, environ);
-      if (result == 0)
-        temphead = saved_temphead;
+
+      temphead = saved_temphead;

       cs_leave (&cs);

@@ -1154,12 +1154,18 @@ maybe_create_temp (FILE **pfp, bool 
survive_fd_exhaustion)
   if (compress_program)
     {
       int pipefds[2];
+      static int last_result = 0;
+
       int result = pipe_child (&node->pid, pipefds, tempfd, false,
                                MAX_TRIES_COMPRESS);

       if (result)
-        error (SORT_FAILURE, result, _("could not run compress program %s"),
-               quoteaf (compress_program));
+        {
+          if (result != last_result)
+            error (0, result, _("could not run compress program %s"),
+                   quoteaf (compress_program));
+          last_result = result;
+        }
       else
         {
           close (tempfd);