bpo-35537: subprocess uses os.posix_spawn in some cases - #11452
Conversation
Sorry, something went wrong.
subprocess.Popen now uses os.posix_spawn() in some cases, if: * os.posix_spawn() is available and properly reports errors to the parent process: macOS or glibc 2.26 and newer (or glibc 2.24 and newer on Linux). * executable path contains a directory * close_fds=False * preexec_fn, pass_fds, cwd, stdin, stdout, stderr and start_new_session parameters are not set
Expose os.posix_spawnp() https://bugs.python.org/issue35674 will allow to remove this restriction. |
Sorry, something went wrong.
|
@pablogsal, @gpshead; @izbyshev, @serhiy-storchaka: Would you mind to review this PR? |
Sorry, something went wrong.
| if libc == 'glibc' and version >= (2, 26): | ||
| # glibc 2.26 added a pipe to the POSIX implementation | ||
| # of posix_spawn() to properly report errors to the parent process. | ||
| return True |
There was a problem hiding this comment.
Note that the patch that added a pipe to the POSIX implementation also switched to unconditional use of fork. Since the main motivation for using posix_spawn appears to be performance benefits of vfork, it seems that glibc's POSIX implementation shouldn't be used at all. I suggest to remove this branch.
Otherwise, LGTM.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah. I didn't require to use vfork. But ok, I modified my PR to prefer posix_spawn() implementations which can use vfork in some cases for best performances. Let's start with a minimum platform support, and extend it later.
Sorry, something went wrong.
|
I don't get it. posix_spawn() uses vfork and is 61x faster than fork+exec
(_posixsubprocess) on my laptop:
https://bugs.python.org/issue35537#msg332204
Performance is one of the 2 reasons to use it. The other one is atomicity
(that I call "safety") on macOS where it's a syscall.
…--
Night gathers, and now my watch begins. It shall not end until my death.
|
Sorry, something went wrong.
|
I meant the POSIX implementation (in sysdeps/posix/spawni.c), not the Linux one (in sysdeps/unix/sysv/linux/spawni.c) which uses clone(CLONE_VM|CLONE_VFORK) and is probably used on your laptop. In my understanding, you check for the POSIX one in glibc 2.26 branch and for the Linux one in glibc 2.24 branch. |
Sorry, something went wrong.
|
@giampaolo: Would you mind to review this change? |
Sorry, something went wrong.
giampaolo
left a comment
There was a problem hiding this comment.
I can't comment on the usage of posix_spawn per-se (I wasn't aware it existed). The only comment I can provide is that I took a look at the discussion in bpo-35537 and the rationale seems to make sense (quite consistent speedup, nice one!). IMHO assuming using posix_spawn is safe, the patch as-is LGTM and considering the gain it makes sense to take the risk.
Nitpick: perhaps it would be good to include the benchmark results in whatsnew.
Sorry, something went wrong.
Honestly, I'm not 100% sure that the change is safe. But I want to make it because of the nice speedup. I prefer to push the change early during Python 3.8 devcycle, so if someone spot an issue, we have time to try to fix it, or just revert the change.
@serhiy-storchaka asked to not write it, at least not announce a generic "60x speedup": Moreover, the speedup is only enabled under very specific conditions (see the long list of conditions in my What's New in Python 3.8 entry). We can run more benchmarks later to have a better idea of the "average speedup", and so document the speedup. |
Sorry, something went wrong.
|
Follow-up (support pipes): PR #11575. |
Sorry, something went wrong.
subprocess.Popen now uses os.posix_spawn() in some cases, if:
parent process: macOS or glibc 2.26 and newer (or glibc 2.24 and
newer on Linux).
and start_new_session parameters are not set
https://bugs.python.org/issue35537