diff options
| author | Matthias Krüger <matthias.krueger@famsik.de> | 2024-12-11 20:00:14 +0100 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-12-11 20:00:14 +0100 |
| commit | 2e60288ce0b317134779d95daf80df601a4c17c5 (patch) | |
| tree | 01767cbe358ec89c0dae0a83ee10c99c04f760b7 | |
| parent | fe516ef9f4dc2152ad970093107630a8727a2772 (diff) | |
| parent | fa87a3e88942e407a2214728bb466620f538d13d (diff) | |
Rollup merge of #133598 - ChayimFriedman2:get-many-mut-detailed-err, r=scottmcm
Change `GetManyMutError` to match T-libs-api decision
That is, differentiate between out-of-bounds and overlapping indices, and remove the generic parameter `N`.
I also exported `GetManyMutError` from `alloc` (and `std`), which was apparently forgotten.
Changing the error to carry additional details means LLVM no longer generates separate short-circuiting branches for the checks, instead it generates one branch at the end. I therefore changed the code to use early returns to make LLVM generate jumps. Benchmark results between the approaches are somewhat mixed, but I chose this approach because it is significantly faster with ranges and also faster with `unwrap()`.
Benchmark (`jumps` refer to short-circuiting, `acc` is not short-circuiting):
```rust
use criterion::{black_box, criterion_group, criterion_main, Criterion};
use my_crate::{get_many_check_valid_acc, get_many_check_valid_jumps, GetManyMutError};
mod externs {
#[unsafe(no_mangle)]
fn foo() {}
#[unsafe(no_mangle)]
fn bar() {}
#[unsafe(no_mangle)]
fn baz() {}
}
unsafe extern "C" {
safe fn foo();
safe fn bar();
safe fn baz();
}
fn bench_method(c: &mut Criterion) {
c.bench_function("jumps two usize", |b| {
b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10)))
});
c.bench_function("jumps two usize unwrap", |b| {
b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10)).unwrap())
});
c.bench_function("jumps two usize ok", |b| {
b.iter(|| get_many_check_valid_jumps(&[black_box(1), black_box(5)], black_box(10)).ok())
});
c.bench_function("jumps three usize", |b| {
b.iter(|| {
get_many_check_valid_jumps(&[black_box(1), black_box(5), black_box(7)], black_box(10))
})
});
c.bench_function("jumps three usize match", |b| {
b.iter(|| {
match get_many_check_valid_jumps(
&[black_box(1), black_box(5), black_box(7)],
black_box(10),
) {
Err(GetManyMutError::IndexOutOfBounds) => foo(),
Err(GetManyMutError::OverlappingIndices) => bar(),
Ok(()) => baz(),
}
})
});
c.bench_function("jumps two Range", |b| {
b.iter(|| {
get_many_check_valid_jumps(
&[black_box(1)..black_box(5), black_box(7)..black_box(8)],
black_box(10),
)
})
});
c.bench_function("jumps two RangeInclusive", |b| {
b.iter(|| {
get_many_check_valid_jumps(
&[black_box(1)..=black_box(5), black_box(7)..=black_box(8)],
black_box(10),
)
})
});
c.bench_function("acc two usize", |b| {
b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10)))
});
c.bench_function("acc two usize unwrap", |b| {
b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10)).unwrap())
});
c.bench_function("acc two usize ok", |b| {
b.iter(|| get_many_check_valid_acc(&[black_box(1), black_box(5)], black_box(10)).ok())
});
c.bench_function("acc three usize", |b| {
b.iter(|| {
get_many_check_valid_acc(&[black_box(1), black_box(5), black_box(7)], black_box(10))
})
});
c.bench_function("acc three usize match", |b| {
b.iter(|| {
match get_many_check_valid_jumps(
&[black_box(1), black_box(5), black_box(7)],
black_box(10),
) {
Err(GetManyMutError::IndexOutOfBounds) => foo(),
Err(GetManyMutError::OverlappingIndices) => bar(),
Ok(()) => baz(),
}
})
});
c.bench_function("acc two Range", |b| {
b.iter(|| {
get_many_check_valid_acc(
&[black_box(1)..black_box(5), black_box(7)..black_box(8)],
black_box(10),
)
})
});
c.bench_function("acc two RangeInclusive", |b| {
b.iter(|| {
get_many_check_valid_acc(
&[black_box(1)..=black_box(5), black_box(7)..=black_box(8)],
black_box(10),
)
})
});
}
criterion_group!(benches, bench_method);
criterion_main!(benches);
```
Benchmark results:
```none
jumps two usize time: [586.44 ps 590.20 ps 594.50 ps]
jumps two usize unwrap time: [390.44 ps 393.63 ps 397.44 ps]
jumps two usize ok time: [585.52 ps 591.74 ps 599.38 ps]
jumps three usize time: [976.51 ps 983.79 ps 991.51 ps]
jumps three usize match time: [390.82 ps 393.80 ps 397.07 ps]
jumps two Range time: [1.2583 ns 1.2640 ns 1.2695 ns]
jumps two RangeInclusive time: [1.2673 ns 1.2770 ns 1.2877 ns]
acc two usize time: [592.63 ps 596.44 ps 600.52 ps]
acc two usize unwrap time: [582.65 ps 587.07 ps 591.90 ps]
acc two usize ok time: [581.59 ps 587.82 ps 595.71 ps]
acc three usize time: [894.69 ps 901.23 ps 908.24 ps]
acc three usize match time: [392.68 ps 395.73 ps 399.17 ps]
acc two Range time: [1.5531 ns 1.5617 ns 1.5711 ns]
acc two RangeInclusive time: [1.5746 ns 1.5840 ns 1.5939 ns]
```
| -rw-r--r-- | library/alloc/src/slice.rs | 2 | ||||
| -rw-r--r-- | library/core/src/error.rs | 2 | ||||
| -rw-r--r-- | library/core/src/slice/mod.rs | 58 |
3 files changed, 34 insertions, 28 deletions
diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index e3c7835f1d1..edc8d99f2f9 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -27,6 +27,8 @@ pub use core::slice::ArrayChunksMut; pub use core::slice::ArrayWindows; #[stable(feature = "inherent_ascii_escape", since = "1.60.0")] pub use core::slice::EscapeAscii; +#[unstable(feature = "get_many_mut", issue = "104642")] +pub use core::slice::GetManyMutError; #[stable(feature = "slice_get_slice", since = "1.28.0")] pub use core::slice::SliceIndex; #[cfg(not(no_global_oom_handling))] diff --git a/library/core/src/error.rs b/library/core/src/error.rs index 95a39cc3aed..91549f49f9f 100644 --- a/library/core/src/error.rs +++ b/library/core/src/error.rs @@ -1076,4 +1076,4 @@ impl Error for crate::time::TryFromFloatSecsError {} impl Error for crate::ffi::FromBytesUntilNulError {} #[unstable(feature = "get_many_mut", issue = "104642")] -impl<const N: usize> Error for crate::slice::GetManyMutError<N> {} +impl Error for crate::slice::GetManyMutError {} diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index 191eaccff98..97a7338b0bf 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -4629,13 +4629,11 @@ impl<T> [T] { pub fn get_many_mut<I, const N: usize>( &mut self, indices: [I; N], - ) -> Result<[&mut I::Output; N], GetManyMutError<N>> + ) -> Result<[&mut I::Output; N], GetManyMutError> where I: GetManyMutIndex + SliceIndex<Self>, { - if !get_many_check_valid(&indices, self.len()) { - return Err(GetManyMutError { _private: () }); - } + get_many_check_valid(&indices, self.len())?; // SAFETY: The `get_many_check_valid()` call checked that all indices // are disjunct and in bounds. unsafe { Ok(self.get_many_unchecked_mut(indices)) } @@ -4978,53 +4976,59 @@ impl<T, const N: usize> SlicePattern for [T; N] { /// This will do `binomial(N + 1, 2) = N * (N + 1) / 2 = 0, 1, 3, 6, 10, ..` /// comparison operations. #[inline] -fn get_many_check_valid<I: GetManyMutIndex, const N: usize>(indices: &[I; N], len: usize) -> bool { +fn get_many_check_valid<I: GetManyMutIndex, const N: usize>( + indices: &[I; N], + len: usize, +) -> Result<(), GetManyMutError> { // NB: The optimizer should inline the loops into a sequence // of instructions without additional branching. - let mut valid = true; for (i, idx) in indices.iter().enumerate() { - valid &= idx.is_in_bounds(len); + if !idx.is_in_bounds(len) { + return Err(GetManyMutError::IndexOutOfBounds); + } for idx2 in &indices[..i] { - valid &= !idx.is_overlapping(idx2); + if idx.is_overlapping(idx2) { + return Err(GetManyMutError::OverlappingIndices); + } } } - valid + Ok(()) } -/// The error type returned by [`get_many_mut<N>`][`slice::get_many_mut`]. +/// The error type returned by [`get_many_mut`][`slice::get_many_mut`]. /// /// It indicates one of two possible errors: /// - An index is out-of-bounds. -/// - The same index appeared multiple times in the array. +/// - The same index appeared multiple times in the array +/// (or different but overlapping indices when ranges are provided). /// /// # Examples /// /// ``` /// #![feature(get_many_mut)] +/// use std::slice::GetManyMutError; /// /// let v = &mut [1, 2, 3]; -/// assert!(v.get_many_mut([0, 999]).is_err()); -/// assert!(v.get_many_mut([1, 1]).is_err()); +/// assert_eq!(v.get_many_mut([0, 999]), Err(GetManyMutError::IndexOutOfBounds)); +/// assert_eq!(v.get_many_mut([1, 1]), Err(GetManyMutError::OverlappingIndices)); /// ``` #[unstable(feature = "get_many_mut", issue = "104642")] -// NB: The N here is there to be forward-compatible with adding more details -// to the error type at a later point -#[derive(Clone, PartialEq, Eq)] -pub struct GetManyMutError<const N: usize> { - _private: (), +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum GetManyMutError { + /// An index provided was out-of-bounds for the slice. + IndexOutOfBounds, + /// Two indices provided were overlapping. + OverlappingIndices, } #[unstable(feature = "get_many_mut", issue = "104642")] -impl<const N: usize> fmt::Debug for GetManyMutError<N> { +impl fmt::Display for GetManyMutError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("GetManyMutError").finish_non_exhaustive() - } -} - -#[unstable(feature = "get_many_mut", issue = "104642")] -impl<const N: usize> fmt::Display for GetManyMutError<N> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt("an index is out of bounds or appeared multiple times in the array", f) + let msg = match self { + GetManyMutError::IndexOutOfBounds => "an index is out of bounds", + GetManyMutError::OverlappingIndices => "there were overlapping indices", + }; + fmt::Display::fmt(msg, f) } } |
