Skip to content

Commit 0b9995b

Browse files
borsbors
committed
Auto merge of #31618 - alexcrichton:no-thread-spawns, r=brson
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.
2 parents cc62db8 + 7c3038f commit 0b9995b

19 files changed

+609
-53
lines changed

‎src/liblibc

‎src/libstd/fs.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ use ffi::OsString;
2222
use io::{self, SeekFrom, Seek, Read, Write};
2323
use path::{Path, PathBuf};
2424
use sys::fs as fs_imp;
25-
use sys_common::io::read_to_end_uninitialized;
2625
use sys_common::{AsInnerMut, FromInner, AsInner, IntoInner};
2726
use vec::Vec;
2827
use time::SystemTime;
@@ -351,7 +350,7 @@ impl Read for File {
351350
self.inner.read(buf)
352351
}
353352
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
354-
unsafe { read_to_end_uninitialized(self, buf) }
353+
self.inner.read_to_end(buf)
355354
}
356355
}
357356
#[stable(feature = "rust1", since = "1.0.0")]
@@ -372,6 +371,9 @@ impl<'a> Read for &'a File {
372371
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
373372
self.inner.read(buf)
374373
}
374+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
375+
self.inner.read_to_end(buf)
376+
}
375377
}
376378
#[stable(feature = "rust1", since = "1.0.0")]
377379
impl<'a> Write for &'a File {

‎src/libstd/io/stdio.rs

+10-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ use io::lazy::Lazy;
1818
use io::{self, BufReader, LineWriter};
1919
use sync::{Arc, Mutex, MutexGuard};
2020
use sys::stdio;
21-
use sys_common::io::{read_to_end_uninitialized};
2221
use sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
2322
use thread::LocalKeyState;
2423

@@ -78,6 +77,9 @@ fn stderr_raw() -> io::Result<StderrRaw> { stdio::Stderr::new().map(StderrRaw) }
7877

7978
impl Read for StdinRaw {
8079
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }
80+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
81+
self.0.read_to_end(buf)
82+
}
8183
}
8284
impl Write for StdoutRaw {
8385
fn write(&mut self, buf: &[u8]) -> io::Result<usize> { self.0.write(buf) }
@@ -116,6 +118,12 @@ impl<R: io::Read> io::Read for Maybe<R> {
116118
Maybe::Fake => Ok(0)
117119
}
118120
}
121+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
122+
match *self {
123+
Maybe::Real(ref mut r) => handle_ebadf(r.read_to_end(buf), 0),
124+
Maybe::Fake => Ok(0)
125+
}
126+
}
119127
}
120128

121129
fn handle_ebadf<T>(r: io::Result<T>, default: T) -> io::Result<T> {
@@ -294,7 +302,7 @@ impl<'a> Read for StdinLock<'a> {
294302
self.inner.read(buf)
295303
}
296304
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
297-
unsafe { read_to_end_uninitialized(self, buf) }
305+
self.inner.read_to_end(buf)
298306
}
299307
}
300308

‎src/libstd/net/tcp.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ use io::prelude::*;
1414
use fmt;
1515
use io;
1616
use net::{ToSocketAddrs, SocketAddr, Shutdown};
17-
use sys_common::io::read_to_end_uninitialized;
1817
use sys_common::net as net_imp;
1918
use sys_common::{AsInner, FromInner, IntoInner};
2019
use time::Duration;
@@ -269,7 +268,7 @@ impl TcpStream {
269268
impl Read for TcpStream {
270269
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }
271270
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
272-
unsafe { read_to_end_uninitialized(self, buf) }
271+
self.0.read_to_end(buf)
273272
}
274273
}
275274
#[stable(feature = "rust1", since = "1.0.0")]
@@ -281,7 +280,7 @@ impl Write for TcpStream {
281280
impl<'a> Read for &'a TcpStream {
282281
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }
283282
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
284-
unsafe { read_to_end_uninitialized(self, buf) }
283+
self.0.read_to_end(buf)
285284
}
286285
}
287286
#[stable(feature = "rust1", since = "1.0.0")]

‎src/libstd/process.rs

+30-19
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,9 @@ use fmt;
2020
use io;
2121
use path::Path;
2222
use str;
23-
use sys::pipe::AnonPipe;
23+
use sys::pipe::{read2, AnonPipe};
2424
use sys::process as imp;
2525
use sys_common::{AsInner, AsInnerMut, FromInner, IntoInner};
26-
use thread::{self, JoinHandle};
2726

2827
/// Representation of a running or exited child process.
2928
///
@@ -134,6 +133,9 @@ impl Read for ChildStdout {
134133
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
135134
self.inner.read(buf)
136135
}
136+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
137+
self.inner.read_to_end(buf)
138+
}
137139
}
138140

139141
impl AsInner<AnonPipe> for ChildStdout {
@@ -161,6 +163,9 @@ impl Read for ChildStderr {
161163
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
162164
self.inner.read(buf)
163165
}
166+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
167+
self.inner.read_to_end(buf)
168+
}
164169
}
165170

166171
impl AsInner<AnonPipe> for ChildStderr {
@@ -289,7 +294,7 @@ impl Command {
289294
/// By default, stdin, stdout and stderr are inherited from the parent.
290295
#[stable(feature = "process", since = "1.0.0")]
291296
pub fn spawn(&mut self) -> io::Result<Child> {
292-
self.inner.spawn(imp::Stdio::Inherit).map(Child::from_inner)
297+
self.inner.spawn(imp::Stdio::Inherit, true).map(Child::from_inner)
293298
}
294299

295300
/// Executes the command as a child process, waiting for it to finish and
@@ -312,7 +317,7 @@ impl Command {
312317
/// ```
313318
#[stable(feature = "process", since = "1.0.0")]
314319
pub fn output(&mut self) -> io::Result<Output> {
315-
self.inner.spawn(imp::Stdio::MakePipe).map(Child::from_inner)
320+
self.inner.spawn(imp::Stdio::MakePipe, false).map(Child::from_inner)
316321
.and_then(|p| p.wait_with_output())
317322
}
318323

@@ -334,7 +339,8 @@ impl Command {
334339
/// ```
335340
#[stable(feature = "process", since = "1.0.0")]
336341
pub fn status(&mut self) -> io::Result<ExitStatus> {
337-
self.spawn().and_then(|mut p| p.wait())
342+
self.inner.spawn(imp::Stdio::Inherit, false).map(Child::from_inner)
343+
.and_then(|mut p| p.wait())
338344
}
339345
}
340346

@@ -496,24 +502,29 @@ impl Child {
496502
#[stable(feature = "process", since = "1.0.0")]
497503
pub fn wait_with_output(mut self) -> io::Result<Output> {
498504
drop(self.stdin.take());
499-
fn read<R>(mut input: R) -> JoinHandle<io::Result<Vec<u8>>>
500-
where R: Read + Send + 'static
501-
{
502-
thread::spawn(move || {
503-
let mut ret = Vec::new();
504-
input.read_to_end(&mut ret).map(|_| ret)
505-
})
505+
506+
let (mut stdout, mut stderr) = (Vec::new(), Vec::new());
507+
match (self.stdout.take(), self.stderr.take()) {
508+
(None, None) => {}
509+
(Some(mut out), None) => {
510+
let res = out.read_to_end(&mut stdout);
511+
res.unwrap();
512+
}
513+
(None, Some(mut err)) => {
514+
let res = err.read_to_end(&mut stderr);
515+
res.unwrap();
516+
}
517+
(Some(out), Some(err)) => {
518+
let res = read2(out.inner, &mut stdout, err.inner, &mut stderr);
519+
res.unwrap();
520+
}
506521
}
507-
let stdout = self.stdout.take().map(read);
508-
let stderr = self.stderr.take().map(read);
509-
let status = try!(self.wait());
510-
let stdout = stdout.and_then(|t| t.join().unwrap().ok());
511-
let stderr = stderr.and_then(|t| t.join().unwrap().ok());
512522

523+
let status = try!(self.wait());
513524
Ok(Output {
514525
status: status,
515-
stdout: stdout.unwrap_or(Vec::new()),
516-
stderr: stderr.unwrap_or(Vec::new()),
526+
stdout: stdout,
527+
stderr: stderr,
517528
})
518529
}
519530
}

‎src/libstd/sys/common/net.rs

+4
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,10 @@ impl TcpStream {
225225
self.inner.read(buf)
226226
}
227227

228+
pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
229+
self.inner.read_to_end(buf)
230+
}
231+
228232
pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
229233
let len = cmp::min(buf.len(), <wrlen_t>::max_value() as usize) as wrlen_t;
230234
let ret = try!(cvt(unsafe {

‎src/libstd/sys/unix/fd.rs

+36-2
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,17 @@
88
// option. This file may not be copied, modified, or distributed
99
// except according to those terms.
1010

11-
use io;
11+
#![unstable(reason = "not public", issue = "0", feature = "fd")]
12+
13+
use prelude::v1::*;
14+
15+
use io::{self, Read};
1216
use libc::{self, c_int, size_t, c_void};
1317
use mem;
18+
use sync::atomic::{AtomicBool, Ordering};
1419
use sys::cvt;
1520
use sys_common::AsInner;
16-
use sync::atomic::{AtomicBool, Ordering};
21+
use sys_common::io::read_to_end_uninitialized;
1722

1823
pub struct FileDesc {
1924
fd: c_int,
@@ -42,6 +47,11 @@ impl FileDesc {
4247
Ok(ret as usize)
4348
}
4449

50+
pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
51+
let mut me = self;
52+
(&mut me).read_to_end(buf)
53+
}
54+
4555
pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
4656
let ret = try!(cvt(unsafe {
4757
libc::write(self.fd,
@@ -67,6 +77,20 @@ impl FileDesc {
6777
}
6878
}
6979

80+
pub fn set_nonblocking(&self, nonblocking: bool) {
81+
unsafe {
82+
let previous = libc::fcntl(self.fd, libc::F_GETFL);
83+
debug_assert!(previous != -1);
84+
let new = if nonblocking {
85+
previous | libc::O_NONBLOCK
86+
} else {
87+
previous & !libc::O_NONBLOCK
88+
};
89+
let ret = libc::fcntl(self.fd, libc::F_SETFL, new);
90+
debug_assert!(ret != -1);
91+
}
92+
}
93+
7094
pub fn duplicate(&self) -> io::Result<FileDesc> {
7195
// We want to atomically duplicate this file descriptor and set the
7296
// CLOEXEC flag, and currently that's done via F_DUPFD_CLOEXEC. This
@@ -118,6 +142,16 @@ impl FileDesc {
118142
}
119143
}
120144

145+
impl<'a> Read for &'a FileDesc {
146+
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
147+
(**self).read(buf)
148+
}
149+
150+
fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {
151+
unsafe { read_to_end_uninitialized(self, buf) }
152+
}
153+
}
154+
121155
impl AsInner<c_int> for FileDesc {
122156
fn as_inner(&self) -> &c_int { &self.fd }
123157
}

‎src/libstd/sys/unix/fs.rs

+4
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,10 @@ impl File {
486486
self.0.read(buf)
487487
}
488488

489+
pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
490+
self.0.read_to_end(buf)
491+
}
492+
489493
pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
490494
self.0.write(buf)
491495
}

‎src/libstd/sys/unix/net.rs

+4
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,10 @@ impl Socket {
116116
self.0.read(buf)
117117
}
118118

119+
pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
120+
self.0.read_to_end(buf)
121+
}
122+
119123
pub fn set_timeout(&self, dur: Option<Duration>, kind: libc::c_int) -> io::Result<()> {
120124
let timeout = match dur {
121125
Some(dur) => {

‎src/libstd/sys/unix/pipe.rs

+59
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,12 @@
88
// option. This file may not be copied, modified, or distributed
99
// except according to those terms.
1010

11+
use prelude::v1::*;
12+
13+
use cmp;
1114
use io;
1215
use libc::{self, c_int};
16+
use mem;
1317
use sys::cvt_r;
1418
use sys::fd::FileDesc;
1519

@@ -57,10 +61,65 @@ impl AnonPipe {
5761
self.0.read(buf)
5862
}
5963

64+
pub fn read_to_end(&self, buf: &mut Vec<u8>) -> io::Result<usize> {
65+
self.0.read_to_end(buf)
66+
}
67+
6068
pub fn write(&self, buf: &[u8]) -> io::Result<usize> {
6169
self.0.write(buf)
6270
}
6371

6472
pub fn fd(&self) -> &FileDesc { &self.0 }
6573
pub fn into_fd(self) -> FileDesc { self.0 }
6674
}
75+
76+
pub fn read2(p1: AnonPipe,
77+
v1: &mut Vec<u8>,
78+
p2: AnonPipe,
79+
v2: &mut Vec<u8>) -> io::Result<()> {
80+
// Set both pipes into nonblocking mode as we're gonna be reading from both
81+
// in the `select` loop below, and we wouldn't want one to block the other!
82+
let p1 = p1.into_fd();
83+
let p2 = p2.into_fd();
84+
p1.set_nonblocking(true);
85+
p2.set_nonblocking(true);
86+
87+
let max = cmp::max(p1.raw(), p2.raw());
88+
loop {
89+
// wait for either pipe to become readable using `select`
90+
try!(cvt_r(|| unsafe {
91+
let mut read: libc::fd_set = mem::zeroed();
92+
libc::FD_SET(p1.raw(), &mut read);
93+
libc::FD_SET(p2.raw(), &mut read);
94+
libc::select(max + 1, &mut read, 0 as *mut _, 0 as *mut _,
95+
0 as *mut _)
96+
}));
97+
98+
// Read as much as we can from each pipe, ignoring EWOULDBLOCK or
99+
// EAGAIN. If we hit EOF, then this will happen because the underlying
100+
// reader will return Ok(0), in which case we'll see `Ok` ourselves. In
101+
// this case we flip the other fd back into blocking mode and read
102+
// whatever's leftover on that file descriptor.
103+
let read = |fd: &FileDesc, dst: &mut Vec<u8>| {
104+
match fd.read_to_end(dst) {
105+
Ok(_) => Ok(true),
106+
Err(e) => {
107+
if e.raw_os_error() == Some(libc::EWOULDBLOCK) ||
108+
e.raw_os_error() == Some(libc::EAGAIN) {
109+
Ok(false)
110+
} else {
111+
Err(e)
112+
}
113+
}
114+
}
115+
};
116+
if try!(read(&p1, v1)) {
117+
p2.set_nonblocking(false);
118+
return p2.read_to_end(v2).map(|_| ());
119+
}
120+
if try!(read(&p2, v2)) {
121+
p1.set_nonblocking(false);
122+
return p1.read_to_end(v1).map(|_| ());
123+
}
124+
}
125+
}

0 commit comments

Comments
 (0)