From f9259d1b73f01c4c690dfd7429bb3ce25a34874f Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 9 May 2023 01:04:32 -0400 Subject: [PATCH 1/5] Boost intersperse(_with) performance I did some benchmark digging into the `intersperse` and `intersperse_with` code as part of the https://internals.rust-lang.org/t/add-iterate-with-separators-iterator-function/18781/13 discussion, and as a result I optimized them a bit, without relying on the peekable iterator. --- library/core/src/iter/adapters/intersperse.rs | 126 ++++++++++-------- 1 file changed, 74 insertions(+), 52 deletions(-) diff --git a/library/core/src/iter/adapters/intersperse.rs b/library/core/src/iter/adapters/intersperse.rs index d8bbd424cf2..654e3077771 100644 --- a/library/core/src/iter/adapters/intersperse.rs +++ b/library/core/src/iter/adapters/intersperse.rs @@ -1,4 +1,5 @@ -use super::Peekable; +use core::fmt; +use core::iter::{Fuse, FusedIterator}; /// An iterator adapter that places a separator between all elements. /// @@ -11,8 +12,16 @@ pub struct Intersperse I::Item: Clone, { separator: I::Item, - iter: Peekable, - needs_sep: bool, + next_item: Option, + iter: Fuse, +} + +#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "79524")] +impl FusedIterator for Intersperse +where + I: FusedIterator, + I::Item: Clone, +{ } impl Intersperse @@ -20,7 +29,8 @@ impl Intersperse I::Item: Clone, { pub(in crate::iter) fn new(iter: I, separator: I::Item) -> Self { - Self { iter: iter.peekable(), separator, needs_sep: false } + let mut iter = iter.fuse(); + Self { separator, next_item: iter.next(), iter } } } @@ -33,27 +43,31 @@ impl Iterator for Intersperse type Item = I::Item; #[inline] - fn next(&mut self) -> Option { - if self.needs_sep && self.iter.peek().is_some() { - self.needs_sep = false; - Some(self.separator.clone()) + fn next(&mut self) -> Option { + if let Some(v) = self.next_item.take() { + Some(v) } else { - self.needs_sep = true; - self.iter.next() + let next_item = self.iter.next(); + if next_item.is_some() { + self.next_item = next_item; + Some(self.separator.clone()) + } else { + None + } } } + fn size_hint(&self) -> (usize, Option) { + intersperse_size_hint(&self.iter, self.next_item.is_some()) + } + fn fold(self, init: B, f: F) -> B where Self: Sized, F: FnMut(B, Self::Item) -> B, { let separator = self.separator; - intersperse_fold(self.iter, init, f, move || separator.clone(), self.needs_sep) - } - - fn size_hint(&self) -> (usize, Option) { - intersperse_size_hint(&self.iter, self.needs_sep) + intersperse_fold(self.iter, init, f, move || separator.clone(), self.next_item) } } @@ -67,38 +81,46 @@ pub struct IntersperseWith I: Iterator, { separator: G, - iter: Peekable, - needs_sep: bool, + next_item: Option, + iter: Fuse, } #[unstable(feature = "iter_intersperse", reason = "recently added", issue = "79524")] -impl crate::fmt::Debug for IntersperseWith +impl FusedIterator for IntersperseWith where - I: Iterator + crate::fmt::Debug, - I::Item: crate::fmt::Debug, - G: crate::fmt::Debug, + I: FusedIterator, + G: FnMut() -> I::Item, { - fn fmt(&self, f: &mut crate::fmt::Formatter<'_>) -> crate::fmt::Result { +} + +#[unstable(feature = "iter_intersperse", reason = "recently added", issue = "79524")] +impl fmt::Debug for IntersperseWith +where + I: Iterator + fmt::Debug, + I::Item: fmt::Debug, + G: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("IntersperseWith") .field("separator", &self.separator) .field("iter", &self.iter) - .field("needs_sep", &self.needs_sep) + .field("next_item", &self.next_item) .finish() } } #[unstable(feature = "iter_intersperse", reason = "recently added", issue = "79524")] -impl crate::clone::Clone for IntersperseWith +impl Clone for IntersperseWith where - I: Iterator + crate::clone::Clone, - I::Item: crate::clone::Clone, + I: Iterator + Clone, + I::Item: Clone, G: Clone, { fn clone(&self) -> Self { - IntersperseWith { + Self { separator: self.separator.clone(), iter: self.iter.clone(), - needs_sep: self.needs_sep.clone(), + next_item: self.next_item.clone(), } } } @@ -109,7 +131,8 @@ impl IntersperseWith G: FnMut() -> I::Item, { pub(in crate::iter) fn new(iter: I, separator: G) -> Self { - Self { iter: iter.peekable(), separator, needs_sep: false } + let mut iter = iter.fuse(); + Self { separator, next_item: iter.next(), iter } } } @@ -122,47 +145,50 @@ impl Iterator for IntersperseWith type Item = I::Item; #[inline] - fn next(&mut self) -> Option { - if self.needs_sep && self.iter.peek().is_some() { - self.needs_sep = false; - Some((self.separator)()) + fn next(&mut self) -> Option { + if let Some(v) = self.next_item.take() { + Some(v) } else { - self.needs_sep = true; - self.iter.next() + let next_item = self.iter.next(); + if next_item.is_some() { + self.next_item = next_item; + Some((self.separator)()) + } else { + None + } } } + fn size_hint(&self) -> (usize, Option) { + intersperse_size_hint(&self.iter, self.next_item.is_some()) + } + fn fold(self, init: B, f: F) -> B where Self: Sized, F: FnMut(B, Self::Item) -> B, { - intersperse_fold(self.iter, init, f, self.separator, self.needs_sep) - } - - fn size_hint(&self) -> (usize, Option) { - intersperse_size_hint(&self.iter, self.needs_sep) + intersperse_fold(self.iter, init, f, self.separator, self.next_item) } } -fn intersperse_size_hint(iter: &I, needs_sep: bool) -> (usize, Option) +fn intersperse_size_hint(iter: &I, next_is_elem: bool) -> (usize, Option) where I: Iterator, { let (lo, hi) = iter.size_hint(); - let next_is_elem = !needs_sep; ( - lo.saturating_sub(next_is_elem as usize).saturating_add(lo), - hi.and_then(|hi| hi.saturating_sub(next_is_elem as usize).checked_add(hi)), + lo.saturating_add(next_is_elem as usize).saturating_add(lo), + hi.map(|hi| hi.saturating_add(next_is_elem as usize).saturating_add(hi)), ) } fn intersperse_fold( - mut iter: I, + iter: I, init: B, mut f: F, mut separator: G, - needs_sep: bool, + mut next_item: Option, ) -> B where I: Iterator, @@ -171,12 +197,8 @@ fn intersperse_fold( { let mut accum = init; - if !needs_sep { - if let Some(x) = iter.next() { - accum = f(accum, x); - } else { - return accum; - } + if let Some(x) = next_item.take() { + accum = f(accum, x); } iter.fold(accum, |mut accum, x| { From b8d245e7490be49fedf0609bd9763adc1116b725 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 9 May 2023 18:31:56 -0400 Subject: [PATCH 2/5] Postpone .next() call until iteration --- library/core/src/iter/adapters/intersperse.rs | 78 ++++++++++++------- 1 file changed, 49 insertions(+), 29 deletions(-) diff --git a/library/core/src/iter/adapters/intersperse.rs b/library/core/src/iter/adapters/intersperse.rs index 654e3077771..2ff24cd7792 100644 --- a/library/core/src/iter/adapters/intersperse.rs +++ b/library/core/src/iter/adapters/intersperse.rs @@ -11,6 +11,7 @@ pub struct Intersperse where I::Item: Clone, { + started: bool, separator: I::Item, next_item: Option, iter: Fuse, @@ -29,8 +30,7 @@ impl Intersperse I::Item: Clone, { pub(in crate::iter) fn new(iter: I, separator: I::Item) -> Self { - let mut iter = iter.fuse(); - Self { separator, next_item: iter.next(), iter } + Self { started: false, separator, next_item: None, iter: iter.fuse() } } } @@ -44,21 +44,26 @@ impl Iterator for Intersperse #[inline] fn next(&mut self) -> Option { - if let Some(v) = self.next_item.take() { - Some(v) - } else { - let next_item = self.iter.next(); - if next_item.is_some() { - self.next_item = next_item; - Some(self.separator.clone()) + if self.started { + if let Some(v) = self.next_item.take() { + Some(v) } else { - None + let next_item = self.iter.next(); + if next_item.is_some() { + self.next_item = next_item; + Some(self.separator.clone()) + } else { + None + } } + } else { + self.started = true; + self.iter.next() } } fn size_hint(&self) -> (usize, Option) { - intersperse_size_hint(&self.iter, self.next_item.is_some()) + intersperse_size_hint(&self.iter, self.started, self.next_item.is_some()) } fn fold(self, init: B, f: F) -> B @@ -67,7 +72,7 @@ fn fold(self, init: B, f: F) -> B F: FnMut(B, Self::Item) -> B, { let separator = self.separator; - intersperse_fold(self.iter, init, f, move || separator.clone(), self.next_item) + intersperse_fold(self.iter, init, f, move || separator.clone(), self.started,self.next_item) } } @@ -80,6 +85,7 @@ pub struct IntersperseWith where I: Iterator, { + started: bool, separator: G, next_item: Option, iter: Fuse, @@ -102,6 +108,7 @@ impl fmt::Debug for IntersperseWith { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("IntersperseWith") + .field("started", &self.started) .field("separator", &self.separator) .field("iter", &self.iter) .field("next_item", &self.next_item) @@ -118,6 +125,7 @@ impl Clone for IntersperseWith { fn clone(&self) -> Self { Self { + started: self.started, separator: self.separator.clone(), iter: self.iter.clone(), next_item: self.next_item.clone(), @@ -131,8 +139,7 @@ impl IntersperseWith G: FnMut() -> I::Item, { pub(in crate::iter) fn new(iter: I, separator: G) -> Self { - let mut iter = iter.fuse(); - Self { separator, next_item: iter.next(), iter } + Self { started: false, separator, next_item: None, iter: iter.fuse() } } } @@ -146,21 +153,26 @@ impl Iterator for IntersperseWith #[inline] fn next(&mut self) -> Option { - if let Some(v) = self.next_item.take() { - Some(v) - } else { - let next_item = self.iter.next(); - if next_item.is_some() { - self.next_item = next_item; - Some((self.separator)()) + if self.started { + if let Some(v) = self.next_item.take() { + Some(v) } else { - None + let next_item = self.iter.next(); + if next_item.is_some() { + self.next_item = next_item; + Some((self.separator)()) + } else { + None + } } + } else { + self.started = true; + self.iter.next() } } fn size_hint(&self) -> (usize, Option) { - intersperse_size_hint(&self.iter, self.next_item.is_some()) + intersperse_size_hint(&self.iter, self.started, self.next_item.is_some()) } fn fold(self, init: B, f: F) -> B @@ -168,26 +180,33 @@ fn fold(self, init: B, f: F) -> B Self: Sized, F: FnMut(B, Self::Item) -> B, { - intersperse_fold(self.iter, init, f, self.separator, self.next_item) + intersperse_fold(self.iter, init, f, self.separator, self.started, self.next_item) } } -fn intersperse_size_hint(iter: &I, next_is_elem: bool) -> (usize, Option) +fn intersperse_size_hint(iter: &I, started: bool, next_is_some: bool) -> (usize, Option) where I: Iterator, { let (lo, hi) = iter.size_hint(); ( - lo.saturating_add(next_is_elem as usize).saturating_add(lo), - hi.map(|hi| hi.saturating_add(next_is_elem as usize).saturating_add(hi)), + lo.saturating_sub(!started as usize) + .saturating_add(next_is_some as usize) + .saturating_add(lo), + hi.map(|hi| { + hi.saturating_sub(!started as usize) + .saturating_add(next_is_some as usize) + .saturating_add(hi) + }), ) } fn intersperse_fold( - iter: I, + mut iter: I, init: B, mut f: F, mut separator: G, + started: bool, mut next_item: Option, ) -> B where @@ -197,7 +216,8 @@ fn intersperse_fold( { let mut accum = init; - if let Some(x) = next_item.take() { + let first = if started { next_item.take() } else { iter.next() }; + if let Some(x) = first { accum = f(accum, x); } From f1dbc7b35ed70847237046a5a61af1ba88eae31d Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 9 May 2023 18:40:54 -0400 Subject: [PATCH 3/5] fmt --- library/core/src/iter/adapters/intersperse.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/core/src/iter/adapters/intersperse.rs b/library/core/src/iter/adapters/intersperse.rs index 2ff24cd7792..aab3743a51a 100644 --- a/library/core/src/iter/adapters/intersperse.rs +++ b/library/core/src/iter/adapters/intersperse.rs @@ -72,7 +72,14 @@ fn fold(self, init: B, f: F) -> B F: FnMut(B, Self::Item) -> B, { let separator = self.separator; - intersperse_fold(self.iter, init, f, move || separator.clone(), self.started,self.next_item) + intersperse_fold( + self.iter, + init, + f, + move || separator.clone(), + self.started, + self.next_item, + ) } } From 8cbff0b426b0f8821c6852545b8ed6aa74bfeffe Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 25 Jan 2024 20:33:06 -0500 Subject: [PATCH 4/5] Update library/core/src/iter/adapters/intersperse.rs Co-authored-by: Josh Stone --- library/core/src/iter/adapters/intersperse.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/iter/adapters/intersperse.rs b/library/core/src/iter/adapters/intersperse.rs index aab3743a51a..f436fe02dca 100644 --- a/library/core/src/iter/adapters/intersperse.rs +++ b/library/core/src/iter/adapters/intersperse.rs @@ -1,5 +1,5 @@ -use core::fmt; -use core::iter::{Fuse, FusedIterator}; +use crate::fmt; +use crate::iter::{Fuse, FusedIterator}; /// An iterator adapter that places a separator between all elements. /// From 77f31ef2b29e0ff16b9db8907327741c057bea8e Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 25 Jan 2024 20:56:52 -0500 Subject: [PATCH 5/5] use checked_add for upper bound --- library/core/src/iter/adapters/intersperse.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/iter/adapters/intersperse.rs b/library/core/src/iter/adapters/intersperse.rs index f436fe02dca..c97a59b614f 100644 --- a/library/core/src/iter/adapters/intersperse.rs +++ b/library/core/src/iter/adapters/intersperse.rs @@ -200,10 +200,10 @@ fn intersperse_size_hint(iter: &I, started: bool, next_is_some: bool) -> (usi lo.saturating_sub(!started as usize) .saturating_add(next_is_some as usize) .saturating_add(lo), - hi.map(|hi| { + hi.and_then(|hi| { hi.saturating_sub(!started as usize) .saturating_add(next_is_some as usize) - .saturating_add(hi) + .checked_add(hi) }), ) }