Skip to content

bpo-41586: Add pipesize parameter to subprocess. Add F_GETPIPE_SZ and F_SETPIPE_SZ to fcntl. #21921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Oct 19, 2020

Conversation

rhpvorderman
Copy link
Contributor

@rhpvorderman rhpvorderman commented Aug 19, 2020

A detailed performance comparison using subprocess and altering the pipesize can be found here: pycompression/xopen#35.

This PR adds a pipesize parameter to subprocess.Popen (and therefore also to subprocess.run etc. which pass arguments.) This will make it easier for users to control the size of the pipe as the default of 64K can be too small in HPC applications.

In order to enable this functionality. F_SETPIPE_SZ and F_GETPIPE_SZ were added to the fcntl module.

Platform compatibility has been taken care of:

  • Pipesize is only changed on systems that have the fcntl module, and where this module has the F_SETPIPE_SZ constant. As far as I know, this is only on Linux platforms.
  • On other platforms the pipesize will not be changed. No errors will be raised.

_winapi.CreatePipe can set the pipesize. This was not done however, because testing if the pipesize has changed requires GetNamedPipeInfo from the windows api, and this has not been incorporated in the _winapi. Since I never have worked with _winapi before and never have developed software on Windows, I am a bit hesitant to add this.

https://bugs.python.org/issue41586

This will allow the user to control the size of the pipes.
On linux the default is 64K. When a pipe is full it blocks for writing.
When a pipe is empty it blocks for reading. On processes that are
very fast this can lead to a lot of wasted CPU cycles. On a typical
Linux system the max pipe size is 1024K which is much better.
For high performance-oriented libraries such as xopen it is nice to
be able to set the pipe size.
The current workaround is to use my_popen_process.stdout.fileno() in
conjuction with fcntl and 1031 (value of F_SETPIPE_SZ) to acquire this
behavior.
Windows does support changing pipesize, but currently _winapi does not support measuring it. Hence the pipesize cannot be tested on this platform (yet).
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@rhpvorderman

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@rhpvorderman
Copy link
Contributor Author

I have signed the CLA, just now. So it may take some time before it is processed.

I am aware that this PR affects two different modules. If this is a problem, I am prepared to split this PR.

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

nice! Overall this looks good, some comments to address but nothing fundamental.

"F_SETPIPE_SZ and F_GETPIPE_SZ are not available on all unix platforms.")
def test_fcntl_f_pipesize(self):
test_pipe_r, test_pipe_w = os.pipe()
pipesize = 16 * 1024 # 64K is linux default.
Copy link
Member

Choose a reason for hiding this comment

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

minor nit:

rather than rely on an assumed default, use F_GETPIPE_SZ to determine the default. then set the value to half or double that and confirm that it worked.

Copy link
Contributor Author

@rhpvorderman rhpvorderman Aug 24, 2020

Choose a reason for hiding this comment

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

Great suggestion! I was struggling how to determine the default myself. os.sysconf values do not help here unfortunately. This is much easier. I feel a bit stupid for having missed this possibility.

@@ -625,6 +625,13 @@ functions.
* :data:`CREATE_DEFAULT_ERROR_MODE`
* :data:`CREATE_BREAKAWAY_FROM_JOB`

*pipesize* can be used to change the size of the pipe when
:data:`PIPE` is used for *stdin*, *stdout* or *stderr*. The size of the pipe
is only changed on platforms that support this.
Copy link
Member

Choose a reason for hiding this comment

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

mention something like (Linux at the time of this writing) at the end here.

that's enough of a documentation carrot to cause someone who wants this on Windows, macOS, or some BSD to pop up and contribute support on that platform in the future.

@gpshead gpshead self-assigned this Aug 21, 2020
@gpshead gpshead added the type-feature A feature request or enhancement label Aug 21, 2020
@rhpvorderman
Copy link
Contributor Author

@gpshead Thanks for the review. I have incorporated the necessary changes.

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 RHEL8 3.x has failed when building commit 23c0fb8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/529/builds/160) and take a look at the build logs.
  4. Check if the failure is related to this commit (23c0fb8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/529/builds/160

Failed tests:

  • test_subprocess
  • test_fcntl

Failed subtests:

  • test_fcntl_f_pipesize - test.test_fcntl.TestFcntl

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-aarch64/build/Lib/test/test_fcntl.py", line 200, in test_fcntl_f_pipesize
    fcntl.fcntl(test_pipe_w, fcntl.F_SETPIPE_SZ, pipesize_default * 2)
PermissionError: [Errno 1] Operation not permitted

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64 Fedora 3.x has failed when building commit 23c0fb8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/237/builds/220) and take a look at the build logs.
  4. Check if the failure is related to this commit (23c0fb8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/237/builds/220

Failed tests:

  • test_subprocess
  • test_fcntl

Failed subtests:

  • test_fcntl_f_pipesize - test.test_fcntl.TestFcntl

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_fcntl.py", line 200, in test_fcntl_f_pipesize
    fcntl.fcntl(test_pipe_w, fcntl.F_SETPIPE_SZ, pipesize_default * 2)
PermissionError: [Errno 1] Operation not permitted

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 3.x has failed when building commit 23c0fb8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/446/builds/199) and take a look at the build logs.
  4. Check if the failure is related to this commit (23c0fb8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/446/builds/199

Failed tests:

  • test_subprocess
  • test_fcntl

Failed subtests:

  • test_fcntl_f_pipesize - test.test_fcntl.TestFcntl

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_fcntl.py", line 200, in test_fcntl_f_pipesize
    fcntl.fcntl(test_pipe_w, fcntl.F_SETPIPE_SZ, pipesize_default * 2)
PermissionError: [Errno 1] Operation not permitted

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 LTO + PGO 3.x has failed when building commit 23c0fb8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/43/builds/219) and take a look at the build logs.
  4. Check if the failure is related to this commit (23c0fb8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/43/builds/219

Failed tests:

  • test_subprocess
  • test_fcntl

Failed subtests:

  • test_fcntl_f_pipesize - test.test_fcntl.TestFcntl

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto-pgo/build/Lib/test/test_fcntl.py", line 200, in test_fcntl_f_pipesize
    fcntl.fcntl(test_pipe_w, fcntl.F_SETPIPE_SZ, pipesize_default * 2)
PermissionError: [Errno 1] Operation not permitted

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 LTO 3.x has failed when building commit 23c0fb8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/503/builds/198) and take a look at the build logs.
  4. Check if the failure is related to this commit (23c0fb8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/503/builds/198

Failed tests:

  • test_subprocess
  • test_fcntl

Failed subtests:

  • test_fcntl_f_pipesize - test.test_fcntl.TestFcntl

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_fcntl.py", line 200, in test_fcntl_f_pipesize
    fcntl.fcntl(test_pipe_w, fcntl.F_SETPIPE_SZ, pipesize_default * 2)
PermissionError: [Errno 1] Operation not permitted

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…SETPIPE_SZ to fcntl. (pythonGH-21921)

* Add F_SETPIPE_SZ and F_GETPIPE_SZ to fcntl module
* Add pipesize parameter for subprocess.Popen class

This will allow the user to control the size of the pipes.
On linux the default is 64K. When a pipe is full it blocks for writing.
When a pipe is empty it blocks for reading. On processes that are
very fast this can lead to a lot of wasted CPU cycles. On a typical
Linux system the max pipe size is 1024K which is much better.
For high performance-oriented libraries such as xopen it is nice to
be able to set the pipe size.

The workaround without this feature is to use my_popen_process.stdout.fileno() in
conjuction with fcntl and 1031 (value of F_SETPIPE_SZ) to acquire this behavior.
@vstinner
Copy link
Member

vstinner commented Oct 6, 2023

The test now fails on s390x Fedora. It seems like the read end and the write end of the pipe don't have the same pipe buffer size. I wrote PR gh-110465 to fix that.

@rhpvorderman
Copy link
Contributor Author

@vstinner, amazing! Thanks!

@rhpvorderman rhpvorderman deleted the add_pipesize_subprocess branch October 6, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants