-
Notifications
You must be signed in to change notification settings - Fork 1k
perf: override ArrayIter default impl for nth, nth_back, last and count
#8785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| pub fn new(array: T) -> Self { | ||
| let len = array.len(); | ||
| let logical_nulls = array.logical_nulls(); | ||
| let logical_nulls = array.logical_nulls().filter(|x| x.null_count() > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid checking for nulls if null buffer exists but no nulls
| fn last(mut self) -> Option<Self::Item> { | ||
| self.next_back() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default implementation is doing it in O(n) and is not (currently) taking advantage of it being DoubleEndedIterator while we are doing it in constant time.
this is the default impl:
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn last(self) -> Option<Self::Item>
where
Self: Sized,
{
#[inline]
fn some<T>(_: Option<T>, x: T) -> Option<T> {
Some(x)
}
self.fold(None, some)
}from Rust source code
| fn count(self) -> usize | ||
| where | ||
| Self: Sized, | ||
| { | ||
| self.len() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count default impl does not (currently) taking advantage of it being ExactSizeIterator and it does it in O(n) while we are doing it in constant time:
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn count(self) -> usize
where
Self: Sized,
{
self.fold(
0,
#[rustc_inherit_overflow_checks]
|count, _| count + 1,
)
}From Rust source code
| fn nth(&mut self, n: usize) -> Option<Self::Item> { | ||
| // Check if we can advance to the desired offset | ||
| match self.current.checked_add(n) { | ||
| // Yes, and still within bounds | ||
| Some(new_current) if new_current < self.current_end => { | ||
| self.current = new_current; | ||
| } | ||
|
|
||
| // Either overflow or would exceed current_end | ||
| _ => { | ||
| self.current = self.current_end; | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| self.next() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default implementation does it in O(n) while we can do it in constant time.
we also can't implement advance_by which is used by nth in the default implementation as it is unstable
This is the default implementation:
#[inline]
#[unstable(feature = "iter_advance_by", reason = "recently added", issue = "77404")]
fn advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> {
/// Helper trait to specialize `advance_by` via `try_fold` for `Sized` iterators.
trait SpecAdvanceBy {
fn spec_advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>>;
}
impl<I: Iterator + ?Sized> SpecAdvanceBy for I {
default fn spec_advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> {
for i in 0..n {
if self.next().is_none() {
// SAFETY: `i` is always less than `n`.
return Err(unsafe { NonZero::new_unchecked(n - i) });
}
}
Ok(())
}
}
impl<I: Iterator> SpecAdvanceBy for I {
fn spec_advance_by(&mut self, n: usize) -> Result<(), NonZero<usize>> {
let Some(n) = NonZero::new(n) else {
return Ok(());
};
let res = self.try_fold(n, |n, _| NonZero::new(n.get() - 1));
match res {
None => Ok(()),
Some(n) => Err(n),
}
}
}
self.spec_advance_by(n)
}
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn nth(&mut self, n: usize) -> Option<Self::Item> {
self.advance_by(n).ok()?;
self.next()
}From Rust source code
| } | ||
| } | ||
|
|
||
| fn nth_back(&mut self, n: usize) -> Option<Self::Item> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea as in nth
Which issue does this PR close?
N/A
Rationale for this change
The default implementations iterate over the iterator to get the value, while we can do that in constant time
What changes are included in this PR?
override
nth,nth_back,lastandcountAre these changes tested?
existing tests in this file that I added in previous pr
Are there any user-facing changes?
Nope
Extracted from the following PR as I probably close it as it is not faster locally in some cases:
ArrayIterwith dedicated null/non-nullable versions #8697