Optimize some functions in std::process #31613
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This pushes the implementation detail of proxying `read_to_end` through to `read_to_end_uninitialized` all the way down to the `FileDesc` and `Handle` implementations on Unix/Windows. This way intermediate layers will also be able to take advantage of this optimized implementation. This commit also adds the optimized implementation for `ChildStdout` and `ChildStderr`.
For example if `Command::output` or `Command::status` is used then stdin is just immediately closed. Add an option for this so as an optimization we can avoid creating pipes entirely. This should help reduce the number of active file descriptors when spawning processes on Unix and the number of active handles on Windows.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
force-pushed
the
no-thread-spawns
branch
from
ccc2eac
to
b817acc
Compare
|
As some rationale for this as well, it's always bothered me that we've spawned threads in the standard library for this (seems a bit overkill), and I've also been curious lately as to our why our build times for Windows are so much higher than Unix (on the buildbots we have). I have done basically 0 investigation into why, but I figured it can't help to try to optimize |
Semantically there's actually no reason for us to spawn threads as part of the call to `wait_with_output`, and that's generally an incredibly heavyweight operation for just reading a few bytes (especially when stderr probably rarely has bytes!). An equivalent operation in terms of what's implemented today would be to just drain both pipes of all contents and then call `wait` on the child process itself. On Unix we can implement this through some convenient use of the `select` function, whereas on Windows we can make use of overlapped I/O. Note that on Windows this requires us to use named pipes instead of anonymous pipes, but they're semantically the same under the hood.
alexcrichton
force-pushed
the
no-thread-spawns
branch
from
b817acc
to
05c188d
Compare
|
well, as usual I figured I'd fully run the windows side tests after I pushed this up, but yet again it seems I have jumped the gun. Looks like there's some more pieces to disentangle there, so closing for now. |
bors
added a commit
that referenced
this pull request
Mar 9, 2016
Optimize some functions in std::process * Be sure that `read_to_end` gets directed towards `read_to_end_uninitialized` for all handles * When spawning a child that guaranteed doesn't need a stdin, don't actually create a stdin pipe for that process, instead just redirect it to /dev/null * When calling `wait_with_output`, don't spawn threads to read out the pipes of the child. Instead drain all pipes on the calling thread and *then* wait on the process. Functionally, it is intended that nothing changes as part of this PR --- Note that this was the same as #31613, and even after that it turned out that fixing Windows was easier than I thought! To copy a comment from over there: > As some rationale for this as well, it's always bothered me that we've spawned threads in the standard library for this (seems a bit overkill), and I've also been curious lately as to our why our build times for Windows are so much higher than Unix (on the buildbots we have). I have done basically 0 investigation into why, but I figured it can't help to try to optimize Command::output which I believe is called quite a few times during the test suite.
bors
added a commit
that referenced
this pull request
Mar 10, 2016
Optimize some functions in std::process * Be sure that `read_to_end` gets directed towards `read_to_end_uninitialized` for all handles * When spawning a child that guaranteed doesn't need a stdin, don't actually create a stdin pipe for that process, instead just redirect it to /dev/null * When calling `wait_with_output`, don't spawn threads to read out the pipes of the child. Instead drain all pipes on the calling thread and *then* wait on the process. Functionally, it is intended that nothing changes as part of this PR --- Note that this was the same as #31613, and even after that it turned out that fixing Windows was easier than I thought! To copy a comment from over there: > As some rationale for this as well, it's always bothered me that we've spawned threads in the standard library for this (seems a bit overkill), and I've also been curious lately as to our why our build times for Windows are so much higher than Unix (on the buildbots we have). I have done basically 0 investigation into why, but I figured it can't help to try to optimize Command::output which I believe is called quite a few times during the test suite.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
read_to_endgets directed towardsread_to_end_uninitializedfor all handleswait_with_output, don't spawn threads to read out the pipes of the child. Instead drain all pipes on the calling thread and then wait on the process.Functionally, it is intended that nothing changes as part of this PR