Skip to content

Commit 28928c7

Browse files
committed
Auto merge of #77617 - AnthonyMikh:slice_windows_no_bounds_checking, r=lcnr
Eliminate bounds checking in slice::Windows This is how `<core::slice::Windows as Iterator>::next` looks right now: ```rust fn next(&mut self) -> Option<&'a [T]> { if self.size > self.v.len() { None } else { let ret = Some(&self.v[..self.size]); self.v = &self.v[1..]; ret } } ``` The line with `self.v = &self.v[1..];` relies on assumption that `self.v` is definitely not empty at this point. Else branch is taken when `self.size <= self.v.len()`, so `self.v` can be empty if `self.size` is zero. In practice, since `Windows` is never created directly but rather trough `[T]::windows` which panics when `size` is zero, `self.size` is never zero. However, the compiler doesn't know about this check, so it keeps the code which checks bounds and panics. Using `NonZeroUsize` lets the compiler know about this invariant and reliably eliminate bounds checking without `unsafe` on `-O2`. Here is assembly of `Windows<'a, u32>::next` before and after this change ([goldbolt](https://godbolt.org/z/xrefzx)): <details> <summary>Before</summary> ``` example::next: push rax mov rcx, qword ptr [rdi + 8] mov rdx, qword ptr [rdi + 16] cmp rdx, rcx jbe .LBB0_2 xor eax, eax pop rcx ret .LBB0_2: test rcx, rcx je .LBB0_5 mov rax, qword ptr [rdi] mov rsi, rax add rsi, 4 add rcx, -1 mov qword ptr [rdi], rsi mov qword ptr [rdi + 8], rcx pop rcx ret .LBB0_5: lea rdx, [rip + .L__unnamed_1] mov edi, 1 xor esi, esi call qword ptr [rip + core::slice::slice_index_order_fail@GOTPCREL] ud2 .L__unnamed_2: .ascii "./example.rs" .L__unnamed_1: .quad .L__unnamed_2 .asciz "\f\000\000\000\000\000\000\000\016\000\000\000\027\000\000" ``` </details> <details> <summary>After</summary> ``` example::next: mov rcx, qword ptr [rdi + 8] mov rdx, qword ptr [rdi + 16] cmp rdx, rcx jbe .LBB0_2 xor eax, eax ret .LBB0_2: mov rax, qword ptr [rdi] lea rsi, [rax + 4] add rcx, -1 mov qword ptr [rdi], rsi mov qword ptr [rdi + 8], rcx ret ``` </details> Note the lack of call to `core::slice::slice_index_order_fail` in second snippet. #### Possible reasons _not_ to merge this PR: * this changes the error message on panic in `[T]::windows`. However, AFAIK this messages are not covered by backwards compatibility policy.
2 parents deec530 + e699e83 commit 28928c7

File tree

3 files changed

+52
-15
lines changed

3 files changed

+52
-15
lines changed

library/core/src/slice/iter.rs

+15-14
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use crate::intrinsics::{assume, exact_div, unchecked_sub};
1010
use crate::iter::{FusedIterator, TrustedLen, TrustedRandomAccess};
1111
use crate::marker::{PhantomData, Send, Sized, Sync};
1212
use crate::mem;
13+
use crate::num::NonZeroUsize;
1314
use crate::ptr::NonNull;
1415

1516
use super::{from_raw_parts, from_raw_parts_mut};
@@ -1187,12 +1188,12 @@ forward_iterator! { RSplitNMut: T, &'a mut [T] }
11871188
#[stable(feature = "rust1", since = "1.0.0")]
11881189
pub struct Windows<'a, T: 'a> {
11891190
v: &'a [T],
1190-
size: usize,
1191+
size: NonZeroUsize,
11911192
}
11921193

11931194
impl<'a, T: 'a> Windows<'a, T> {
11941195
#[inline]
1195-
pub(super) fn new(slice: &'a [T], size: usize) -> Self {
1196+
pub(super) fn new(slice: &'a [T], size: NonZeroUsize) -> Self {
11961197
Self { v: slice, size }
11971198
}
11981199
}
@@ -1211,21 +1212,21 @@ impl<'a, T> Iterator for Windows<'a, T> {
12111212

12121213
#[inline]
12131214
fn next(&mut self) -> Option<&'a [T]> {
1214-
if self.size > self.v.len() {
1215+
if self.size.get() > self.v.len() {
12151216
None
12161217
} else {
1217-
let ret = Some(&self.v[..self.size]);
1218+
let ret = Some(&self.v[..self.size.get()]);
12181219
self.v = &self.v[1..];
12191220
ret
12201221
}
12211222
}
12221223

12231224
#[inline]
12241225
fn size_hint(&self) -> (usize, Option<usize>) {
1225-
if self.size > self.v.len() {
1226+
if self.size.get() > self.v.len() {
12261227
(0, Some(0))
12271228
} else {
1228-
let size = self.v.len() - self.size + 1;
1229+
let size = self.v.len() - self.size.get() + 1;
12291230
(size, Some(size))
12301231
}
12311232
}
@@ -1237,7 +1238,7 @@ impl<'a, T> Iterator for Windows<'a, T> {
12371238

12381239
#[inline]
12391240
fn nth(&mut self, n: usize) -> Option<Self::Item> {
1240-
let (end, overflow) = self.size.overflowing_add(n);
1241+
let (end, overflow) = self.size.get().overflowing_add(n);
12411242
if end > self.v.len() || overflow {
12421243
self.v = &[];
12431244
None
@@ -1250,10 +1251,10 @@ impl<'a, T> Iterator for Windows<'a, T> {
12501251

12511252
#[inline]
12521253
fn last(self) -> Option<Self::Item> {
1253-
if self.size > self.v.len() {
1254+
if self.size.get() > self.v.len() {
12541255
None
12551256
} else {
1256-
let start = self.v.len() - self.size;
1257+
let start = self.v.len() - self.size.get();
12571258
Some(&self.v[start..])
12581259
}
12591260
}
@@ -1264,18 +1265,18 @@ impl<'a, T> Iterator for Windows<'a, T> {
12641265
// which means that `i` cannot overflow an `isize`, and the
12651266
// slice created by `from_raw_parts` is a subslice of `self.v`
12661267
// thus is guaranteed to be valid for the lifetime `'a` of `self.v`.
1267-
unsafe { from_raw_parts(self.v.as_ptr().add(idx), self.size) }
1268+
unsafe { from_raw_parts(self.v.as_ptr().add(idx), self.size.get()) }
12681269
}
12691270
}
12701271

12711272
#[stable(feature = "rust1", since = "1.0.0")]
12721273
impl<'a, T> DoubleEndedIterator for Windows<'a, T> {
12731274
#[inline]
12741275
fn next_back(&mut self) -> Option<&'a [T]> {
1275-
if self.size > self.v.len() {
1276+
if self.size.get() > self.v.len() {
12761277
None
12771278
} else {
1278-
let ret = Some(&self.v[self.v.len() - self.size..]);
1279+
let ret = Some(&self.v[self.v.len() - self.size.get()..]);
12791280
self.v = &self.v[..self.v.len() - 1];
12801281
ret
12811282
}
@@ -1284,11 +1285,11 @@ impl<'a, T> DoubleEndedIterator for Windows<'a, T> {
12841285
#[inline]
12851286
fn nth_back(&mut self, n: usize) -> Option<Self::Item> {
12861287
let (end, overflow) = self.v.len().overflowing_sub(n);
1287-
if end < self.size || overflow {
1288+
if end < self.size.get() || overflow {
12881289
self.v = &[];
12891290
None
12901291
} else {
1291-
let ret = &self.v[end - self.size..end];
1292+
let ret = &self.v[end - self.size.get()..end];
12921293
self.v = &self.v[..end - 1];
12931294
Some(ret)
12941295
}

library/core/src/slice/mod.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use crate::cmp::Ordering::{self, Equal, Greater, Less};
1212
use crate::marker::Copy;
1313
use crate::mem;
14+
use crate::num::NonZeroUsize;
1415
use crate::ops::{FnMut, Range, RangeBounds};
1516
use crate::option::Option;
1617
use crate::option::Option::{None, Some};
@@ -751,7 +752,7 @@ impl<T> [T] {
751752
#[stable(feature = "rust1", since = "1.0.0")]
752753
#[inline]
753754
pub fn windows(&self, size: usize) -> Windows<'_, T> {
754-
assert_ne!(size, 0);
755+
let size = NonZeroUsize::new(size).expect("size is zero");
755756
Windows::new(self, size)
756757
}
757758

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#![crate_type = "lib"]
2+
3+
// compile-flags: -O
4+
5+
use std::slice::Windows;
6+
7+
// CHECK-LABEL: @naive_string_search
8+
#[no_mangle]
9+
pub fn naive_string_search(haystack: &str, needle: &str) -> Option<usize> {
10+
if needle.is_empty() {
11+
return Some(0);
12+
}
13+
// CHECK-NOT: panic
14+
// CHECK-NOT: fail
15+
haystack
16+
.as_bytes()
17+
.windows(needle.len())
18+
.position(|sub| sub == needle.as_bytes())
19+
}
20+
21+
// CHECK-LABEL: @next
22+
#[no_mangle]
23+
pub fn next<'a>(w: &mut Windows<'a, u32>) -> Option<&'a [u32]> {
24+
// CHECK-NOT: panic
25+
// CHECK-NOT: fail
26+
w.next()
27+
}
28+
29+
// CHECK-LABEL: @next_back
30+
#[no_mangle]
31+
pub fn next_back<'a>(w: &mut Windows<'a, u32>) -> Option<&'a [u32]> {
32+
// CHECK-NOT: panic
33+
// CHECK-NOT: fail
34+
w.next_back()
35+
}

0 commit comments

Comments
 (0)