From 74a85b5a8b486020f4a652135eb0990a28e9a64b Mon Sep 17 00:00:00 2001 From: The 8472 Date: Sun, 5 Feb 2023 20:37:14 +0100 Subject: [PATCH 1/6] add bench for chained slice iterators --- library/core/benches/iter.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/core/benches/iter.rs b/library/core/benches/iter.rs index 05fec0c4b9d26..cc38eea6d3d1d 100644 --- a/library/core/benches/iter.rs +++ b/library/core/benches/iter.rs @@ -356,6 +356,12 @@ bench_sums! { .take(1000000) } +bench_sums! { + bench_slice_chain_sum, + bench_slice_chain_ref_sum, + (&[0; 512]).iter().chain((&[1; 512]).iter()) +} + // Checks whether Skip> is as fast as Zip, Skip>, from // https://users.rust-lang.org/t/performance-difference-between-iterator-zip-and-skip-order/15743 #[bench] From 80be3ed277a8126c6c589c0dad1de5b149440584 Mon Sep 17 00:00:00 2001 From: The8472 Date: Mon, 2 Oct 2023 17:01:03 +0200 Subject: [PATCH 2/6] add external iteration version of iter benchmarks The by_ref versions used to exercise external iteration but since the try_fold optimiations this is no loner the case. --- library/core/benches/iter.rs | 37 +++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/library/core/benches/iter.rs b/library/core/benches/iter.rs index cc38eea6d3d1d..f98fead49721d 100644 --- a/library/core/benches/iter.rs +++ b/library/core/benches/iter.rs @@ -201,7 +201,7 @@ fn bench_for_each_chain_ref_fold(b: &mut Bencher) { /// Helper to benchmark `sum` for iterators taken by value which /// can optimize `fold`, and by reference which cannot. macro_rules! bench_sums { - ($bench_sum:ident, $bench_ref_sum:ident, $iter:expr) => { + ($bench_sum:ident, $bench_ref_sum:ident, $bench_ext_sum:ident, $iter:expr) => { #[bench] fn $bench_sum(b: &mut Bencher) { b.iter(|| -> i64 { $iter.map(black_box).sum() }); @@ -211,144 +211,178 @@ macro_rules! bench_sums { fn $bench_ref_sum(b: &mut Bencher) { b.iter(|| -> i64 { $iter.map(black_box).by_ref().sum() }); } + + #[bench] + fn $bench_ext_sum(b: &mut Bencher) { + b.iter(|| -> i64 { + let mut sum = 0; + for i in $iter.map(black_box) { + sum += i; + } + sum + }); + } }; } bench_sums! { bench_flat_map_sum, bench_flat_map_ref_sum, + bench_flat_map_ext_sum, (0i64..1000).flat_map(|x| x..x+1000) } bench_sums! { bench_flat_map_chain_sum, bench_flat_map_chain_ref_sum, + bench_flat_map_chain_ext_sum, (0i64..1000000).flat_map(|x| once(x).chain(once(x))) } bench_sums! { bench_enumerate_sum, bench_enumerate_ref_sum, + bench_enumerate_ext_sum, (0i64..1000000).enumerate().map(|(i, x)| x * i as i64) } bench_sums! { bench_enumerate_chain_sum, bench_enumerate_chain_ref_sum, + bench_enumerate_chain_ext_sum, (0i64..1000000).chain(0..1000000).enumerate().map(|(i, x)| x * i as i64) } bench_sums! { bench_filter_sum, bench_filter_ref_sum, + bench_filter_ext_sum, (0i64..1000000).filter(|x| x % 3 == 0) } bench_sums! { bench_filter_chain_sum, bench_filter_chain_ref_sum, + bench_filter_chain_ext_sum, (0i64..1000000).chain(0..1000000).filter(|x| x % 3 == 0) } bench_sums! { bench_filter_map_sum, bench_filter_map_ref_sum, + bench_filter_map_ext_sum, (0i64..1000000).filter_map(|x| x.checked_mul(x)) } bench_sums! { bench_filter_map_chain_sum, bench_filter_map_chain_ref_sum, + bench_filter_map_chain_ext_sum, (0i64..1000000).chain(0..1000000).filter_map(|x| x.checked_mul(x)) } bench_sums! { bench_fuse_sum, bench_fuse_ref_sum, + bench_fuse_ext_sum, (0i64..1000000).fuse() } bench_sums! { bench_fuse_chain_sum, bench_fuse_chain_ref_sum, + bench_fuse_chain_ext_sum, (0i64..1000000).chain(0..1000000).fuse() } bench_sums! { bench_inspect_sum, bench_inspect_ref_sum, + bench_inspect_ext_sum, (0i64..1000000).inspect(|_| {}) } bench_sums! { bench_inspect_chain_sum, bench_inspect_chain_ref_sum, + bench_inspect_chain_ext_sum, (0i64..1000000).chain(0..1000000).inspect(|_| {}) } bench_sums! { bench_peekable_sum, bench_peekable_ref_sum, + bench_peekable_ext_sum, (0i64..1000000).peekable() } bench_sums! { bench_peekable_chain_sum, bench_peekable_chain_ref_sum, + bench_peekable_chain_ext_sum, (0i64..1000000).chain(0..1000000).peekable() } bench_sums! { bench_skip_sum, bench_skip_ref_sum, + bench_skip_ext_sum, (0i64..1000000).skip(1000) } bench_sums! { bench_skip_chain_sum, bench_skip_chain_ref_sum, + bench_skip_chain_ext_sum, (0i64..1000000).chain(0..1000000).skip(1000) } bench_sums! { bench_skip_while_sum, bench_skip_while_ref_sum, + bench_skip_while_ext_sum, (0i64..1000000).skip_while(|&x| x < 1000) } bench_sums! { bench_skip_while_chain_sum, bench_skip_while_chain_ref_sum, + bench_skip_while_chain_ext_sum, (0i64..1000000).chain(0..1000000).skip_while(|&x| x < 1000) } bench_sums! { bench_take_while_chain_sum, bench_take_while_chain_ref_sum, + bench_take_while_chain_ext_sum, (0i64..1000000).chain(1000000..).take_while(|&x| x < 1111111) } bench_sums! { bench_cycle_take_sum, bench_cycle_take_ref_sum, + bench_cycle_take_ext_sum, (0..10000).cycle().take(1000000) } bench_sums! { bench_cycle_skip_take_sum, bench_cycle_skip_take_ref_sum, + bench_cycle_skip_take_ext_sum, (0..100000).cycle().skip(1000000).take(1000000) } bench_sums! { bench_cycle_take_skip_sum, bench_cycle_take_skip_ref_sum, + bench_cycle_take_skip_ext_sum, (0..100000).cycle().take(1000000).skip(100000) } bench_sums! { bench_skip_cycle_skip_zip_add_sum, bench_skip_cycle_skip_zip_add_ref_sum, + bench_skip_cycle_skip_zip_add_ext_sum, (0..100000).skip(100).cycle().skip(100) .zip((0..100000).cycle().skip(10)) .map(|(a,b)| a+b) @@ -359,6 +393,7 @@ bench_sums! { bench_sums! { bench_slice_chain_sum, bench_slice_chain_ref_sum, + bench_slice_chain_ext_sum, (&[0; 512]).iter().chain((&[1; 512]).iter()) } From 5b39ec8ec85f7c92f7d51bbb3eff683e7f51f2d3 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Sun, 5 Feb 2023 20:55:08 +0100 Subject: [PATCH 3/6] specialize iter::Chain when both sides have the same type --- library/core/src/iter/adapters/chain.rs | 41 ++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/library/core/src/iter/adapters/chain.rs b/library/core/src/iter/adapters/chain.rs index 26aa959e6da3f..668d9f46c8b86 100644 --- a/library/core/src/iter/adapters/chain.rs +++ b/library/core/src/iter/adapters/chain.rs @@ -1,5 +1,6 @@ use crate::iter::{DoubleEndedIterator, FusedIterator, Iterator, TrustedLen}; use crate::num::NonZeroUsize; +use crate::mem; use crate::ops::Try; /// An iterator that links two iterators together, in a chain. @@ -48,7 +49,7 @@ where #[inline] fn next(&mut self) -> Option { - and_then_or_clear(&mut self.a, Iterator::next).or_else(|| self.b.as_mut()?.next()) + SpecChain::next(self) } #[inline] @@ -303,3 +304,41 @@ fn and_then_or_clear(opt: &mut Option, f: impl FnOnce(&mut T) -> Option } x } + +#[rustc_unsafe_specialization_marker] +trait SymmetricalArms {} + +impl SymmetricalArms for Chain {} + +trait SpecChain: Iterator { + fn next(&mut self) -> Option; +} + +impl SpecChain for Chain +where + A: Iterator, + B: Iterator, +{ + #[inline] + default fn next(&mut self) -> Option { + and_then_or_clear(&mut self.a, Iterator::next).or_else(|| self.b.as_mut()?.next()) + } +} + +impl SpecChain for Chain +where + A: Iterator, + B: Iterator, + Self: SymmetricalArms, +{ + #[inline] + fn next(&mut self) -> Option { + let mut result = and_then_or_clear(&mut self.a, Iterator::next); + if result.is_none() { + // SAFETY: SymmetricalArms guarantees that A and B are the same type. + unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; + result = and_then_or_clear(&mut self.a, Iterator::next); + } + result + } +} From bff32edc7c419886b8a46cb1d0a4e54caab5427a Mon Sep 17 00:00:00 2001 From: The 8472 Date: Tue, 21 Feb 2023 20:56:52 +0100 Subject: [PATCH 4/6] mark specialization trait as unsafe and add safety comments --- library/core/src/iter/adapters/chain.rs | 33 +++++++++++++++++++++---- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/library/core/src/iter/adapters/chain.rs b/library/core/src/iter/adapters/chain.rs index 668d9f46c8b86..b1e5576cd4f3c 100644 --- a/library/core/src/iter/adapters/chain.rs +++ b/library/core/src/iter/adapters/chain.rs @@ -305,10 +305,33 @@ fn and_then_or_clear(opt: &mut Option, f: impl FnOnce(&mut T) -> Option x } +/// Marks the two generic parameters of Chain as sufficiently equal that their values can be swapped +/// +/// # Safety +/// +/// This would be trivially safe if both types were identical, including lifetimes. +/// However we can't specify bounds like that and it would be overly restrictive since it's not +/// uncommon for borrowing iterators to have slightly different lifetimes. +/// +/// We can relax this by only requiring that the base struct type is the same while ignoring +/// lifetime parameters as long as +/// * the actual runtime lifespan of the values is capped by the shorter of the two lifetimes +/// * all invoked trait methods (and drop code) monomorphize down to the same code #[rustc_unsafe_specialization_marker] -trait SymmetricalArms {} - -impl SymmetricalArms for Chain {} +unsafe trait SymmetricalModuloLifetimes {} + +/// Safety: +/// * ensures that the basic type is the same +/// * actual lifespan of the values is capped by the combined lifetime of Chain's fields as long as +/// there is no way to destructure Chain into. I.e. Chain must not implement `SourceIter`, +/// `into_parts(self)` or similar methods. +/// * we rely on the language currently having no mechanism that would allow lifetime-dependent +/// code paths. Specialization forbids `where T: 'static` and similar bounds (modulo the exposed +/// `#[rustc_unsafe_specialization_marker]` traits). +/// And any trait depending on `Any` would have to be 'static in *both* arms to make a useful Chain. +/// This is only true as long as *all* impls on `Chain` have the same bounds for A and B, +/// which currently is the case. +unsafe impl SymmetricalModuloLifetimes for Chain {} trait SpecChain: Iterator { fn next(&mut self) -> Option; @@ -329,13 +352,13 @@ impl SpecChain for Chain where A: Iterator, B: Iterator, - Self: SymmetricalArms, + Self: SymmetricalModuloLifetimes, { #[inline] fn next(&mut self) -> Option { let mut result = and_then_or_clear(&mut self.a, Iterator::next); if result.is_none() { - // SAFETY: SymmetricalArms guarantees that A and B are the same type. + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; result = and_then_or_clear(&mut self.a, Iterator::next); } From 2c2a695eb426c9b0388dcb9d56a6f2210d7e6661 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Tue, 21 Feb 2023 21:39:43 +0100 Subject: [PATCH 5/6] add SymmetricalModuloLifetimes specialization for double-ended chains --- library/core/src/iter/adapters/chain.rs | 35 ++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/library/core/src/iter/adapters/chain.rs b/library/core/src/iter/adapters/chain.rs index b1e5576cd4f3c..681a3f3d72373 100644 --- a/library/core/src/iter/adapters/chain.rs +++ b/library/core/src/iter/adapters/chain.rs @@ -179,7 +179,7 @@ where { #[inline] fn next_back(&mut self) -> Option { - and_then_or_clear(&mut self.b, |b| b.next_back()).or_else(|| self.a.as_mut()?.next_back()) + SpecChainBack::next_back(self) } #[inline] @@ -337,6 +337,10 @@ trait SpecChain: Iterator { fn next(&mut self) -> Option; } +trait SpecChainBack: DoubleEndedIterator { + fn next_back(&mut self) -> Option; +} + impl SpecChain for Chain where A: Iterator, @@ -348,6 +352,17 @@ where } } +impl SpecChainBack for Chain +where + A: DoubleEndedIterator, + B: DoubleEndedIterator, +{ + #[inline] + default fn next_back(&mut self) -> Option { + and_then_or_clear(&mut self.b, |b| b.next_back()).or_else(|| self.a.as_mut()?.next_back()) + } +} + impl SpecChain for Chain where A: Iterator, @@ -365,3 +380,21 @@ where result } } + +impl SpecChainBack for Chain +where + A: DoubleEndedIterator, + B: DoubleEndedIterator, + Self: SymmetricalModuloLifetimes, +{ + #[inline] + fn next_back(&mut self) -> Option { + let mut result = and_then_or_clear(&mut self.b, DoubleEndedIterator::next_back); + if result.is_none() { + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap + unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; + result = and_then_or_clear(&mut self.b, DoubleEndedIterator::next_back); + } + result + } +} From 50a2248bacf5092e9dd512b5d9ae5ea6d8cef629 Mon Sep 17 00:00:00 2001 From: The8472 Date: Tue, 3 Oct 2023 01:40:42 +0200 Subject: [PATCH 6/6] reduce the work done per iteration for non-Drop, fused iterators --- library/core/src/iter/adapters/chain.rs | 46 ++++++++++++++++++------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/library/core/src/iter/adapters/chain.rs b/library/core/src/iter/adapters/chain.rs index 681a3f3d72373..1bd5ffed2fdd2 100644 --- a/library/core/src/iter/adapters/chain.rs +++ b/library/core/src/iter/adapters/chain.rs @@ -1,6 +1,6 @@ use crate::iter::{DoubleEndedIterator, FusedIterator, Iterator, TrustedLen}; use crate::num::NonZeroUsize; -use crate::mem; +use crate::{mem, ptr}; use crate::ops::Try; /// An iterator that links two iterators together, in a chain. @@ -365,17 +365,27 @@ where impl SpecChain for Chain where - A: Iterator, - B: Iterator, + A: Iterator + FusedIterator, + B: Iterator + FusedIterator, Self: SymmetricalModuloLifetimes, { #[inline] fn next(&mut self) -> Option { - let mut result = and_then_or_clear(&mut self.a, Iterator::next); + let mut result = self.a.as_mut().and_then( Iterator::next); if result.is_none() { - // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap - unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; - result = and_then_or_clear(&mut self.a, Iterator::next); + if mem::needs_drop::() { + // swap iters to avoid running drop code inside the loop. + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap. + unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; + } else { + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap. + // And they dont need drop, so we can overwrite the values directly. + unsafe { + ptr::write(&mut self.a, ptr::from_ref(&self.b).cast::>().read()); + ptr::write(&mut self.b, None); + } + } + result = self.a.as_mut().and_then(Iterator::next); } result } @@ -383,17 +393,27 @@ where impl SpecChainBack for Chain where - A: DoubleEndedIterator, - B: DoubleEndedIterator, + A: DoubleEndedIterator + FusedIterator, + B: DoubleEndedIterator + FusedIterator, Self: SymmetricalModuloLifetimes, { #[inline] fn next_back(&mut self) -> Option { - let mut result = and_then_or_clear(&mut self.b, DoubleEndedIterator::next_back); + let mut result = self.b.as_mut().and_then( DoubleEndedIterator::next_back); if result.is_none() { - // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap - unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; - result = and_then_or_clear(&mut self.b, DoubleEndedIterator::next_back); + if mem::needs_drop::() { + // swap iters to avoid running drop code inside the loop. + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap. + unsafe { mem::swap(&mut self.a, &mut *(&mut self.b as *mut _ as *mut Option)) }; + } else { + // SAFETY: SymmetricalModuloLifetimes guarantees that A and B are safe to swap. + // And they dont need drop, so we can overwrite the values directly. + unsafe { + ptr::write(&mut self.b, ptr::from_ref(&self.a).cast::>().read()); + ptr::write(&mut self.a, None); + } + } + result = self.b.as_mut().and_then(DoubleEndedIterator::next_back); } result }