Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
Description
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 since124af17. The best way to reproduce it is to spawn lots of processes from aThreadPoolExecutor. For example:
#!/usr/bin/env python3fromconcurrent.futuresimportThreadPoolExecutor,waitfromsubprocessimportPopendeftarget(i):p=Popen(['touch',f'/tmp/reproc-{i}'])p.communicate()executor=ThreadPoolExecutor(max_workers=64)futures=set()foriinrange(10_000):futures.add(executor.submit(target,i))wait(futures)
Before, on49cae39, it’s roughly this:
real 0m2.419suser 0m4.524ssys 0m0.976sSince124af17, it’s roughly this:
real 0m11.772suser 0m10.287ssys 0m14.409sAn attempt at an analysis and possible fix
strace shows that the new code doesn’t usevfork() anymore butclone(). I believe that the reason for this is an incorrect check ofnum_groups (orextra_group_size, as it is now called onmain).
124af17 checks ifextra_group_size isless than zero to determine if we can usevfork(). Is that correct? Maybe this should beequal to zero? I’m talking about these two locations (diff relative tomain/9e56eedd018e1a46):
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.cindex 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 toPySequence_Size(extra_groups_packed). If I understandthe 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) gotocleanup;
Later in the code,extra_group_size can never be less than zero. It can, however, be equal to zero ifextra_groups is an empty list. I believe this is what wasmeant 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
Metadata
Metadata
Assignees
Labels
Projects
Status