This commit is a followup to https://github.com/rust-lang/rust/pull/124032. It
replaces the tests that test the various sort functions in the standard library
with a test-suite developed as part of
https://github.com/Voultapher/sort-research-rs. The current tests suffer a
couple of problems:
- They don't cover important real world patterns that the implementations take
advantage of and execute special code for.
- The input lengths tested miss out on code paths. For example, important safety
property tests never reach the quicksort part of the implementation.
- The miri side is often limited to `len <= 20` which means it very thoroughly
tests the insertion sort, which accounts for 19 out of 1.5k LoC.
- They are split into to core and alloc, causing code duplication and uneven
coverage.
- The randomness is not repeatable, as it
relies on `std:#️⃣:RandomState::new().build_hasher()`.
Most of these issues existed before
https://github.com/rust-lang/rust/pull/124032, but they are intensified by it.
One thing that is new and requires additional testing, is that the new sort
implementations specialize based on type properties. For example `Freeze` and
non `Freeze` execute different code paths.
Effectively there are three dimensions that matter:
- Input type
- Input length
- Input pattern
The ported test-suite tests various properties along all three dimensions,
greatly improving test coverage. It side-steps the miri issue by preferring
sampled approaches. For example the test that checks if after a panic the set of
elements is still the original one, doesn't do so for every single possible
panic opportunity but rather it picks one at random, and performs this test
across a range of input length, which varies the panic point across them. This
allows regular execution to easily test inputs of length 10k, and miri execution
up to 100 which covers significantly more code. The randomness used is tied to a
fixed - but random per process execution - seed. This allows for fully
repeatable tests and fuzzer like exploration across multiple runs.
Structure wise, the tests are previously found in the core integration tests for
`sort_unstable` and alloc unit tests for `sort`. The new test-suite was
developed to be a purely black-box approach, which makes integration testing the
better place, because it can't accidentally rely on internal access. Because
unwinding support is required the tests can't be in core, even if the
implementation is, so they are now part of the alloc integration tests. Are
there architectures that can only build and test core and not alloc? If so, do
such platforms require sort testing? For what it's worth the current
implementation state passes miri `--target mips64-unknown-linux-gnuabi64` which
is big endian.
The test-suite also contains tests for properties that were and are given by the
current and previous implementations, and likely relied upon by users but
weren't tested. For example `self_cmp` tests that the two parameters `a` and `b`
passed into the comparison function are never references to the same object,
which if the user is sorting for example a `&mut [Mutex<i32>]` could lead to a
deadlock.
Instead of using the hashed caller location as rand seed, it uses seconds since
unix epoch / 10, which given timestamps in the CI should be reasonably easy to
reproduce, but also allows fuzzer like space exploration.
This restores the original binary search implementation from #45333
which has the nice property of having a loop count that only depends on
the size of the slice. This, along with explicit conditional moves
from #128250, means that the entire binary search loop can be perfectly
predicted by the branch predictor.
Additionally, LLVM is able to unroll the loop when the slice length is
known at compile-time. This results in a very compact code sequence of
3-4 instructions per binary search step and zero branches.
Fixes#53823
Replace sort implementations
This PR replaces the sort implementations with tailor-made ones that strike a balance of run-time, compile-time and binary-size, yielding run-time and compile-time improvements. Regressing binary-size for `slice::sort` while improving it for `slice::sort_unstable`. All while upholding the existing soft and hard safety guarantees, and even extending the soft guarantees, detecting strict weak ordering violations with a high chance and reporting it to users via a panic.
* `slice::sort` -> driftsort [design document](https://github.com/Voultapher/sort-research-rs/blob/main/writeup/driftsort_introduction/text.md), includes detailed benchmarks and analysis.
* `slice::sort_unstable` -> ipnsort [design document](https://github.com/Voultapher/sort-research-rs/blob/main/writeup/ipnsort_introduction/text.md), includes detailed benchmarks and analysis.
#### Why should we change the sort implementations?
In the [2023 Rust survey](https://blog.rust-lang.org/2024/02/19/2023-Rust-Annual-Survey-2023-results.html#challenges), one of the questions was: "In your opinion, how should work on the following aspects of Rust be prioritized?". The second place was "Runtime performance" and the third one "Compile Times". This PR aims to improve both.
#### Why is this one big PR and not multiple?
* The current documentation gives performance recommendations for `slice::sort` and `slice::sort_unstable`. If for example only one of them were to be changed, this advice would be misleading for some Rust versions. By replacing them atomically, the advice remains largely unchanged, and users don't have to change their code.
* driftsort and ipnsort share a substantial part of their implementations.
* The implementation of `select_nth_unstable` uses internals of `slice::sort_unstable`, which makes it impractical to split changes.
---
This PR is a collaboration with `@orlp.`
- `slice::sort` -> driftsort
https://github.com/Voultapher/sort-research-rs/blob/main/writeup/driftsort_introduction/text.md
- `slice::sort_unstable` -> ipnsort
https://github.com/Voultapher/sort-research-rs/blob/main/writeup/ipnsort_introduction/text.md
Replaces the sort implementations with tailor made ones that strike a
balance of run-time, compile-time and binary-size, yielding run-time and
compile-time improvements. Regressing binary-size for `slice::sort`
while improving it for `slice::sort_unstable`. All while upholding the
existing soft and hard safety guarantees, and even extending the soft
guarantees, detecting strict weak ordering violations with a high chance
and reporting it to users via a panic.
In addition the implementation of `select_nth_unstable` is also adapted
as it uses `slice::sort_unstable` internals.
Use `bool` instead of `PartiolOrd` as return value of the comparison closure in `{slice,Iteraotr}::is_sorted_by`
Changes the function signature of the closure given to `{slice,Iteraotr}::is_sorted_by` to return a `bool` instead of a `PartiolOrd` as suggested by the libs-api team here: https://github.com/rust-lang/rust/issues/53485#issuecomment-1766411980.
This means these functions now return true if the closure returns true for all the pairs of values.
Stabilize `slice_first_last_chunk`
This PR does a few different things based around stabilizing `slice_first_last_chunk`. They are split up so this PR can be by-commit reviewed, I can move parts to a separate PR if desired.
This feature provides a very elegant API to extract arrays from either end of a slice, such as for parsing integers from binary data.
## Stabilize `slice_first_last_chunk`
ACP: https://github.com/rust-lang/libs-team/issues/69
Implementation: https://github.com/rust-lang/rust/issues/90091
Tracking issue: https://github.com/rust-lang/rust/issues/111774
This stabilizes the functionality from https://github.com/rust-lang/rust/issues/111774:
```rust
impl [T] {
pub const fn first_chunk<const N: usize>(&self) -> Option<&[T; N]>;
pub fn first_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
pub const fn last_chunk<const N: usize>(&self) -> Option<&[T; N]>;
pub fn last_chunk_mut<const N: usize>(&mut self) -> Option<&mut [T; N]>;
pub const fn split_first_chunk<const N: usize>(&self) -> Option<(&[T; N], &[T])>;
pub fn split_first_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T; N], &mut [T])>;
pub const fn split_last_chunk<const N: usize>(&self) -> Option<(&[T], &[T; N])>;
pub fn split_last_chunk_mut<const N: usize>(&mut self) -> Option<(&mut [T], &mut [T; N])>;
}
```
Const stabilization is included for all non-mut methods, which are blocked on `const_mut_refs`. This change includes marking the trivial function `slice_split_at_unchecked` const-stable for internal use (but not fully stable).
## Remove `split_array` slice methods
Tracking issue: https://github.com/rust-lang/rust/issues/90091
Implementation: https://github.com/rust-lang/rust/pull/83233#pullrequestreview-780315524
This PR also removes the following unstable methods from the `split_array` feature, https://github.com/rust-lang/rust/issues/90091:
```rust
impl<T> [T] {
pub fn split_array_ref<const N: usize>(&self) -> (&[T; N], &[T]);
pub fn split_array_mut<const N: usize>(&mut self) -> (&mut [T; N], &mut [T]);
pub fn rsplit_array_ref<const N: usize>(&self) -> (&[T], &[T; N]);
pub fn rsplit_array_mut<const N: usize>(&mut self) -> (&mut [T], &mut [T; N]);
}
```
This is done because discussion at #90091 and its implementation PR indicate a strong preference for nonpanicking APIs that return `Option`. The only difference between functions under the `split_array` and `slice_first_last_chunk` features is `Option` vs. panic, so remove the duplicates as part of this stabilization.
This does not affect the array methods from `split_array`. We will want to revisit these once `generic_const_exprs` is further along.
## Reverse order of return tuple for `split_last_chunk{,_mut}`
An unresolved question for #111774 is whether to return `(preceding_slice, last_chunk)` (`(&[T], &[T; N])`) or the reverse (`(&[T; N], &[T])`), from `split_last_chunk` and `split_last_chunk_mut`. It is currently implemented as `(last_chunk, preceding_slice)` which matches `split_last -> (&T, &[T])`. The first commit changes these to `(&[T], &[T; N])` for these reasons:
- More consistent with other splitting methods that return multiple values: `str::rsplit_once`, `slice::split_at{,_mut}`, `slice::align_to` all return tuples with the items in order
- More intuitive (arguably opinion, but it is consistent with other language elements like pattern matching `let [a, b, rest @ ..] ...`
- If we ever added a varidic way to obtain multiple chunks, it would likely return something in order: `.split_many_last::<(2, 4)>() -> (&[T], &[T; 2], &[T; 4])`
- It is the ordering used in the `rsplit_array` methods
I think the inconsistency with `split_last` could be acceptable in this case, since for `split_last` the scalar `&T` doesn't have any internal order to maintain with the other items.
## Unresolved questions
Do we want to reserve the same names on `[u8; N]` to avoid inference confusion? https://github.com/rust-lang/rust/pull/117561#issuecomment-1793388647
---
`slice_first_last_chunk` has only been around since early 2023, but `split_array` has been around since 2021.
`@rustbot` label -T-libs +T-libs-api -T-libs +needs-fcp
cc `@rust-lang/wg-const-eval,` `@scottmcm` who raised this topic, `@clarfonthey` implementer of `slice_first_last_chunk` `@jethrogb` implementer of `split_array`
Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Stabilizing.20array-from-slice.20*something*.3FFixes: #111774
detects redundant imports that can be eliminated.
for #117772 :
In order to facilitate review and modification, split the checking code and
removing redundant imports code into two PR.
The functionality of these methods from `split_array` has been absorbed by the
`slice_first_last_chunk` feature. This only affects the methods on slices,
not those with the same name that are implemented on array types.
Also adjusts testing to reflect this change.
Implement `slice::split_once` and `slice::rsplit_once`
Feature gate is `slice_split_once` and tracking issue is #112811. These are equivalents to the existing `str::split_once` and `str::rsplit_once` methods.
A successful advance is now signalled by returning `0` and other values now represent the remaining number
of steps that couldn't be advanced as opposed to the amount of steps that have been advanced during a partial advance_by.
This simplifies adapters a bit, replacing some `match`/`if` with arithmetic. Whether this is beneficial overall depends
on whether `advance_by` is mostly used as a building-block for other iterator methods and adapters or whether
we also see uses by users where `Result` might be more useful.
Carefully remove bounds checks from some chunk iterator functions
So, I was writing code that requires the equivalent of `rchunks(N).rev()` (which isn't the same as forward `chunks(N)` — in particular, if the buffer length is not a multiple of `N`, I must handle the "remainder" first).
I happened to look at the codegen output of the function (I was actually interested in whether or not a nested loop was being unrolled — it was), and noticed that in the outer `rchunks(n).rev()` loop, LLVM seemed to be unable to remove the bounds checks from the iteration: https://rust.godbolt.org/z/Tnz4MYY8f (this panic was from the split_at in `RChunks::next_back`).
After doing some experimentation, it seems all of the `next_back` in the non-exact chunk iterators have the issue: (`Chunks::next_back`, `RChunks::next_back`, `ChunksMut::next_back`, and `RChunksMut::next_back`)...
Even worse, the forward `rchunks` iterators sometimes have the issue as well (... but only sometimes). For example https://rust.godbolt.org/z/oGhbqv53r has bounds checks, but if I uncomment the loop body, it manages to remove the check (which is bizarre, since I'd expect the opposite...). I suspect it's highly dependent on the surrounding code, so I decided to remove the bounds checks from them anyway. Overall, this change includes:
- All `next_back` functions on the non-`Exact` iterators (e.g. `R?Chunks(Mut)?`).
- All `next` functions on the non-exact rchunks iterators (e.g. `RChunks(Mut)?`).
I wasn't able to catch any of the other chunk iterators failing to remove the bounds checks (I checked iterations over `r?chunks(_exact)?(_mut)?` with constant chunk sizes under `-O3`, `-Os`, and `-Oz`), which makes sense, since these were the cases where it was harder to prove the bounds check correct to remove...
In fact, it took quite a bit of thinking to convince myself that using unchecked_ here was valid — so I'm not really surprised that LLVM had trouble (although compilers are slightly better at this sort of reasoning than humans). A consequence of that is the fact that the `// SAFETY` comment for these are... kinda long...
---
I didn't do this for, or even think about it for, any of the other iteration methods; just `next` and `next_back` (where it mattered). If this PR is accepted, I'll file a follow up for someone (possibly me) to look at the others later (in particular, `nth`/`nth_back` looked like they had similar logic), but I wanted to do this now, as IMO `next`/`next_back` are the most important here, since they're what gets used by the iteration protocol.
---
Note: While I don't expect this to impact performance directly, the panic is a side effect, which would otherwise not exist in these loops. That is, this could prevent the compiler from being able to move/remove/otherwise rework a loop over these iterators (as an example, it could not delete the code for a loop whose body computes a value which doesn't get used).
Also, some like to be able to have confidence this code has no panicking branches in the optimized code, and "no bounds checks" is kinda part of the selling point of Rust's iterators anyway.
The `advance_by(n)` docs state that in the error case `Err(k)` that k is always less than n.
It also states that `advance_by(0)` may return `Err(0)` to indicate an exhausted iterator.
These statements are inconsistent.
Since only one implementation (Skip) actually made use of that I changed it to return Ok(()) in that case too.
While adding some tests I also found a bug in `Take::advance_back_by`.
