Stop forcing array::map through an unnecessary Result

Author: scottmcm

library/core/src/array/mod.rs

@@ -825,14 +825,13 @@ impl<T, const N: usize> [T; N] {
 /// Pulls `N` items from `iter` and returns them as an array. If the iterator
 /// yields fewer than `N` items, this function exhibits undefined behavior.
 ///
-/// See [`try_collect_into_array`] for more information.
-///
-///
 /// # Safety
 ///
 /// It is up to the caller to guarantee that `iter` yields at least `N` items.
 /// Violating this condition causes undefined behavior.
-unsafe fn try_collect_into_array_unchecked<I, T, R, const N: usize>(iter: &mut I) -> R::TryType
+unsafe fn try_collect_into_array_unchecked<I, T, R, const N: usize>(
+ iter: &mut I,
+) -> ChangeOutputType<I::Item, [T; N]>
 where
  // Note: `TrustedLen` here is somewhat of an experiment. This is just an
  // internal function, so feel free to remove if this bound turns out to be a
@@ -845,11 +844,21 @@ where
  debug_assert!(N <= iter.size_hint().1.unwrap_or(usize::MAX));
  debug_assert!(N <= iter.size_hint().0);
 
- // SAFETY: covered by the function contract.
- unsafe { try_collect_into_array(iter).unwrap_unchecked() }
+ let mut array = MaybeUninit::uninit_array::<N>();
+ let cf = try_collect_into_array_erased(iter, &mut array);
+ match cf {
+ ControlFlow::Break(r) => FromResidual::from_residual(r),
+ ControlFlow::Continue(initialized) => {
+ debug_assert_eq!(initialized, N);
+ // SAFETY: because of our function contract, all the elements
+ // must have been initialized.
+ let output = unsafe { MaybeUninit::array_assume_init(array) };
+ Try::from_output(output)
+ }
+ }
 }
 
-// Infallible version of `try_collect_into_array_unchecked`.
+/// Infallible version of [`try_collect_into_array_unchecked`].
 unsafe fn collect_into_array_unchecked<I, const N: usize>(iter: &mut I) -> [I::Item; N]
 where
  I: Iterator + TrustedLen,
@@ -864,63 +873,48 @@ where
  }
 }
 
-/// 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.
+/// Rather than *returning* the array, this fills in a passed-in buffer.
+/// If any of the iterator elements short-circuit, it drops everything in the
+/// buffer and return the error. Otherwise it returns the number of items
+/// which were initialized in the buffer.
 ///
-/// Since the iterator is passed as a mutable reference and this function calls
-/// `next` at most `N` times, the iterator can still be used afterwards to
-/// retrieve the remaining items.
+/// (The caller is responsible for dropping those items on success, but not
+/// doing that is just a leak, not UB, so this function is itself safe.)
 ///
-/// If `iter.next()` panicks, all items already yielded by the iterator are
-/// dropped.
+/// This means less monomorphization, but more importantly it means that the
+/// returned array doesn't need to be copied into the `Result`, since returning
+/// the result seemed (2023-01) to cause in an extra `N + 1`-length `alloca`
+/// even if it's always `unwrap_unchecked` later.
 #[inline]
-fn try_collect_into_array<I, T, R, const N: usize>(
+fn try_collect_into_array_erased<I, T, R>(
  iter: &mut I,
-) -> Result<R::TryType, IntoIter<T, N>>
+ buffer: &mut [MaybeUninit<T>],
+) -> ControlFlow<R, usize>
 where
  I: Iterator,
  I::Item: Try<Output = T, Residual = R>,
- R: Residual<[T; N]>,
 {
- if N == 0 {
- // SAFETY: An empty array is always inhabited and has no validity invariants.
- return Ok(Try::from_output(unsafe { mem::zeroed() }));
- }
-
- let mut array = MaybeUninit::uninit_array::<N>();
- let mut guard = Guard { array_mut: &mut array, initialized: 0 };
+ let n = buffer.len();
+ let mut guard = Guard { array_mut: buffer, initialized: 0 };
 
- for _ in 0..N {
+ for _ in 0..n {
  match iter.next() {
  Some(item_rslt) => {
- let item = match item_rslt.branch() {
- ControlFlow::Break(r) => {
- return Ok(FromResidual::from_residual(r));
- }
- ControlFlow::Continue(elem) => elem,
- };
+ let item = item_rslt.branch()?;
 
  // SAFETY: `guard.initialized` starts at 0, which means push can be called
- // at most N times, which this loop does.
+ // at most `n` times, which this loop does.
  unsafe {
  guard.push_unchecked(item);
  }
  }
- None => {
- let alive = 0..guard.initialized;
- mem::forget(guard);
- // SAFETY: `array` was initialized with exactly `initialized`
- // number of elements.
- return Err(unsafe { IntoIter::new_unchecked(array, alive) });
- }
+ None => break,
  }
  }
 
+ let initialized = guard.initialized;
  mem::forget(guard);
- // SAFETY: All elements of the array were populated in the loop above.
- let output = unsafe { array.transpose().assume_init() };
- Ok(Try::from_output(output))
+ ControlFlow::Continue(initialized)
 }
 
 /// Panic guard for incremental initialization of arrays.
@@ -934,14 +928,14 @@ where
 ///
 /// To minimize indirection fields are still pub but callers should at least use
 /// `push_unchecked` to signal that something unsafe is going on.
-pub(crate) struct Guard<'a, T, const N: usize> {
+pub(crate) struct Guard<'a, T> {
  /// The array to be initialized.
- pub array_mut: &'a mut [MaybeUninit<T>; N],
+ pub array_mut: &'a mut [MaybeUninit<T>],
  /// The number of items that have been initialized so far.
  pub initialized: usize,
 }
 
-impl<T, const N: usize> Guard<'_, T, N> {
+impl<T> Guard<'_, T> {
  /// Adds an item to the array and updates the initialized item counter.
  ///
  /// # Safety
@@ -959,9 +953,9 @@ impl<T, const N: usize> Guard<'_, T, N> {
  }
 }
 
-impl<T, const N: usize> Drop for Guard<'_, T, N> {
+impl<T> Drop for Guard<'_, T> {
  fn drop(&mut self) {
- debug_assert!(self.initialized <= N);
+ debug_assert!(self.initialized <= self.array_mut.len());
 
  // SAFETY: this slice will contain only initialized objects.
  unsafe {
@@ -972,15 +966,33 @@ impl<T, const N: usize> Drop for Guard<'_, T, N> {
  }
 }
 
-/// Returns the next chunk of `N` items from the iterator or errors with an
-/// iterator over the remainder. Used for `Iterator::next_chunk`.
+/// 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.
+///
+/// Since the iterator is passed as a mutable reference and this function calls
+/// `next` at most `N` times, the iterator can still be used afterwards to
+/// retrieve the remaining items.
+///
+/// If `iter.next()` panicks, all items already yielded by the iterator are
+/// dropped.
+///
+/// Used for [`Iterator::next_chunk`].
 #[inline]
-pub(crate) fn iter_next_chunk<I, const N: usize>(
- iter: &mut I,
-) -> Result<[I::Item; N], IntoIter<I::Item, N>>
-where
- I: Iterator,
-{
+pub(crate) fn iter_next_chunk<T, const N: usize>(
+ iter: &mut impl Iterator<Item = T>,
+) -> Result<[T; N], IntoIter<T, N>> {
  let mut map = iter.map(NeverShortCircuit);
- try_collect_into_array(&mut map).map(|NeverShortCircuit(arr)| arr)
+ let mut array = MaybeUninit::uninit_array::<N>();
+ let ControlFlow::Continue(initialized) = try_collect_into_array_erased(&mut map, &mut array);
+ if initialized == N {
+ // SAFETY: All elements of the array were populated.
+ let output = unsafe { MaybeUninit::array_assume_init(array) };
+ Ok(output)
+ } else {
+ let alive = 0..initialized;
+ // SAFETY: `array` was initialized with exactly `initialized`
+ // number of elements.
+ return Err(unsafe { IntoIter::new_unchecked(array, alive) });
+ }
 }

tests/codegen/array-map.rs

@@ -1,4 +1,4 @@
-// compile-flags: -C opt-level=3 -C target-cpu=x86-64-v3 -C llvm-args=-x86-asm-syntax=intel --emit=llvm-ir,asm
+// compile-flags: -C opt-level=3 -C target-cpu=x86-64-v3
 // no-system-llvm
 // only-x86_64
 // ignore-debug (the extra assertions get in the way)
@@ -40,8 +40,7 @@ pub fn short_integer_zip_map(x: [u32; 8], y: [u32; 8]) -> [u32; 8] {
 #[no_mangle]
 pub fn long_integer_map(x: [u32; 64]) -> [u32; 64] {
  // CHECK: start:
- // CHECK-NEXT: alloca [{{64|65}} x i32]
- // CHECK-NEXT: alloca [{{64|65}} x i32]
+ // CHECK-NEXT: alloca [64 x i32]
  // CHECK-NEXT: alloca %"core::mem::manually_drop::ManuallyDrop<[u32; 64]>"
  // CHECK-NOT: alloca
  x.map(|x| 2 * x + 1)