-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Conversation
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).
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 MissingOur records indicate the following people have not signed the CLA: 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 You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
fcntl is not available on Windows.
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. |
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.
nice! Overall this looks good, some comments to address but nothing fundamental.
Lib/test/test_fcntl.py
Outdated
"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. |
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.
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.
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.
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.
Doc/library/subprocess.rst
Outdated
@@ -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. |
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.
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 Thanks for the review. I have incorporated the necessary changes. |
|
|
|
|
|
…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.
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. |
@vstinner, amazing! Thanks! |
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 tosubprocess.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:
F_SETPIPE_SZ
constant. As far as I know, this is only on Linux platforms._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