GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-35537: subprocess uses os.posix_spawn in some cases by vstinner · Pull Request #11452 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jan 7, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
bpo-35537: subprocess uses os.posix_spawn in some cases
cc999ff 
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
@vstinner
vstinner requested a review from gpshead as a code owner January 7, 2019 00:08
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 7, 2019
@bedevere-bot bedevere-bot added the awaiting merge label Jan 7, 2019
@vstinner

vstinner commented Jan 14, 2019

Copy link
Copy Markdown
Member Author

executable path contains a directory

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jan 14, 2019

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
izbyshev
izbyshev reviewed Jan 14, 2019
View reviewed changes
Comment thread Lib/subprocess.py Outdated
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

@izbyshev izbyshev Jan 14, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jan 15, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Jan 14, 2019 via email

Copy link
Copy Markdown
Member Author
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@izbyshev

izbyshev commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jan 15, 2019

Copy link
Copy Markdown
Member Author

@giampaolo: Would you mind to review this change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner added 4 commits January 15, 2019 11:07
@vstinner
Don't use glibc POSIX implementation
634a523
@vstinner
Rephrase NEWS entry
2630276
@vstinner
Merge branch 'master' into subprocess_spawn
6474453
@vstinner
Document optimization in What's New in Python 3.8
f000375
giampaolo
giampaolo reviewed Jan 15, 2019
View reviewed changes

@giampaolo giampaolo left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Jan 15, 2019

Copy link
Copy Markdown
Member Author

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.
giampaolo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 9daecf3 into python:master Jan 15, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 15, 2019
@vstinner
vstinner deleted the subprocess_spawn branch January 15, 2019 23:02
@vstinner vstinner mentioned this pull request Jan 16, 2019
bpo-35537: subprocess can use posix_spawn with pipes #11575
Merged
@vstinner

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Jan 16, 2019
bpo-35537: use os.posix_spawn in subprocess #11242
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giampaolo giampaolo giampaolo left review comments
@gpshead gpshead Awaiting requested review from gpshead
+1 more reviewer
@izbyshev izbyshev izbyshev left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@vstinner @izbyshev @giampaolo @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.