From 43448d4d62f056994cc1c6dc019c05c2e349aef3 Mon Sep 17 00:00:00 2001 From: Alejandro Vaz Date: Fri, 21 Aug 2026 21:01:07 +0200 Subject: [PATCH] =?UTF-8?q?Revert=20"Refactor=20v2:=20faster=20pop/reserve?= =?UTF-8?q?/retain=5Fmut,=20modern=20Criterion=20benches,=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 36f34e1ba8a500c8101bba0b130434c24fc5c8d5. --- .github/workflows/main.yml | 1 + Cargo.toml | 6 - benches/bench.rs | 238 ++++++++++--------------------------- src/lib.rs | 142 +++++++--------------- src/tests.rs | 11 +- 5 files changed, 115 insertions(+), 283 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5703b4fd..79edf65f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -92,3 +92,4 @@ jobs: - name: Mark the job as unsuccessful run: exit 1 if: "!success()" + diff --git a/Cargo.toml b/Cargo.toml index 6188ae61..a2a1b256 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,13 +26,7 @@ malloc_size_of = { version = "0.1.1", optional = true, default-features = false [dev-dependencies] bincode = "1.0.1" -criterion = "0.8" [package.metadata.docs.rs] all-features = true rustdoc-args = ["--cfg", "docsrs"] - -[[bench]] -name = "bench" -path = "benches/bench.rs" -harness = false diff --git a/benches/bench.rs b/benches/bench.rs index bded165b..23864009 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -1,8 +1,10 @@ +#![feature(test)] #![allow(deprecated)] -use criterion::{criterion_group, criterion_main, Bencher, Criterion}; +extern crate test; + use smallvec::{smallvec, SmallVec}; -use std::hint::black_box; +use test::Bencher; const VEC_SIZE: usize = 16; const SPILLED_SIZE: usize = 100; @@ -16,90 +18,87 @@ trait Vector: for<'a> From<&'a [T]> + Extend { fn from_elem(val: T, n: usize) -> Self; fn from_elems(val: &[T]) -> Self; fn extend_from_slice(&mut self, other: &[T]); - fn retain_mut(&mut self, f: F) - where - F: FnMut(&mut T) -> bool; } impl Vector for Vec { fn new() -> Self { Self::with_capacity(VEC_SIZE) } + fn push(&mut self, val: T) { self.push(val) } + fn pop(&mut self) -> Option { self.pop() } + fn remove(&mut self, p: usize) -> T { self.remove(p) } + fn insert(&mut self, n: usize, val: T) { self.insert(n, val) } + fn from_elem(val: T, n: usize) -> Self { vec![val; n] } + fn from_elems(val: &[T]) -> Self { val.to_owned() } + fn extend_from_slice(&mut self, other: &[T]) { Vec::extend_from_slice(self, other) } - fn retain_mut(&mut self, f: F) - where - F: FnMut(&mut T) -> bool, - { - self.retain_mut(f) - } } impl Vector for SmallVec { fn new() -> Self { Self::new() } + fn push(&mut self, val: T) { self.push(val) } + fn pop(&mut self) -> Option { self.pop() } + fn remove(&mut self, p: usize) -> T { self.remove(p) } + fn insert(&mut self, n: usize, val: T) { self.insert(n, val) } + fn from_elem(val: T, n: usize) -> Self { smallvec![val; n] } + fn from_elems(val: &[T]) -> Self { SmallVec::from(val) } + fn extend_from_slice(&mut self, other: &[T]) { SmallVec::extend_from_slice(self, other) } - fn retain_mut(&mut self, f: F) - where - F: FnMut(&mut T) -> bool, - { - self.retain_mut(f) - } } macro_rules! make_benches { ($typ:ty { $($b_name:ident => $g_name:ident($($args:expr),*),)* }) => { $( - fn $b_name(c: &mut Criterion) { - c.bench_function(stringify!($b_name), |b: &mut Bencher| { - $g_name::<$typ>($($args,)* b) - }); + #[bench] + fn $b_name(b: &mut Bencher) { + $g_name::<$typ>($($args,)* b) } )* } } -/* ---------- Bench generation (same list, just using the new macro) ---------- */ make_benches! { SmallVec { bench_push => gen_push(SPILLED_SIZE as _), @@ -123,12 +122,6 @@ make_benches! { bench_macro_from_elem => gen_from_elem(SPILLED_SIZE as _), bench_macro_from_elem_small => gen_from_elem(VEC_SIZE as _), bench_pushpop => gen_pushpop(), - bench_retain_mut_half => gen_retain_mut_half(SPILLED_SIZE as _), - bench_retain_mut_half_small => gen_retain_mut_half(VEC_SIZE as _), - bench_retain_mut_all => gen_retain_mut_all(SPILLED_SIZE as _), - bench_retain_mut_all_small => gen_retain_mut_all(VEC_SIZE as _), - bench_retain_mut_none => gen_retain_mut_none(SPILLED_SIZE as _), - bench_retain_mut_none_small => gen_retain_mut_none(VEC_SIZE as _), } } @@ -155,257 +148,156 @@ make_benches! { bench_macro_from_elem_vec => gen_from_elem(SPILLED_SIZE as _), bench_macro_from_elem_vec_small => gen_from_elem(VEC_SIZE as _), bench_pushpop_vec => gen_pushpop(), - bench_retain_mut_vec_half => gen_retain_mut_half(SPILLED_SIZE as _), - bench_retain_mut_vec_half_small => gen_retain_mut_half(VEC_SIZE as _), - bench_retain_mut_vec_all => gen_retain_mut_all(SPILLED_SIZE as _), - bench_retain_mut_vec_all_small => gen_retain_mut_all(VEC_SIZE as _), - bench_retain_mut_vec_none => gen_retain_mut_none(SPILLED_SIZE as _), - bench_retain_mut_vec_none_small => gen_retain_mut_none(VEC_SIZE as _), } } -/* ---------- Benchmark helpers – unchanged except for Bencher type ---------- */ fn gen_push>(n: u64, b: &mut Bencher) { #[inline(never)] fn push_noinline>(vec: &mut V, x: u64) { - vec.push(black_box(x)); + vec.push(x); } b.iter(|| { - let n = black_box(n); let mut vec = V::new(); for x in 0..n { push_noinline(&mut vec, x); } - black_box(vec) + vec }); } fn gen_insert_push>(n: u64, b: &mut Bencher) { #[inline(never)] fn insert_push_noinline>(vec: &mut V, x: u64) { - vec.insert(black_box(x) as usize, black_box(x)); + vec.insert(x as usize, x); } b.iter(|| { - let n = black_box(n); let mut vec = V::new(); for x in 0..n { insert_push_noinline(&mut vec, x); } - black_box(vec) + vec }); } fn gen_insert>(n: u64, b: &mut Bencher) { #[inline(never)] fn insert_noinline>(vec: &mut V, p: usize, x: u64) { - vec.insert(black_box(p), black_box(x)) + vec.insert(p, x) } b.iter(|| { - let n = black_box(n); let mut vec = V::new(); + // Always insert at position 0 so that we are subject to shifts of + // many different lengths. vec.push(0); for x in 0..n { insert_noinline(&mut vec, 0, x); } - black_box(vec) + vec }); } fn gen_remove>(n: usize, b: &mut Bencher) { #[inline(never)] fn remove_noinline>(vec: &mut V, p: usize) -> u64 { - vec.remove(black_box(p)) + vec.remove(p) } b.iter(|| { - let n = black_box(n); let mut vec = V::from_elem(0, n as _); + for _ in 0..n { - black_box(remove_noinline(&mut vec, 0)); + remove_noinline(&mut vec, 0); } - black_box(vec) }); } fn gen_extend>(n: u64, b: &mut Bencher) { b.iter(|| { - let n = black_box(n); let mut vec = V::new(); vec.extend(0..n); - black_box(vec) + vec }); } fn gen_extend_filtered>(n: u64, b: &mut Bencher) { b.iter(|| { let mut vec = V::new(); - vec.extend((0..black_box(n)).filter(|i| black_box(*i) % 2 == 0)); - black_box(vec) + vec.extend((0..n).filter(|i| i % 2 == 0)); + vec }); } fn gen_from_iter>(n: u64, b: &mut Bencher) { - let v: Vec = (0..black_box(n)).collect(); + let v: Vec = (0..n).collect(); b.iter(|| { - let vec = V::from(black_box(&v)); - black_box(vec) + let vec = V::from(&v); + vec }); } fn gen_from_slice>(n: u64, b: &mut Bencher) { - let v: Vec = (0..black_box(n)).collect(); + let v: Vec = (0..n).collect(); b.iter(|| { - let vec = V::from_elems(black_box(&v)); - black_box(vec) + let vec = V::from_elems(&v); + vec }); } fn gen_extend_from_slice>(n: u64, b: &mut Bencher) { - let v: Vec = (0..black_box(n)).collect(); + let v: Vec = (0..n).collect(); b.iter(|| { let mut vec = V::new(); - vec.extend_from_slice(black_box(&v)); - black_box(vec) + vec.extend_from_slice(&v); + vec }); } fn gen_pushpop>(b: &mut Bencher) { #[inline(never)] fn pushpop_noinline>(vec: &mut V, x: u64) -> Option { - vec.push(black_box(x)); + vec.push(x); vec.pop() } b.iter(|| { let mut vec = V::new(); for x in 0..SPILLED_SIZE as _ { - black_box(pushpop_noinline(&mut vec, x)); + pushpop_noinline(&mut vec, x); } - black_box(vec) + vec }); } fn gen_from_elem>(n: usize, b: &mut Bencher) { b.iter(|| { - let n = black_box(n); - let vec = V::from_elem(black_box(42), n); - black_box(vec) - }); -} - -fn gen_retain_mut_half>(n: usize, b: &mut Bencher) { - b.iter(|| { - let n = black_box(n); - let mut vec = V::from_elem(16, n); - vec.retain_mut(|x| black_box(*x) % 2 == 0); - black_box(vec) + let vec = V::from_elem(42, n); + vec }); } -fn gen_retain_mut_all>(n: usize, b: &mut Bencher) { +#[bench] +fn bench_macro_from_list(b: &mut Bencher) { b.iter(|| { - let n = black_box(n); - let mut vec = V::from_elem(16, n); - vec.retain_mut(|_| true); - black_box(vec) + let vec: SmallVec = smallvec![ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20, 24, 32, 36, 0x40, 0x80, + 0x100, 0x200, 0x400, 0x800, 0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, + 0x80000, 0x100000, + ]; + vec }); } -fn gen_retain_mut_none>(n: usize, b: &mut Bencher) { +#[bench] +fn bench_macro_from_list_vec(b: &mut Bencher) { b.iter(|| { - let n = black_box(n); - let mut vec = V::from_elem(16, n); - vec.retain_mut(|_| false); - black_box(vec) + let vec: Vec = vec![ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20, 24, 32, 36, 0x40, 0x80, + 0x100, 0x200, 0x400, 0x800, 0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, + 0x80000, 0x100000, + ]; + vec }); } - -fn bench_macro_from_list(c: &mut Criterion) { - c.bench_function("bench_macro_from_list", |b| { - b.iter(|| { - let vec: SmallVec = smallvec![ - 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20, 24, 32, 36, 0x40, - 0x80, 0x100, 0x200, 0x400, 0x800, 0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, - 0x40000, 0x80000, 0x100000, - ]; - vec - }) - }); -} - -fn bench_macro_from_list_vec(c: &mut Criterion) { - c.bench_function("bench_macro_from_list_vec", |b| { - b.iter(|| { - let vec: Vec = vec![ - 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20, 24, 32, 36, 0x40, - 0x80, 0x100, 0x200, 0x400, 0x800, 0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, - 0x40000, 0x80000, 0x100000, - ]; - vec - }) - }); -} - -criterion_group!( - benches, - bench_push, - bench_push_small, - bench_insert_push, - bench_insert_push_small, - bench_insert, - bench_insert_small, - bench_remove, - bench_remove_small, - bench_extend, - bench_extend_small, - bench_extend_filtered, - bench_extend_filtered_small, - bench_from_iter, - bench_from_iter_small, - bench_from_slice, - bench_from_slice_small, - bench_extend_from_slice, - bench_extend_from_slice_small, - bench_macro_from_elem, - bench_macro_from_elem_small, - bench_pushpop, - bench_retain_mut_half, - bench_retain_mut_half_small, - bench_retain_mut_all, - bench_retain_mut_all_small, - bench_retain_mut_none, - bench_retain_mut_none_small, - bench_push_vec, - bench_push_vec_small, - bench_insert_push_vec, - bench_insert_push_vec_small, - bench_insert_vec, - bench_insert_vec_small, - bench_remove_vec, - bench_remove_vec_small, - bench_extend_vec, - bench_extend_vec_small, - bench_extend_vec_filtered, - bench_extend_vec_filtered_small, - bench_from_iter_vec, - bench_from_iter_vec_small, - bench_from_slice_vec, - bench_from_slice_vec_small, - bench_extend_from_slice_vec, - bench_extend_from_slice_vec_small, - bench_macro_from_elem_vec, - bench_macro_from_elem_vec_small, - bench_pushpop_vec, - bench_retain_mut_vec_half, - bench_retain_mut_vec_half_small, - bench_retain_mut_vec_all, - bench_retain_mut_vec_all_small, - bench_retain_mut_vec_none, - bench_retain_mut_vec_none_small, - bench_macro_from_list, - bench_macro_from_list_vec -); -criterion_main!(benches); diff --git a/src/lib.rs b/src/lib.rs index d08810fb..98b6f2f2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -499,10 +499,7 @@ impl Drain<'_, T, N> { let range_start = vec.len(); let range_end = self.tail_start; let range_slice = unsafe { - core::slice::from_raw_parts_mut( - vec.as_mut_ptr().add(range_start), - range_end - range_start, - ) + core::slice::from_raw_parts_mut(vec.as_mut_ptr().add(range_start), range_end - range_start) }; for place in range_slice { @@ -698,11 +695,7 @@ impl Drop for Splice<'_, I, N> { } // Collect any remaining elements. - let mut collected = self - .replace_with - .by_ref() - .collect::>() - .into_iter(); + let mut collected = self.replace_with.by_ref().collect::>().into_iter(); // Now we have an exact count. if collected.len() > 0 { self.drain.move_tail(collected.len()); @@ -855,9 +848,7 @@ impl SmallVec { #[inline] pub const fn from_buf(elements: [T; S]) -> Self { - const { - assert!(S <= N); - } + const { assert!(S <= N); } // Although we create a new buffer, since S and N are known at compile time, // even with `-C opt-level=1`, it gets optimized as best as it could be. (Checked with ) @@ -1213,10 +1204,7 @@ impl SmallVec { R: core::ops::RangeBounds, I: IntoIterator, { - Splice { - drain: self.drain(range), - replace_with: replace_with.into_iter(), - } + Splice { drain: self.drain(range), replace_with: replace_with.into_iter() } } #[inline] @@ -1235,16 +1223,15 @@ impl SmallVec { #[inline] pub fn pop(&mut self) -> Option { - let len = self.len(); - if len == 0 { + if self.is_empty() { None } else { - let new_len = len - 1; - // SAFETY: new_len < len since len is non-zero - unsafe { self.set_len(new_len) }; + let len = self.len() - 1; + // SAFETY: len < old_len since this can't overflow, because the old length is non zero + unsafe { self.set_len(len) }; // SAFETY: this element was initialized and we just gave up ownership of it, so we can // give it away - let value = unsafe { self.as_mut_ptr().add(new_len).read() }; + let value = unsafe { self.as_mut_ptr().add(len).read() }; Some(value) } } @@ -1252,11 +1239,7 @@ impl SmallVec { #[inline] pub fn pop_if(&mut self, predicate: impl FnOnce(&mut T) -> bool) -> Option { let last = self.last_mut()?; - if predicate(last) { - self.pop() - } else { - None - } + if predicate(last) { self.pop() } else { None } } #[inline] @@ -1325,26 +1308,16 @@ impl SmallVec { } #[inline] - #[track_caller] pub fn reserve(&mut self, additional: usize) { - // Callers expect this function to be very cheap when there is already sufficient capacity. - // Therefore, we move all the resizing and error-handling logic behind a call, while making - // sure that this function is likely to be inlined as just a comparison and a call if the - // comparison fails. - #[cold] - fn do_reserve_and_handle(slf: &mut SmallVec, additional: usize) { + // can't overflow since len <= capacity + if additional > self.capacity() - self.len() { let new_capacity = infallible( - slf.len() + self.len() .checked_add(additional) .and_then(usize::checked_next_power_of_two) .ok_or(CollectionAllocErr::CapacityOverflow), ); - slf.grow(new_capacity); - } - - // can't overflow since len <= capacity - if additional > self.capacity() - self.len() { - do_reserve_and_handle(self, additional); + self.grow(new_capacity); } } @@ -1356,30 +1329,22 @@ impl SmallVec { .checked_add(additional) .and_then(usize::checked_next_power_of_two) .ok_or(CollectionAllocErr::CapacityOverflow)?; - self.try_grow(new_capacity)?; + self.try_grow(new_capacity) + } else { + Ok(()) } - Ok(()) } #[inline] - #[track_caller] pub fn reserve_exact(&mut self, additional: usize) { - #[cold] - fn do_reserve_exact_and_handle( - slf: &mut SmallVec, - additional: usize, - ) { + // can't overflow since len <= capacity + if additional > self.capacity() - self.len() { let new_capacity = infallible( - slf.len() + self.len() .checked_add(additional) .ok_or(CollectionAllocErr::CapacityOverflow), ); - slf.grow(new_capacity); - } - - // can't overflow since len <= capacity - if additional > self.capacity() - self.len() { - do_reserve_exact_and_handle(self, additional); + self.grow(new_capacity); } } @@ -1390,9 +1355,10 @@ impl SmallVec { .len() .checked_add(additional) .ok_or(CollectionAllocErr::CapacityOverflow)?; - self.try_grow(new_capacity)?; + self.try_grow(new_capacity) + } else { + Ok(()) } - Ok(()) } #[inline] @@ -1438,10 +1404,7 @@ impl SmallVec { self.set_inline(); alloc::alloc::dealloc( ptr.cast().as_ptr(), - Layout::from_size_align_unchecked( - capacity * size_of::(), - align_of::(), - ), + Layout::from_size_align_unchecked(capacity * size_of::(), align_of::()), ); } } else if target < self.capacity() { @@ -1472,10 +1435,7 @@ impl SmallVec { #[inline] pub fn swap_remove(&mut self, index: usize) -> T { let len = self.len(); - assert!( - index < len, - "swap_remove index (is {index}) should be < len (is {len})" - ); + assert!(index < len, "swap_remove index (is {index}) should be < len (is {len})"); // This can't overflow since `len > index >= 0` let new_len = len - 1; unsafe { @@ -1507,10 +1467,7 @@ impl SmallVec { #[inline] pub fn remove(&mut self, index: usize) -> T { let len = self.len(); - assert!( - index < len, - "removal index (is {index}) should be < len (is {len})" - ); + assert!(index < len, "removal index (is {index}) should be < len (is {len})"); let new_len = len - 1; unsafe { // SAFETY: new_len < len @@ -1527,10 +1484,7 @@ impl SmallVec { #[inline] pub fn insert(&mut self, index: usize, value: T) { let len = self.len(); - assert!( - index <= len, - "insertion index (is {index}) should be <= len (is {len})" - ); + assert!(index <= len, "insertion index (is {index}) should be <= len (is {len})"); self.reserve(1); let ptr = self.as_mut_ptr(); unsafe { @@ -1641,28 +1595,21 @@ impl SmallVec { #[inline] pub fn retain_mut bool>(&mut self, mut f: F) { + let mut del = 0; let len = self.len(); - - if len == 0 { - // return early as hint to llvm, like what std does - return; - } - let ptr = self.as_mut_ptr(); - let mut write_idx = 0; - - for read_idx in 0..len { + for i in 0..len { // SAFETY: all the pointers are in bounds + // `i - del` never overflows since `del <= i` is a maintained invariant unsafe { - if f(&mut *ptr.add(read_idx)) { - if write_idx < read_idx { - core::ptr::swap(ptr.add(read_idx), ptr.add(write_idx)); - } - write_idx += 1; + if !f(&mut *ptr.add(i)) { + del += 1; + } else if del > 0 { + core::ptr::swap(ptr.add(i), ptr.add(i - del)); } } } - self.truncate(write_idx); + self.truncate(len - del); } #[inline] @@ -1733,9 +1680,7 @@ impl SmallVec { pub fn leak<'a>(self) -> &'a mut [T] { if !self.spilled() { - panic!( - "SmallVec::leak() called on inline (stack) SmallVec, which cannot be safely leaked" - ); + panic!("SmallVec::leak() called on inline (stack) SmallVec, which cannot be safely leaked"); } let mut me = ManuallyDrop::new(self); unsafe { core::slice::from_raw_parts_mut(me.as_mut_ptr(), me.len()) } @@ -1868,11 +1813,12 @@ impl SmallVec { #[inline] pub fn extend_from_slice_copy(&mut self, other: &[T]) where - T: Copy, + T: Copy { + let len = other.len(); let src = other.as_ptr(); - + let l = self.len(); self.reserve(len); @@ -1888,7 +1834,7 @@ impl SmallVec { pub fn extend_from_within_copy(&mut self, src: R) where R: core::ops::RangeBounds, - T: Copy, + T: Copy { let src = slice_range(src, ..self.len()); let core::ops::Range { start, end } = src; @@ -1907,7 +1853,7 @@ impl SmallVec { pub fn insert_from_slice_copy(&mut self, index: usize, other: &[T]) where - T: Copy, + T: Copy { let l = self.len(); let len = other.len(); @@ -1931,7 +1877,7 @@ impl SmallVec { /// for types with the [`Copy`] trait. pub fn from_slice_copy(slice: &[T]) -> Self where - T: Copy, + T: Copy { let src = slice.as_ptr(); let len = slice.len(); @@ -2644,7 +2590,7 @@ impl Clone for SmallVec { #[cfg(not(feature = "specialization"))] { - self.clone_from_fallback(source); + self.clone_from_fallback(&*source); } } } diff --git a/src/tests.rs b/src/tests.rs index 4d1b934b..a6db2c8b 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -587,7 +587,10 @@ fn test_from() { #[test] fn test_from_slice() { assert_eq!(&SmallVec::::from(&[1][..])[..], [1]); - assert_eq!(&SmallVec::::from(&[1, 2, 3][..])[..], [1, 2, 3]); + assert_eq!( + &SmallVec::::from(&[1, 2, 3][..])[..], + [1, 2, 3] + ); } #[test] @@ -986,11 +989,7 @@ fn collect_from_iter() { // A length of 3 is fine to trigger this bug under valgrind, but making the vector 1 million // elements makes it crash - which is much easier to detect. - #[cfg(miri)] - const ELEMENTS: usize = 1000; - #[cfg(not(miri))] - const ELEMENTS: usize = 1_000_000; - let iter = IterNoHint(std::iter::repeat(1u8).take(ELEMENTS)); + let iter = IterNoHint(std::iter::repeat(1u8).take(1_000_000)); let _y: SmallVec = SmallVec::from_iter(iter); }