Use posix_spawn() on unix if program is a path
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]: 5f72f9800b/posix/execvpe.c (L81)
Deny broken intra-doc links in linkchecker
Since rustdoc isn't warning about these links, check for them manually.
This also fixes the broken links that popped up from the lint.
stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union'
As [discussed by @SimonSapin and @withoutboats](https://github.com/rust-lang/rust/issues/55149#issuecomment-634692020), this PR proposes to stabilize parts of the `untagged_union` feature gate:
* It will be possible to have a union with field type `ManuallyDrop<T>` for any `T`.
* While at it I propose we also stabilize `impl Drop for Union`; to my knowledge, there are no open concerns around this feature.
In the RFC discussion, we also talked about allowing `&mut T` as another non-`Copy` non-dropping type, but that felt to me like an overly specific exception so I figured we'd wait if there is actually any use for such a special case.
Some things remain unstable and still require the `untagged_union` feature gate:
* Union with fields that do not drop, are not `Copy`, and are not `ManuallyDrop<_>`. The reason to not stabilize this is to avoid semver concerns around libraries adding `Drop` implementations later. (This is already not fully semver compatible as, to my knowledge, the borrow checker will exploit the non-dropping nature of any type, but it seems prudent to avoid further increasing the amount of trouble adding an `impl Drop` can cause.)
Due to this, quite a few tests still need the `untagged_union` feature, but I think the ones where I could remove the feature flag provide good test coverage for the stable part.
Cc @rust-lang/lang
Cleanup cloudabi mutexes and condvars
This gets rid of lots of unnecessary unsafety.
All the AtomicU32s were wrapped in UnsafeCell or UnsafeCell<MaybeUninit>, and raw pointers were used to get to the AtomicU32 inside. This change cleans that up by using AtomicU32 directly.
Also replaces a UnsafeCell<u32> by a safer Cell<u32>.
@rustbot modify labels: +C-cleanup
Static mutex is static
StaticMutex is only ever used with as a static (as the name already suggests). So it doesn't have to be generic over a lifetime, but can simply assume 'static.
This 'static lifetime guarantees the object is never moved, so this is no longer a manually checked requirement for unsafe calls to lock().
@rustbot modify labels: +T-libs +A-concurrency +C-cleanup
For backtrace, use StaticMutex instead of a raw sys Mutex.
The code used the very unsafe `sys::mutex::Mutex` directly, and built its own unlock-on-drop wrapper around it. The StaticMutex wrapper already provides that and is easier to use safely.
@rustbot modify labels: +T-libs +C-cleanup
Use futex-based thread-parker for Wasm32.
This uses the existing `sys_common/thread_parker/futex.rs` futex-based thread parker (that was already used for Linux) for wasm32 as well (if the wasm32 atomics target feature is enabled, which is not the case by default).
Wasm32 provides the basic futex operations as instructions: https://webassembly.github.io/threads/syntax/instructions.html
These are now exposed from `sys::futex::{futex_wait, futex_wake}`, just like on Linux. So, `thread_parker/futex.rs` stays completely unmodified.
Refactor io/buffered.rs into submodules
This pull request splits `BufWriter`, `BufReader`, `LineWriter`, and `LineWriterShim` (along with their associated tests) into separate submodules. It contains no functional changes. This change is being made in anticipation of adding another type of buffered writer which can be switched between line- and block-buffering mode.
Part of a series of pull requests resolving #60673.
StaticMutex is only ever used with as a static (as the name already
suggests). So it doesn't have to be generic over a lifetime, but can
simply assume 'static.
This 'static lifetime guarantees the object is never moved, so this is
no longer a manually checked requirement for unsafe calls to lock().
The comment said it's UB to call lock() while it is locked. That'd be
quite a useless Mutex. :) It was supposed to say 'locked by the same
thread', not just 'locked'.
Remove unsafety from sys/unsupported and add deny(unsafe_op_in_unsafe_fn).
Replacing `UnsafeCell`s by a `Cell`s simplifies things and makes the mutex and rwlock implementations safe. Other than that, only unsafety in strlen() contained unsafe code.
@rustbot modify labels: +F-unsafe-block-in-unsafe-fn +C-cleanup
Implement `AsRawFd` for `StdinLock` etc. on WASI.
WASI implements `AsRawFd` for `Stdin`, `Stdout`, and `Stderr`, so
implement it for `StdinLock`, `StdoutLock`, and `StderrLock` as well.
r? @alexcrichton
Avoid SeqCst or static mut in mach_timebase_info and QueryPerformanceFrequency caches
This patch went through a couple iterations but the end result is replacing a pattern where an `AtomicUsize` (updated with many SeqCst ops) guards a `static mut` with a single `AtomicU64` that is known to use 0 as a value indicating that it is not initialized.
The code in both places exists to cache values used in the conversion of Instants to Durations on macOS, iOS, and Windows.
I have no numbers to prove that this improves performance (It seems a little futile to benchmark something like this), but it's much simpler, safer, and in practice we'd expect it to be faster everywhere where Relaxed operations on AtomicU64 are cheaper than SeqCst operations on AtomicUsize, which is a lot of places.
Anyway, it also removes a bunch of unsafe code and greatly simplifies the logic, so IMO that alone would be worth it unless it was a regression.
If you want to take a look at the assembly output though, see https://godbolt.org/z/rbr6vn for x86_64, https://godbolt.org/z/cqcbqv for aarch64 (Note that this just the output of the mac side, but i'd expect the windows part to be the same and don't feel like doing another godbolt for it). There are several versions of this function in the godbolt:
- `info_new`: version in the current patch
- `info_less_new`: version in initial PR
- `info_original`: version currently in the tree
- `info_orig_but_better_orderings`: a version that just tries to change the original code's orderings from SeqCst to the (probably) minimal orderings required for soundness/correctness.
The biggest concern I have here is if we can use AtomicU64, or if there are targets that dont have it that this code supports. AFAICT: no. (If that changes in the future, it's easy enough to do something different for them)
r? `@Amanieu` because he caught a couple issues last time I tried to do a patch reducing orderings 😅
---
<details>
<summary>I rewrote this whole message so the original is inside here</summary>
I happened to notice the code we use for caching the result of mach_timebase_info uses SeqCst exclusively.
However, thinking a little more, it's actually pretty easy to avoid the static mut by packing the timebase info into an AtomicU64.
This entirely avoids needing to do the compare_exchange. The AtomicU64 can be read/written using Relaxed ops, which on current macos/ios platforms (x86_64/aarch64) have no overhead compared to direct loads/stores. This simplifies the code and makes it a lot safer too.
I have no numbers to prove that this improves performance (It seems a little futile to benchmark something like this), although it should do that on both targets it applies to.
That said, it also removes a bunch of unsafe code and simplifies the logic (arguably at least — there are only two states now, initialized or not), so I think it's a net win even without concrete numbers.
If you want to take a look at the assembly output though, see below. It has the new version, the original, and a version of the original with lower Orderings (which is still worse than the version in this PR)
- godbolt.org/z/obfqf9 x86_64-apple-darwin
- godbolt.org/z/Wz5cWc aarch64-unknown-linux-gnu (godbolt can't do aarch64-apple-ios but that doesn't matter here)
A different (and more efficient) option than this would be to just use the AtomicU64 and use the knowledge that after initialization the denominator should be nonzero... That felt like it's relying on too many things I'm not confident in, so I didn't want to do that.
</details>
doc: disambiguate stat in MetadataExt::as_raw_stat
A few architectures in `os::linux::raw` import `libc::stat`, rather than
defining that type directly. However, that also imports the _function_
called `stat`, which makes this doc link ambiguous:
error: `crate::os::linux::raw::stat` is both a struct and a function
--> library/std/src/os/linux/fs.rs:21:19
|
21 | /// [`stat`]: crate::os::linux::raw::stat
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ambiguous link
|
= note: `-D broken-intra-doc-links` implied by `-D warnings`
help: to link to the struct, prefix with the item type
|
21 | /// [`stat`]: struct@crate::os::linux::raw::stat
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to link to the function, add parentheses
|
21 | /// [`stat`]: crate::os::linux::raw::stat()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We want the `struct`, so it's now prefixed accordingly.
Link to documentation-specific guidelines.
Changed contribution information URL because it's not obvious how to get from the current URL to the documentation-specific content.
The current URL points to this "Getting Started" page, which contains nothing specific about documentation[*] and instead launches into how to *build* `rustc` which is not a strict prerequisite for contributing documentation fixes:
* https://rustc-dev-guide.rust-lang.org/getting-started.html
[*] The most specific content is a "Writing documentation" bullet point which is not itself a link to anything (I guess a patch for that might be helpful too).
### Why?
Making this change will make it easier for people who wish to make small "drive by" documentation fixes (and read contribution guidelines ;) ) which I find are often how I start contributing to a project. (Exhibit A: https://github.com/rust-lang/rust/pull/77050 :) )
### Background
My impression is the change of content linked is an unintentional change due to a couple of other changes:
* Originally, the link pointed to `contributing.md` which started with a "table of contents" linking to each section. But the content in `contributing.md` was removed and replaced with a link to the "Getting Started" section here:
* 3f6928f1f6 (diff-6a3371457528722a734f3c51d9238c13L1)
But the changed link doesn't actually point to the equivalent content, which is now located here:
* https://rustc-dev-guide.rust-lang.org/contributing.html
(If the "Guide to Rustc Development" is now considered the canonical location of "How to Contribute" content it might be a good idea to merge some of the "Contributing" Introduction section into the "Getting Started" section.)
* This was then compounded by changing the link from `contributing.md` to `contributing.html` here:
* https://github.com/rust-lang/rust/pull/74037/files#diff-242481015141f373dcb178e93cffa850L88
In order to even find the new location of the previous `contributing.md` content I ended up needing to do a GitHub search of the `rust-lang` org for the phrase "Documentation improvements are very welcome". :D
Fix error checking in posix_spawn implementation of Command
* Check for errors returned from posix_spawn*_init functions
* Check for non-zero return value from posix_spawn functions
A few architectures in `os::linux::raw` import `libc::stat`, rather than
defining that type directly. However, that also imports the _function_
called `stat`, which makes this doc link ambiguous:
error: `crate::os::linux::raw::stat` is both a struct and a function
--> library/std/src/os/linux/fs.rs:21:19
|
21 | /// [`stat`]: crate::os::linux::raw::stat
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ambiguous link
|
= note: `-D broken-intra-doc-links` implied by `-D warnings`
help: to link to the struct, prefix with the item type
|
21 | /// [`stat`]: struct@crate::os::linux::raw::stat
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to link to the function, add parentheses
|
21 | /// [`stat`]: crate::os::linux::raw::stat()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We want the `struct`, so it's now prefixed accordingly.
`DirEntry` contains a `ReadDir` handle, which used to just be a wrapper
on `Arc<InnerReadDir>`. Commit af75314ecd added `end_of_stream: bool`
which is not needed by `DirEntry`, but adds 8 bytes after padding. We
can let `DirEntry` have an `Arc<InnerReadDir>` directly to avoid that.
