From b9c12b0d4a018d876332dfacdf5aa0f566c5c4fb Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 28 Jan 2021 18:01:20 +0100 Subject: [PATCH 1/5] FEAT: Rename maybe_uninit to Array::uninit and move to base type Old type situation: `Array, D>::maybe_uninit()` New type situation: `Array::uninit()` The link between the regular array storage type and the maybeuninit version of it is made explicit in the DataOwned trait and this makes it much easier to work with this constructor in generic code. The new name is "uninit", just like many types in std (likej `Box::uninit`). The old name is deprecated. Because of the unfortunate generics situation of the old name, the implementation is a copy & paste (short allegory - inside ArrayBase::uninit we have types S and S::MaybeUninit "available" as known type, inside ArrayBase::maybe_uninit we only have a `DataOwned>` type "avaialable" and no way to find the corresponding "plain" storage type.) --- benches/bench1.rs | 2 +- examples/sort-axis.rs | 2 +- src/data_traits.rs | 10 ++++++++++ src/impl_constructors.rs | 33 +++++++++++++++++++++++++-------- src/linalg/impl_linalg.rs | 2 +- src/stacking.rs | 4 ++-- src/zip/mod.rs | 2 +- tests/array-construct.rs | 10 +++++----- 8 files changed, 46 insertions(+), 19 deletions(-) diff --git a/benches/bench1.rs b/benches/bench1.rs index c446b1e20..c24729d6f 100644 --- a/benches/bench1.rs +++ b/benches/bench1.rs @@ -271,7 +271,7 @@ fn add_2d_alloc_zip_uninit(bench: &mut test::Bencher) { let a = Array::::zeros((ADD2DSZ, ADD2DSZ)); let b = Array::::zeros((ADD2DSZ, ADD2DSZ)); bench.iter(|| unsafe { - let mut c = Array::, _>::maybe_uninit(a.dim()); + let mut c = Array::::uninit(a.dim()); azip!((&a in &a, &b in &b, c in c.raw_view_mut().cast::()) c.write(a + b) ); diff --git a/examples/sort-axis.rs b/examples/sort-axis.rs index a02dad603..09410e819 100644 --- a/examples/sort-axis.rs +++ b/examples/sort-axis.rs @@ -102,7 +102,7 @@ where assert_eq!(axis_len, perm.indices.len()); debug_assert!(perm.correct()); - let mut result = Array::maybe_uninit(self.dim()); + let mut result = Array::uninit(self.dim()); unsafe { // logically move ownership of all elements from self into result diff --git a/src/data_traits.rs b/src/data_traits.rs index ef8a3ceaa..4bbe870fa 100644 --- a/src/data_traits.rs +++ b/src/data_traits.rs @@ -9,7 +9,9 @@ //! The data (inner representation) traits for ndarray use rawpointer::PointerExt; + use std::mem::{self, size_of}; +use std::mem::MaybeUninit; use std::ptr::NonNull; use alloc::sync::Arc; use alloc::vec::Vec; @@ -401,6 +403,10 @@ unsafe impl<'a, A> DataMut for ViewRepr<&'a mut A> {} /// /// ***Internal trait, see `Data`.*** pub unsafe trait DataOwned: Data { + /// Corresponding owned data with MaybeUninit elements + type MaybeUninit: DataOwned> + + RawDataSubst; + #[doc(hidden)] fn new(elements: Vec) -> Self; @@ -421,6 +427,8 @@ unsafe impl DataShared for OwnedArcRepr {} unsafe impl<'a, A> DataShared for ViewRepr<&'a A> {} unsafe impl DataOwned for OwnedRepr { + type MaybeUninit = OwnedRepr>; + fn new(elements: Vec) -> Self { OwnedRepr::from(elements) } @@ -430,6 +438,8 @@ unsafe impl DataOwned for OwnedRepr { } unsafe impl DataOwned for OwnedArcRepr { + type MaybeUninit = OwnedArcRepr>; + fn new(elements: Vec) -> Self { OwnedArcRepr(Arc::new(OwnedRepr::from(elements))) } diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 622448c58..56ba3fcfe 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -480,7 +480,7 @@ where /// Create an array with uninitalized elements, shape `shape`. /// - /// Prefer to use [`maybe_uninit()`](ArrayBase::maybe_uninit) if possible, because it is + /// Prefer to use [`uninit()`](ArrayBase::uninit) if possible, because it is /// easier to use correctly. /// /// **Panics** if the number of elements in `shape` would overflow isize. @@ -512,13 +512,7 @@ where v.set_len(size); Self::from_shape_vec_unchecked(shape, v) } -} -impl ArrayBase -where - S: DataOwned>, - D: Dimension, -{ /// Create an array with uninitalized elements, shape `shape`. /// /// The uninitialized elements of type `A` are represented by the type `MaybeUninit`, @@ -550,7 +544,7 @@ where /// /// fn shift_by_two(a: &Array2) -> Array2 { /// // create an uninitialized array - /// let mut b = Array2::maybe_uninit(a.dim()); + /// let mut b = Array2::uninit(a.dim()); /// /// // two first columns in b are two last in a /// // rest of columns in b are the initial columns in a @@ -580,6 +574,29 @@ where /// /// # shift_by_two(&Array2::zeros((8, 8))); /// ``` + pub fn uninit(shape: Sh) -> ArrayBase + where + Sh: ShapeBuilder, + { + unsafe { + let shape = shape.into_shape(); + let size = size_of_shape_checked_unwrap!(&shape.dim); + let mut v = Vec::with_capacity(size); + v.set_len(size); + ArrayBase::from_shape_vec_unchecked(shape, v) + } + } +} + +impl ArrayBase +where + S: DataOwned>, + D: Dimension, +{ + /// Create an array with uninitalized elements, shape `shape`. + /// + /// This method has been renamed to `uninit` + #[deprecated(note = "Renamed to `uninit`", since = "0.15.0")] pub fn maybe_uninit(shape: Sh) -> Self where Sh: ShapeBuilder, diff --git a/src/linalg/impl_linalg.rs b/src/linalg/impl_linalg.rs index 0b559d9c5..878444c26 100644 --- a/src/linalg/impl_linalg.rs +++ b/src/linalg/impl_linalg.rs @@ -326,7 +326,7 @@ where // Avoid initializing the memory in vec -- set it during iteration unsafe { - let mut c = Array1::maybe_uninit(m); + let mut c = Array1::uninit(m); general_mat_vec_mul_impl(A::one(), self, rhs, A::zero(), c.raw_view_mut().cast::()); c.assume_init() } diff --git a/src/stacking.rs b/src/stacking.rs index 604a7cc5b..6f4f378f4 100644 --- a/src/stacking.rs +++ b/src/stacking.rs @@ -91,7 +91,7 @@ where // we can safely use uninitialized values here because we will // overwrite every one of them. - let mut res = Array::maybe_uninit(res_dim); + let mut res = Array::uninit(res_dim); { let mut assign_view = res.view_mut(); @@ -159,7 +159,7 @@ where // we can safely use uninitialized values here because we will // overwrite every one of them. - let mut res = Array::maybe_uninit(res_dim); + let mut res = Array::uninit(res_dim); res.axis_iter_mut(axis) .zip(arrays.iter()) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index dd0bce3f8..19b17e741 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -814,7 +814,7 @@ where pub(crate) fn uninitalized_for_current_layout(&self) -> Array, D> { let is_f = self.prefer_f(); - Array::maybe_uninit(self.dimension.clone().set_f(is_f)) + Array::uninit(self.dimension.clone().set_f(is_f)) } } diff --git a/tests/array-construct.rs b/tests/array-construct.rs index 2c195480f..993c2aa04 100644 --- a/tests/array-construct.rs +++ b/tests/array-construct.rs @@ -205,18 +205,18 @@ fn maybe_uninit_1() { unsafe { // Array - type Mat = Array, D>; + type Mat = Array; - let mut a = Mat::maybe_uninit((10, 10)); + let mut a = Mat::uninit((10, 10)); a.mapv_inplace(|_| MaybeUninit::new(1.)); let a_init = a.assume_init(); assert_eq!(a_init, Array2::from_elem(a_init.dim(), 1.)); // ArcArray - type ArcMat = ArcArray, D>; + type ArcMat = ArcArray; - let mut a = ArcMat::maybe_uninit((10, 10)); + let mut a = ArcMat::uninit((10, 10)); a.mapv_inplace(|_| MaybeUninit::new(1.)); let a2 = a.clone(); @@ -228,7 +228,7 @@ fn maybe_uninit_1() { assert_eq!(av_init, Array2::from_elem(a_init.dim(), 1.)); // RawArrayViewMut - let mut a = Mat::maybe_uninit((10, 10)); + let mut a = Mat::uninit((10, 10)); let v = a.raw_view_mut(); Zip::from(v) .for_each(|ptr| *(*ptr).as_mut_ptr() = 1.); From 00a8d4871a6af76413a3084988cd972b1f5f83c5 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 30 Jan 2021 11:17:20 +0100 Subject: [PATCH 2/5] FEAT: Add constructor method build_uninit This method allows creating any owned array and initializing it - the method helps by giving mutable access even to normally copy-on-write arrays, because when first created, the array is unshared. --- src/data_traits.rs | 7 +++++++ src/impl_constructors.rs | 33 +++++++++++++++++++++++++++++++++ src/impl_methods.rs | 11 +++++++++++ 3 files changed, 51 insertions(+) diff --git a/src/data_traits.rs b/src/data_traits.rs index 4bbe870fa..9e814a28c 100644 --- a/src/data_traits.rs +++ b/src/data_traits.rs @@ -402,6 +402,13 @@ unsafe impl<'a, A> DataMut for ViewRepr<&'a mut A> {} /// A representation that is a unique or shared owner of its data. /// /// ***Internal trait, see `Data`.*** +// The owned storage represents the ownership and allocation of the array's elements. +// The storage may be unique or shared ownership style; it must be an aliasable owner +// (permit aliasing pointers, such as our separate array head pointer). +// +// The array storage must be initially mutable - copy on write arrays may require copying for +// unsharing storage before mutating it. The initially allocated storage must be mutable so +// that it can be mutated directly - through .raw_view_mut_unchecked() - for initialization. pub unsafe trait DataOwned: Data { /// Corresponding owned data with MaybeUninit elements type MaybeUninit: DataOwned> diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 56ba3fcfe..449362133 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -586,6 +586,39 @@ where ArrayBase::from_shape_vec_unchecked(shape, v) } } + + /// Create an array with uninitalized elements, shape `shape`. + /// + /// The uninitialized elements of type `A` are represented by the type `MaybeUninit`, + /// an easier way to handle uninit values correctly. + /// + /// The `builder` closure gets unshared access to the array through a raw view + /// and can use it to modify the array before it is returned. This allows initializing + /// the array for any owned array type (avoiding clone requirements for copy-on-write, + /// because the array is unshared when initially created). + /// + /// Only *when* the array is completely initialized with valid elements, can it be + /// converted to an array of `A` elements using [`.assume_init()`]. + /// + /// **Panics** if the number of elements in `shape` would overflow isize. + /// + /// ### Safety + /// + /// The whole of the array must be initialized before it is converted + /// using [`.assume_init()`] or otherwise traversed. + /// + pub(crate) fn build_uninit(shape: Sh, builder: F) -> ArrayBase + where + Sh: ShapeBuilder, + F: FnOnce(RawArrayViewMut, D>), + { + let mut array = Self::uninit(shape); + // Safe because: the array is unshared here + unsafe { + builder(array.raw_view_mut_unchecked()); + } + array + } } impl ArrayBase diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 8f604756d..8b1b5a333 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -1383,6 +1383,17 @@ where unsafe { RawArrayViewMut::new(self.ptr, self.dim.clone(), self.strides.clone()) } } + /// Return a raw mutable view of the array. + /// + /// Safety: The caller must ensure that the owned array is unshared when this is called + #[inline] + pub(crate) unsafe fn raw_view_mut_unchecked(&mut self) -> RawArrayViewMut + where + S: DataOwned, + { + RawArrayViewMut::new(self.ptr, self.dim.clone(), self.strides.clone()) + } + /// Return the array’s data as a slice, if it is contiguous and in standard order. /// Return `None` otherwise. /// From 82150cb0db92182bb4d4f23b98c99dacb3d22b35 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 30 Jan 2021 11:21:13 +0100 Subject: [PATCH 3/5] FEAT: Add internal map_collect method that collects to any array storage This generalizes .map_collect() so that it supports any DataOwned array storage. It is a crate internal method initially - it can be exported later with more experience. --- src/zip/mod.rs | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 19b17e741..f29d3e1ee 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -9,6 +9,7 @@ #[macro_use] mod zipmacro; +#[cfg(feature = "rayon")] use std::mem::MaybeUninit; use alloc::vec::Vec; @@ -811,6 +812,7 @@ where FoldWhile::Continue(acc) } + #[cfg(feature = "rayon")] pub(crate) fn uninitalized_for_current_layout(&self) -> Array, D> { let is_f = self.prefer_f(); @@ -1079,17 +1081,27 @@ macro_rules! map_impl { /// /// If all inputs are c- or f-order respectively, that is preserved in the output. pub fn map_collect(self, f: impl FnMut($($p::Item,)* ) -> R) -> Array { - // Make uninit result - let mut output = self.uninitalized_for_current_layout::(); + self.map_collect_owned(f) + } - // Use partial to counts the number of filled elements, and can drop the right - // number of elements on unwinding (if it happens during apply/collect). + pub(crate) fn map_collect_owned(self, f: impl FnMut($($p::Item,)* ) -> R) + -> ArrayBase + where S: DataOwned + { + // safe because: all elements are written before the array is completed + + let shape = self.dimension.clone().set_f(self.prefer_f()); + let output = >::build_uninit(shape, |output| { + // Use partial to count the number of filled elements, and can drop the right + // number of elements on unwinding (if it happens during apply/collect). + unsafe { + let output_view = output.cast::(); + self.and(output_view) + .collect_with_partial(f) + .release_ownership(); + } + }); unsafe { - let output_view = output.raw_view_mut().cast::(); - self.and(output_view) - .collect_with_partial(f) - .release_ownership(); - output.assume_init() } } From fdb96189f759123c8526b423aef1ef2138c29ac9 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 30 Jan 2021 11:53:03 +0100 Subject: [PATCH 4/5] API: Deprecate Array::uninitialized. Use Array::uninit instead Array::uninitialized is very hard to use correctly (internally and for our users). Prefer the new API with Array::uninit using `MaybeUninit<_>` instead. --- src/impl_constructors.rs | 73 +++++++++++++++++++++------------------- tests/array-construct.rs | 10 +++++- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 449362133..1e743fbbf 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -478,41 +478,6 @@ where } } - /// Create an array with uninitalized elements, shape `shape`. - /// - /// Prefer to use [`uninit()`](ArrayBase::uninit) if possible, because it is - /// easier to use correctly. - /// - /// **Panics** if the number of elements in `shape` would overflow isize. - /// - /// ### Safety - /// - /// Accessing uninitalized values is undefined behaviour. You must overwrite *all* the elements - /// in the array after it is created; for example using - /// [`raw_view_mut`](ArrayBase::raw_view_mut) or other low-level element access. - /// - /// The contents of the array is indeterminate before initialization and it - /// is an error to perform operations that use the previous values. For - /// example it would not be legal to use `a += 1.;` on such an array. - /// - /// This constructor is limited to elements where `A: Copy` (no destructors) - /// to avoid users shooting themselves too hard in the foot. - /// - /// (Also note that the constructors `from_shape_vec` and - /// `from_shape_vec_unchecked` allow the user yet more control, in the sense - /// that Arrays can be created from arbitrary vectors.) - pub unsafe fn uninitialized(shape: Sh) -> Self - where - A: Copy, - Sh: ShapeBuilder, - { - let shape = shape.into_shape(); - let size = size_of_shape_checked_unwrap!(&shape.dim); - let mut v = Vec::with_capacity(size); - v.set_len(size); - Self::from_shape_vec_unchecked(shape, v) - } - /// Create an array with uninitalized elements, shape `shape`. /// /// The uninitialized elements of type `A` are represented by the type `MaybeUninit`, @@ -619,6 +584,44 @@ where } array } + + #[deprecated(note = "This method is hard to use correctly. Use `uninit` instead.", + since = "0.15.0")] + /// Create an array with uninitalized elements, shape `shape`. + /// + /// Prefer to use [`uninit()`](ArrayBase::uninit) if possible, because it is + /// easier to use correctly. + /// + /// **Panics** if the number of elements in `shape` would overflow isize. + /// + /// ### Safety + /// + /// Accessing uninitalized values is undefined behaviour. You must overwrite *all* the elements + /// in the array after it is created; for example using + /// [`raw_view_mut`](ArrayBase::raw_view_mut) or other low-level element access. + /// + /// The contents of the array is indeterminate before initialization and it + /// is an error to perform operations that use the previous values. For + /// example it would not be legal to use `a += 1.;` on such an array. + /// + /// This constructor is limited to elements where `A: Copy` (no destructors) + /// to avoid users shooting themselves too hard in the foot. + /// + /// (Also note that the constructors `from_shape_vec` and + /// `from_shape_vec_unchecked` allow the user yet more control, in the sense + /// that Arrays can be created from arbitrary vectors.) + pub unsafe fn uninitialized(shape: Sh) -> Self + where + A: Copy, + Sh: ShapeBuilder, + { + let shape = shape.into_shape(); + let size = size_of_shape_checked_unwrap!(&shape.dim); + let mut v = Vec::with_capacity(size); + v.set_len(size); + Self::from_shape_vec_unchecked(shape, v) + } + } impl ArrayBase diff --git a/tests/array-construct.rs b/tests/array-construct.rs index 993c2aa04..2b72ab039 100644 --- a/tests/array-construct.rs +++ b/tests/array-construct.rs @@ -52,6 +52,7 @@ fn test_arcarray_thread_safe() { #[test] #[cfg(feature = "std")] +#[allow(deprecated)] // uninitialized fn test_uninit() { unsafe { let mut a = Array::::uninitialized((3, 4).f()); @@ -192,12 +193,19 @@ fn deny_wraparound_from_shape_fn() { #[should_panic] #[test] -fn deny_wraparound_uninit() { +#[allow(deprecated)] // uninitialized +fn deny_wraparound_uninitialized() { unsafe { let _five_large = Array::::uninitialized((3, 7, 29, 36760123, 823996703)); } } +#[should_panic] +#[test] +fn deny_wraparound_uninit() { + let _five_large = Array::::uninit((3, 7, 29, 36760123, 823996703)); +} + #[test] fn maybe_uninit_1() { From 802aa5419d220fce01ae1b4f1dde3261a3da0d5f Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 30 Jan 2021 12:07:13 +0100 Subject: [PATCH 5/5] MAINT: Increase MSRV to 1.49 1.49 had a lot of fixes to associated types and bounds, and that enables the new DataOwned changes to compile. --- .github/workflows/ci.yml | 2 +- src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86aa8b727..92c184ca8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ jobs: - stable - beta - nightly - - 1.42.0 # MSRV + - 1.49.0 # MSRV steps: - uses: actions/checkout@v2 diff --git a/src/lib.rs b/src/lib.rs index 11064d32d..e9a60af15 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -62,7 +62,7 @@ //! needs matching memory layout to be efficient (with some exceptions). //! + Efficient floating point matrix multiplication even for very large //! matrices; can optionally use BLAS to improve it further. -//! - **Requires Rust 1.42 or later** +//! - **Requires Rust 1.49 or later** //! //! ## Crate Feature Flags //!