-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
Optimize array chunks #151668
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?
Optimize array chunks #151668
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -964,6 +964,53 @@ impl<T: [const] Destruct> const Drop for Guard<'_, T> { | |
| } | ||
| } | ||
|
|
||
| /// Panic guard for incremental initialization of arrays from the back. | ||
| /// | ||
| /// Elements of the array are populated starting from the end towards the beginning. | ||
| /// Disarm the guard with `mem::forget` once the array has been fully initialized. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// All write accesses to this structure are unsafe and must maintain a correct | ||
| /// count of `initialized` elements. | ||
| struct GuardBack<'a, T> { | ||
| /// The array to be initialized (will be filled from the end). | ||
| pub array_mut: &'a mut [MaybeUninit<T>], | ||
| /// The number of items that have been initialized so far. | ||
vinDelphini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub initialized: usize, | ||
| } | ||
|
|
||
| impl<T> GuardBack<'_, T> { | ||
| /// Adds an item to the array and updates the initialized item counter. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// No more than N elements must be initialized. | ||
| #[inline] | ||
| pub(crate) unsafe fn push_unchecked(&mut self, item: T) { | ||
| // SAFETY: If `initialized` was correct before and the caller does not | ||
| // invoke this method more than N times, then writes will be in-bounds | ||
| // and slots will not be initialized more than once. | ||
| unsafe { | ||
| self.initialized = self.initialized.unchecked_add(1); | ||
| let index = self.array_mut.len().unchecked_sub(self.initialized); | ||
| self.array_mut.get_unchecked_mut(index).write(item); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T> Drop for GuardBack<'_, T> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be the same as for the Guard which reads:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd leave that for when it is actually used in |
||
| #[inline] | ||
| fn drop(&mut self) { | ||
| debug_assert!(self.initialized <= self.array_mut.len()); | ||
| let len = self.array_mut.len(); | ||
| // SAFETY: this slice will contain only initialized objects. | ||
| unsafe { | ||
| self.array_mut.get_unchecked_mut(len - self.initialized..len).assume_init_drop(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Pulls `N` items from `iter` and returns them as an array. If the iterator | ||
| /// yields fewer than `N` items, `Err` is returned containing an iterator over | ||
| /// the already yielded items. | ||
|
|
@@ -1022,3 +1069,56 @@ fn iter_next_chunk_erased<T>( | |
| mem::forget(guard); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Pulls `N` items from the back of `iter` and returns them as an array. | ||
| /// If the iterator yields fewer than `N` items, `Err` is returned containing | ||
| /// an iterator over the already yielded items. | ||
| /// | ||
| /// Since the iterator is passed as a mutable reference and this function calls | ||
| /// `next_back` at most `N` times, the iterator can still be used afterwards to | ||
| /// retrieve the remaining items. | ||
| /// | ||
| /// If `iter.next_back()` panics, all items already yielded by the iterator are | ||
| /// dropped. | ||
| /// | ||
| /// Used for [`DoubleEndedIterator::next_chunk_back`]. | ||
| #[inline] | ||
| pub(crate) fn iter_next_chunk_back<T, const N: usize>( | ||
| iter: &mut impl DoubleEndedIterator<Item = T>, | ||
| ) -> Result<[T; N], IntoIter<T, N>> { | ||
| let mut array = [const { MaybeUninit::uninit() }; N]; | ||
| let r = iter_next_chunk_back_erased(&mut array, iter); | ||
| match r { | ||
| Ok(()) => { | ||
| // SAFETY: All elements of `array` were populated. | ||
| Ok(unsafe { MaybeUninit::array_assume_init(array) }) | ||
| } | ||
| Err(initialized) => { | ||
| // SAFETY: Only the last `initialized` elements were populated | ||
| Err(unsafe { IntoIter::new_unchecked(array, N - initialized..N) }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Version of [`iter_next_chunk_back`] using a passed-in slice. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. " in order to avoid needing to monomorphize for every array length."
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like the other versions clarify their existence. |
||
| #[inline] | ||
| fn iter_next_chunk_back_erased<T>( | ||
| buffer: &mut [MaybeUninit<T>], | ||
| iter: &mut impl DoubleEndedIterator<Item = T>, | ||
| ) -> Result<(), usize> { | ||
| // if `Iterator::next_back` panics, this guard will drop already initialized items | ||
| let mut guard = GuardBack { array_mut: buffer, initialized: 0 }; | ||
| while guard.initialized < guard.array_mut.len() { | ||
| let Some(item) = iter.next_back() else { | ||
| let initialized = guard.initialized; | ||
| mem::forget(guard); | ||
| return Err(initialized); | ||
| }; | ||
|
|
||
| // SAFETY: The loop condition ensures we have space to push the item | ||
| unsafe { guard.push_unchecked(item) }; | ||
| } | ||
|
|
||
| mem::forget(guard); | ||
| Ok(()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,6 @@ | ||
| use crate::array; | ||
| use crate::iter::adapters::SourceIter; | ||
| use crate::iter::{ | ||
| ByRefSized, FusedIterator, InPlaceIterable, TrustedFused, TrustedRandomAccessNoCoerce, | ||
| }; | ||
| use crate::iter::{FusedIterator, InPlaceIterable, TrustedFused, TrustedRandomAccessNoCoerce}; | ||
| use crate::num::NonZero; | ||
| use crate::ops::{ControlFlow, NeverShortCircuit, Try}; | ||
|
|
||
|
|
@@ -128,15 +126,11 @@ where | |
| self.next_back_remainder(); | ||
|
|
||
| let mut acc = init; | ||
| let mut iter = ByRefSized(&mut self.iter).rev(); | ||
|
|
||
| // NB remainder is handled by `next_back_remainder`, so | ||
| // `next_chunk` can't return `Err` with non-empty remainder | ||
| // `next_chunk_back` can't return `Err` with non-empty remainder | ||
| // (assuming correct `I as ExactSizeIterator` impl). | ||
| while let Ok(mut chunk) = iter.next_chunk() { | ||
| // FIXME: do not do double reverse | ||
| // (we could instead add `next_chunk_back` for example) | ||
| chunk.reverse(); | ||
| while let Ok(chunk) = self.iter.next_chunk_back() { | ||
| acc = f(acc, chunk)? | ||
|
Comment on lines
128
to
134
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do this part in a separate PR, or at least a separate commit. Optimizations shouldn't be tied with new features, both for obvious history and for ability to revert. Update the PR title based on what you decide as well. |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| use crate::array; | ||
| use crate::num::NonZero; | ||
| use crate::ops::{ControlFlow, Try}; | ||
|
|
||
|
|
@@ -93,6 +94,20 @@ pub trait DoubleEndedIterator: Iterator { | |
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| fn next_back(&mut self) -> Option<Self::Item>; | ||
|
|
||
| /// Pulls `N` items from the back of the iterator and returns them as an array. | ||
| /// | ||
| /// See [`Iterator::next_chunk`] for more details. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add a note on the ordering within the array (and a short explanation why this is not equivalent to |
||
| #[inline] | ||
| #[unstable(feature = "iter_next_chunk", issue = "98326")] | ||
| fn next_chunk_back<const N: usize>( | ||
| &mut self, | ||
| ) -> Result<[Self::Item; N], array::IntoIter<Self::Item, N>> | ||
| where | ||
| Self: Sized, | ||
| { | ||
| crate::array::iter_next_chunk_back(self) | ||
| } | ||
vinDelphini marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// Advances the iterator from the back by `n` elements. | ||
| /// | ||
| /// `advance_back_by` is the reverse version of [`advance_by`]. This method will | ||
|
|
||
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.
Why are you deleting documentation?
If this was AI-generated, please make sure you can explain every change in the diff.
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.
I've restored the missing documentation for
iter_next_chunkand added detailed docs for the new backward-filling logic — sorry, it was a few custom config rules I added in my editor that trimmed them out. I'll make sure it doesn't happen again!i implemented a new
GuardBackspecifically to handle the reverse-initialization logic safely. While the standard Guard fills an array from 0..N,GuardBackfills from N down to 0. If a panic occurs,GuardBack::dropcorrectly identifies and drops only the partial results at the end of the array (the len - initialized..len slice). This is required because we're pulling from the back of the iterator but want the final array to be in the original order.I followed the same pattern as next_chunk in #98326 & ArrayChunks implementation in #100450.
Uh oh!
There was an error while loading. Please reload this page.
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.
I'm wondering if we really need a dedicated
GuardBack<'a, T>struct for this? The underlying members ofGuardBack<'a, T>is the same asGuard<'a, T>with the only difference being theDropimplementation, which makes me think thatGuard<'a, T>can be reused for this purpose.For example, I was taking a look at this discussion on using TypeId, which exists in both
core::anyandstd::anymodule. Would it be possible (or a good idea) to approach making the guard like so:If we are able to reuse
Guard<'a, T>like this, I think instead of only havingpush_unchecked(), we should have two different functions:push_unchecked_front(), which replaces the name ofGuard<'a, T>'spush_unchecked(), andpush_unchecked_back(), which is whatGuardBack<'a, T>'spush_unchecked()does. On a nit, I do think we can give it a better name thanGuard<'a,T>(ChunkGuard<'a, T>?), but I don't think that's too important to worry about since this struct is private.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.
Yeah, I get the idea and it does reduce the number of structs on paper - but it also pulls in a lot of clever stuff (generics,
TypeId, 'staticbounds) for something that’s really just an internal guard.For me, keeping
GuardandGuardBackseparate keeps theDroplogic dead simple and very obvious - which matters, because that’s the part doing the safety work. One drops0..n, the other dropslen-n..len. You read it once and you’re done.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.
How about a simple enum instead:
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.
Let's not overcomplicate things, this is fine as is.