From 8fe61546696b626ecf68ef838d5d82e393719e80 Mon Sep 17 00:00:00 2001
From: Alexander Mols <asm@fb.com>
Date: Fri, 2 Oct 2020 07:44:32 -0700
Subject: [PATCH] Use posix_spawn() on unix if program is a path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Previously `Command::spawn` would fall back to the non-posix_spawn based
implementation if the `PATH` environment variable was possibly changed.
On systems with a modern (g)libc `posix_spawn()` can be significantly
faster. If program is a path itself the `PATH` environment variable is
not used for the lookup and it should be safe to use the
`posix_spawnp()` method. [1]

We found this, because we have a cli application that effectively runs a
lot of subprocesses. It would sometimes noticeably hang while printing
output. Profiling showed that the process was spending the majority of
time in the kernel's `copy_page_range` function while spawning
subprocesses. During this time the process is completely blocked from
running, explaining why users were reporting the cli app hanging.

Through this we discovered that `std::process::Command` has a fast and
slow path for process execution. The fast path is backed by
`posix_spawnp()` and the slow path by fork/exec syscalls being called
explicitly. Using fork for process creation is supposed to be fast, but
it slows down as your process uses more memory.  It's not because the
kernel copies the actual memory from the parent, but it does need to
copy the references to it (see `copy_page_range` above!).  We ended up
using the slow path, because the command spawn implementation in falls
back to the slow path if it suspects the PATH environment variable was
changed.

Here is a smallish program demonstrating the slowdown before this code
change:

```
use std::process::Command;
use std::time::Instant;

fn main() {
    let mut args = std::env::args().skip(1);
    if let Some(size) = args.next() {
        // Allocate some memory
        let _xs: Vec<_> = std::iter::repeat(0)
            .take(size.parse().expect("valid number"))
            .collect();

        let mut command = Command::new("/bin/sh");
        command
            .arg("-c")
            .arg("echo hello");

        if args.next().is_some() {
            println!("Overriding PATH");
            command.env("PATH", std::env::var("PATH").expect("PATH env var"));
        }

        let now = Instant::now();
        let child = command
            .spawn()
            .expect("failed to execute process");

        println!("Spawn took: {:?}", now.elapsed());

        let output = child.wait_with_output().expect("failed to wait on process");
        println!("Output: {:?}", output);
    } else {
        eprintln!("Usage: prog [size]");
        std::process::exit(1);
    }
    ()
}
```

Running it and passing different amounts of elements to use to allocate
memory shows that the time taken for `spawn()` can differ quite
significantly. In latter case the `posix_spawnp()` implementation is 30x
faster:

```
$ cargo run --release 10000000
...
Spawn took: 324.275µs
hello
$ cargo run --release 10000000 changepath
...
Overriding PATH
Spawn took: 2.346809ms
hello
$ cargo run --release 100000000
...
Spawn took: 387.842µs
hello
$ cargo run --release 100000000 changepath
...
Overriding PATH
Spawn took: 13.434677ms
hello
```

[1]: https://github.com/bminor/glibc/blob/5f72f9800b250410cad3abfeeb09469ef12b2438/posix/execvpe.c#L81
---
 library/std/src/sys/unix/process/process_common.rs | 6 ++++++
 library/std/src/sys/unix/process/process_unix.rs   | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/library/std/src/sys/unix/process/process_common.rs b/library/std/src/sys/unix/process/process_common.rs
index 9ddd4ad4000..2a384d8817c 100644
--- a/library/std/src/sys/unix/process/process_common.rs
+++ b/library/std/src/sys/unix/process/process_common.rs
@@ -253,11 +253,17 @@ impl Command {
         let maybe_env = self.env.capture_if_changed();
         maybe_env.map(|env| construct_envp(env, &mut self.saw_nul))
     }
+
     #[allow(dead_code)]
     pub fn env_saw_path(&self) -> bool {
         self.env.have_changed_path()
     }
 
+    #[allow(dead_code)]
+    pub fn program_is_path(&self) -> bool {
+        self.program.to_bytes().contains(&b'/')
+    }
+
     pub fn setup_io(
         &self,
         default: Stdio,
diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs
index ea7ea6f067c..6be71980810 100644
--- a/library/std/src/sys/unix/process/process_unix.rs
+++ b/library/std/src/sys/unix/process/process_unix.rs
@@ -279,7 +279,7 @@ impl Command {
 
         if self.get_gid().is_some()
             || self.get_uid().is_some()
-            || self.env_saw_path()
+            || (self.env_saw_path() && !self.program_is_path())
             || !self.get_closures().is_empty()
         {
             return Ok(None);