bpo-35537: use os.posix_spawn in subprocess #11242

nanjekyejoannah · 2018-12-19T16:17:55Z

This patch uses os.posix_spawn in subprocess.

posix_spawn() is faster but it's also safer: it allows us to do a lot of "actions" before exec(), before executing the new program. For example, you can close files and control signals.

@vstinner this is the patch without any options.

https://bugs.python.org/issue35537

vstinner · 2018-12-19T16:19:56Z

@nanjekyejoannah: Thanks! Can you please open an issue on bugs.python.org to get a bug number? Then add "bpo-xxx: " to the title of your PR.

Can you also please add a NEWS entry? You can use the "blurb" tool: https://devguide.python.org/committing/#what-s-new-and-news-entries

nanjekyejoannah · 2018-12-19T16:27:15Z

@vstinner ack. Am doing this shortly.

vstinner · 2018-12-19T17:14:25Z

Lib/subprocess.py

+                    and not pass_fds
+                    and cwd is None
+                    and env is None
+                    and restore_signals is False


"and not restore_signals" would be better. I prefer to avoid "is" to compare with True/False.

vstinner · 2018-12-19T17:14:55Z

Lib/subprocess.py

@@ -1390,6 +1390,11 @@ def _get_handles(self, stdin, stdout, stderr):
                    errread, errwrite)


+        def _posix_spawn(self, args, executable):
+            """Execute program using os.posix_spawn()."""
+            env = dict(os.environ)


Is it really needeed to copy os.environ?

I suggest to pass directly os.environ to os.posix_spawn().

vstinner · 2018-12-19T17:30:01Z

Misc/NEWS.d/next/Library/2018-12-19-17-20-03.bpo-35537.z4E7aA.rst

@@ -0,0 +1 @@
+subprocess.Popen can now use posix_spawn() in some cases.


For better rendering (create links), I suggest:

Suggested change

subprocess.Popen can now use posix_spawn() in some cases.

:class:`subprocess.Popen` can now use :func:`os.posix_spawn` in some cases.

vstinner

LGTM, but I will wait until minor remarks are addressed.

Lib/subprocess.py

vstinner · 2018-12-20T09:10:43Z

Lib/subprocess.py

+                    and env is None
+                    and restore_signals is False
+                    and not start_new_session):
+                return self._posix_spawn(args, executable)


I suggest to pass (executable, args) to follow os.posix_spawn() API. The function has not return value, so I suggest to write instead:

Suggested change

return self._posix_spawn(args, executable)

self._posix_spawn(executable, args)

return

vstinner · 2018-12-20T09:13:19Z

Lib/subprocess.py

@@ -1390,6 +1390,11 @@ def _get_handles(self, stdin, stdout, stderr):
                    errread, errwrite)


+        def _posix_spawn(self, args, executable):
+            """Execute program using os.posix_spawn()."""
+            env = dict(os.environ)


I suggest to pass directly os.environ to os.posix_spawn().

bedevere-bot · 2018-12-20T09:13:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner

Can you please add a sentence to the Optimization section of Doc/whatsnew/3.8.rst? Something like:

"subprocess.Popen is now up to 60x faster on Linux and 106x faster on FreeBSD when os.posix_spawn() can be used."

serhiy-storchaka · 2018-12-20T12:59:59Z

60x and 106x are from particular examples. If you use different examples (allocate 1 GiB or 4 GiB of memory in the parent) you can get different numbers. Concrete numbers depend on a program and do not have meaning without referring to concrete program.

vstinner · 2018-12-20T13:43:38Z

60x and 106x are from particular examples. If you use different examples (allocate 1 GiB or 4 GiB of memory in the parent) you can get different numbers. Concrete numbers depend on a program and do not have meaning without referring to concrete program.

It's hard to measure the average speedup, but IMHO it's nice to say that it can be way faster in some cases. So do you want to explain it differently? For example:

"subprocess.Popen is now up to 60x faster on Linux and 106x faster on FreeBSD when os.posix_spawn() can be used, the parent has large memory footprint (ex: 2 GiB), and the child process execution time is short."

vstinner · 2018-12-20T14:28:45Z

@nanjekyejoannah: Until we agree on the sentence to be added to What's New in Python 3.8, don't add it. We can add it in the following PR.

serhiy-storchaka · 2018-12-20T14:44:56Z

I suggest to not use any precise numbers: "subprocess.Popen is now significantly faster" or "subprocess.Popen can be now tens times faster".

vstinner · 2018-12-20T15:26:23Z

@nanjekyejoannah It seems like you closed the PR. Was it on pupose? I'm unable to reopen the issue.

nanjekyejoannah · 2018-12-20T15:32:54Z

@vstinner I will reopen did it by mistake.

vstinner · 2018-12-20T16:08:16Z

@nanjekyejoannah: this PR is now empty, it doesn't contain any commit :-( It seems like your removed your commit. You can use "git reflog" or open .git/logs/HEAD to recover your removed commit.

vstinner · 2018-12-20T16:29:31Z

Lib/subprocess.py

@@ -1390,6 +1390,11 @@ def _get_handles(self, stdin, stdout, stderr):
                    errread, errwrite)


+        def _posix_spawn(self, args, executable):
+            """Execute program using os.posix_spawn()."""
+            env = os.environ


nitpick: this variable is useless, just pass directly os.environ to posix_spawn().

vstinner

LGTM.

vstinner · 2018-12-20T16:43:11Z

I will merge this PR tomorrow except if someone sees a good reason to not merge it: https://bugs.python.org/issue35537#msg332239

izbyshev · 2018-12-20T17:19:56Z

Lib/subprocess.py

+                    and not start_new_session
+                    and os.path.dirname(executable)
+                    and self.stdin is self.stdout is self.stderr is None):
+                self._posix_spawn(args, executable)


Checking self.std* is still not enough. Those fields are for the parent's ends of pipes, but if a caller redirects to a descriptor, there is no such end. For example, subprocess.call(['/bin/sleep', '1000'], stdout=fd, close_fds=False, restore_signals=False) doesn't work properly.

@izbyshev: Oh right! I fixed the test: test (p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite) instead of (self.stdin, self.stdout, self.stderr). Is it better now?

Yes, LGTM now.

There is another potential problem with posix_spawn: asynchronous error notification. _posixsubprocess uses a pipe to notify the parent about all errors up to and including execve errors, so its style is synchronous. Glibc and musl use the pipe-based notification too, but they are not required to do that, and I don't know what other implementations do.

Test (p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite) instead of (self.stdin, self.stdout, self.stderr).

vstinner · 2018-12-20T21:46:28Z

@nanjekyejoannah: I pushed directly a fix into your branch to fix the latest reported bug about standard streams ;-)

vstinner · 2018-12-20T22:10:10Z

I will merge this PR tomorrow except if someone sees a good reason to not merge it: https://bugs.python.org/issue35537#msg332239

Hum, @izbyshev found another issue (already now fixed) with this PR. Since I will be offline next 2 weeks, I prefer to not merge yet since I would be unable to handle any issue caused by the change. I now prefer to wait until I'm back from holiday to merge the PR.

In the meanwhile, if another core dev feels brave enough to merge it, please go ahead ;-)

nanjekyejoannah · 2018-12-21T14:08:09Z

okay noted.

vstinner · 2018-12-21T19:07:05Z

I expect that posix_spawn() sets errno as does execv(). I tested a non existent executable and I got the expected exception and traceback. The pipe matters more when preexec_fn is used to return the Python traceback of the child and the exception type. But preeexec_fn doesn't make sense with (cannot be supported by) posix_spawn().

…

-- Night gathers, and now my watch begins. It shall not end until my death.

izbyshev · 2018-12-21T19:32:40Z

I expect that posix_spawn() sets errno as does execv()

As I explained, this behavior is not required by POSIX, and the opposite behavior is allowed. Recent glibc- and musl-based Linux behave as you expect, but for glibc, this was added in 2017. In my test on Ubuntu 14.04, ./python -c "import subprocess; subprocess.call(['/xxx'], close_fds=False, restore_signals=False)" silently returns with zero exit code.

FreeBSD also seems not to use any synchronous notification.

Other systems where posix_spawn is a syscall (macOS?) probably behave as you expect, but I've not checked.

vstinner · 2019-01-16T00:33:05Z

I merged PR #11452 instead.

use os.posix_spawn in subprocess

6726dac

nanjekyejoannah requested a review from gpshead as a code owner December 19, 2018 16:17

the-knights-who-say-ni added the CLA signed label Dec 19, 2018

bedevere-bot added the awaiting review label Dec 19, 2018

vstinner requested review from pablogsal and vstinner December 19, 2018 16:22

nanjekyejoannah changed the title ~~use os.posix_spawn in subprocess~~ bpo-35537: use os.posix_spawn in subprocess Dec 19, 2018

vstinner reviewed Dec 19, 2018

View reviewed changes

vstinner approved these changes Dec 19, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 19, 2018

izbyshev reviewed Dec 19, 2018

View reviewed changes

Lib/subprocess.py Show resolved Hide resolved

vstinner requested changes Dec 20, 2018

View reviewed changes

bedevere-bot removed the awaiting merge label Dec 20, 2018

bedevere-bot added the awaiting changes label Dec 20, 2018

vstinner reviewed Dec 20, 2018

View reviewed changes

nanjekyejoannah closed this Dec 20, 2018

nanjekyejoannah reopened this Dec 20, 2018

nanjekyejoannah and others added 2 commits December 20, 2018 19:20

use os.posix_spawn in subprocess

f7df9e8

📜🤖 Added by blurb_it.

c05a078

vstinner reviewed Dec 20, 2018

View reviewed changes

vstinner approved these changes Dec 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 20, 2018

izbyshev reviewed Dec 20, 2018

View reviewed changes

Fix test deciding if posix_spawn() can be used

1b9357a

Test (p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite) instead of (self.stdin, self.stdout, self.stderr).

vstinner mentioned this pull request Jan 15, 2019

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

Merged

vstinner closed this Jan 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-35537: use os.posix_spawn in subprocess #11242

bpo-35537: use os.posix_spawn in subprocess #11242

nanjekyejoannah commented Dec 19, 2018 •

edited by bedevere-bot

vstinner commented Dec 19, 2018

nanjekyejoannah commented Dec 19, 2018

vstinner Dec 19, 2018

vstinner Dec 19, 2018

vstinner Dec 20, 2018

vstinner Dec 19, 2018

vstinner left a comment

vstinner Dec 20, 2018

vstinner Dec 20, 2018

bedevere-bot commented Dec 20, 2018

vstinner left a comment

serhiy-storchaka commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

serhiy-storchaka commented Dec 20, 2018

vstinner commented Dec 20, 2018

nanjekyejoannah commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner Dec 20, 2018

vstinner left a comment

vstinner commented Dec 20, 2018

izbyshev Dec 20, 2018

vstinner Dec 20, 2018

izbyshev Dec 21, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

nanjekyejoannah commented Dec 21, 2018

vstinner commented Dec 21, 2018 via email

izbyshev commented Dec 21, 2018

vstinner commented Jan 16, 2019

		@@ -0,0 +1 @@
		subprocess.Popen can now use posix_spawn() in some cases.

	subprocess.Popen can now use posix_spawn() in some cases.
	:class:`subprocess.Popen` can now use :func:`os.posix_spawn` in some cases.

	return self._posix_spawn(args, executable)
	self._posix_spawn(executable, args)
	return

bpo-35537: use os.posix_spawn in subprocess #11242

bpo-35537: use os.posix_spawn in subprocess #11242

Conversation

nanjekyejoannah commented Dec 19, 2018 • edited by bedevere-bot

vstinner commented Dec 19, 2018

nanjekyejoannah commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 20, 2018

vstinner left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Dec 20, 2018

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

serhiy-storchaka commented Dec 20, 2018

vstinner commented Dec 20, 2018

nanjekyejoannah commented Dec 20, 2018

vstinner commented Dec 20, 2018

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Dec 20, 2018

vstinner commented Dec 20, 2018

nanjekyejoannah commented Dec 21, 2018

vstinner commented Dec 21, 2018 via email

izbyshev commented Dec 21, 2018

vstinner commented Jan 16, 2019

nanjekyejoannah commented Dec 19, 2018 •

edited by bedevere-bot