bpo-35537: subprocess uses os.posix_spawn in some cases

[vstinner]

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

https://bugs.python.org/issue35537

[vstinner]

executable path contains a directory

Expose os.posix_spawnp() https://bugs.python.org/issue35674 will allow to remove this restriction.

[vstinner]

@pablogsal, @gpshead; @izbyshev, @serhiy-storchaka: Would you mind to review this PR?

[izbyshev]

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.

[vstinner]

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.

[vstinner]

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.

[izbyshev]

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.

[vstinner]

@giampaolo: Would you mind to review this change?

[giampaolo]

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.

[vstinner]

IMHO assuming using posix_spawn is safe, the patch as-is LGTM and considering the gain it makes sense to take the risk.

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.

Nitpick: perhaps it would be good to include the benchmark results in whatsnew.

@serhiy-storchaka asked to not write it, at least not announce a generic "60x speedup":
#11242 (comment)

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.

[vstinner]

Follow-up (support pipes): PR #11575.