Bug report

Bug description:

Apologies if this is a duplicate. I couldn’t find a similar report, though.

The issue and how to reproduce

We’re seeing a performance regression since 124af17. The best way to reproduce it is to spawn lots of processes from a ThreadPoolExecutor. For example:

#!/usr/bin/env python3


from concurrent.futures import ThreadPoolExecutor, wait
from subprocess import Popen


def target(i):
    p = Popen(['touch', f'/tmp/reproc-{i}'])
    p.communicate()


executor = ThreadPoolExecutor(max_workers=64)
futures = set()

for i in range(10_000):
    futures.add(executor.submit(target, i))

wait(futures)

Before, on 49cae39, it’s roughly this:

real    0m2.419s
user    0m4.524s
sys     0m0.976s

Since 124af17, it’s roughly this:

real    0m11.772s
user    0m10.287s
sys     0m14.409s

An attempt at an analysis and possible fix

strace shows that the new code doesn’t use vfork() anymore but clone(). I believe that the reason for this is an incorrect check of num_groups (or extra_group_size, as it is now called on main).

124af17 checks if extra_group_size is less than zero to determine if we can use vfork(). Is that correct? Maybe this should be equal to zero? I’m talking about these two locations (diff relative to main/9e56eedd018e1a46):

diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 2898eedc3e..fb6c235901 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -889,7 +889,7 @@ do_fork_exec(char *const exec_array[],
         /* These are checked by our caller; verify them in debug builds. */
         assert(uid == (uid_t)-1);
         assert(gid == (gid_t)-1);
-        assert(extra_group_size < 0);
+        assert(extra_group_size == 0);
         assert(preexec_fn == Py_None);

         /* Drop the GIL so that other threads can continue execution while this
@@ -1208,7 +1208,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
     /* Use vfork() only if it's safe. See the comment above child_exec(). */
     sigset_t old_sigs;
     if (preexec_fn == Py_None && allow_vfork &&
-        uid == (uid_t)-1 && gid == (gid_t)-1 && extra_group_size < 0) {
+        uid == (uid_t)-1 && gid == (gid_t)-1 && extra_group_size == 0) {
         /* Block all signals to ensure that no signal handlers are run in the
          * child process while it shares memory with us. Note that signals
          * used internally by C libraries won't be blocked by

extra_group_size is the result of the call to PySequence_Size(extra_groups_packed). If I understand the docs correctly, then this function only returns negative values to indicate errors. This error condition is already checked, right after the call itself:

extra_group_size = PySequence_Size(extra_groups_packed);

if (extra_group_size < 0)
    goto cleanup;

Later in the code, extra_group_size can never be less than zero. It can, however, be equal to zero if extra_groups is an empty list. I believe this is what was meant to be checked here.

I’ll happily open a PR for this if you agree that this is the way to go.

CPython versions tested on:

3.11, 3.12, 3.13, CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-112334: Restore subprocess's use of vfork() & fix extra_groups=[] behavior #112617
• [3.12] gh-112334: Restore subprocess's use of vfork() & fix extra_groups=[] behavior (GH-112617) #112731
• gh-112334: Regression test that vfork is used when expected. #112734