Skip to content
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

Optimize some functions in std::process #31618

Merged
merged 3 commits into from
Mar 10, 2016

Conversation

alexcrichton
Copy link
Member

  • 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.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton alexcrichton force-pushed the no-thread-spawns branch 2 times, most recently from db8d041 to 63ca1d9 Compare February 20, 2016 01:01
@bors
Copy link
Contributor

bors commented Feb 23, 2016

☔ The latest upstream changes (presumably #31825) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 26, 2016

☔ The latest upstream changes (presumably #31858) made this pull request unmergeable. Please resolve the merge conflicts.

@brson
Copy link
Contributor

brson commented Mar 1, 2016

@bors r+ Glad to see that thread go.

@bors
Copy link
Contributor

bors commented Mar 1, 2016

📌 Commit 956de96 has been approved by brson

@bors
Copy link
Contributor

bors commented Mar 1, 2016

⌛ Testing commit 956de96 with merge 3365f3b...

@bors
Copy link
Contributor

bors commented Mar 1, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton
Copy link
Member Author

@bors: r=brson b49d4b1

@bors
Copy link
Contributor

bors commented Mar 1, 2016

⌛ Testing commit b49d4b1 with merge c2e412c...

@bors
Copy link
Contributor

bors commented Mar 1, 2016

💔 Test failed - auto-win-msvc-64-opt

@alexcrichton
Copy link
Member Author

@bors: r=brson 58e96a8

@bors
Copy link
Contributor

bors commented Mar 1, 2016

⌛ Testing commit 58e96a8 with merge 4274c2a...

@alexcrichton
Copy link
Member Author

@bors: retry force

@bors
Copy link
Contributor

bors commented Mar 1, 2016

⌛ Testing commit 58e96a8 with merge 1b87ace...

@bors
Copy link
Contributor

bors commented Mar 1, 2016

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member Author

@bors: r=brson 3ff611f

@bors
Copy link
Contributor

bors commented Mar 2, 2016

⌛ Testing commit 3ff611f with merge 3e4519e...

@alexcrichton
Copy link
Member Author

@bors: retry force

@bors
Copy link
Contributor

bors commented Mar 9, 2016

⌛ Testing commit 7c3038f with merge 9fbff29...

@bors
Copy link
Contributor

bors commented Mar 9, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member Author

@bors: retry

@bors
Copy link
Contributor

bors commented Mar 10, 2016

⌛ Testing commit 7c3038f with merge 0b9995b...

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.
@bors bors merged commit 7c3038f into rust-lang:master Mar 10, 2016
@alexcrichton alexcrichton deleted the no-thread-spawns branch March 10, 2016 04:17
@alexcrichton
Copy link
Member Author

😲

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 15, 2016
This regression was accidentally introduced in rust-lang#31618, and it's just flipping a
boolean!

Closes rust-lang#32254
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 26, 2016
…uron

std: Fix inheriting stdin on status()

This regression was accidentally introduced in rust-lang#31618, and it's just flipping a
boolean!

Closes rust-lang#32254
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 26, 2016
…uron

std: Fix inheriting stdin on status()

This regression was accidentally introduced in rust-lang#31618, and it's just flipping a
boolean!

Closes rust-lang#32254
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 4, 2017
This commit fixes a mistake introduced in rust-lang#31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes rust-lang#38811
nikomatsakis pushed a commit to nikomatsakis/rust that referenced this pull request Jan 6, 2017
This commit fixes a mistake introduced in rust-lang#31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes rust-lang#38811
bors added a commit that referenced this pull request Jan 6, 2017
std: Don't pass overlapped handles to processes

This commit fixes a mistake introduced in #31618 where overlapped handles were
leaked to child processes on Windows. On Windows once a handle is in overlapped
mode it should always have I/O executed with an instance of `OVERLAPPED`. Most
child processes, however, are not prepared to have their stdio handles in
overlapped mode as they don't use `OVERLAPPED` on reads/writes to the handle.

Now we haven't had any odd behavior in Rust up to this point, and the original
bug was introduced almost a year ago. I believe this is because it turns out
that if you *don't* pass an `OVERLAPPED` then the system will [supply one for
you][link]. In this case everything will go awry if you concurrently operate on
the handle. In Rust, however, the stdio handles are always locked, and there's
no way to not use them unlocked in libstd. Due to that change we've always had
synchronized access to these handles, which means that Rust programs typically
"just work".

Conversely, though, this commit fixes the test case included, which exhibits
behavior that other programs Rust spawns may attempt to execute. Namely, the
stdio handles may be concurrently used and having them in overlapped mode wreaks
havoc.

[link]: https://blogs.msdn.microsoft.com/oldnewthing/20121012-00/?p=6343

Closes #38811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants