From e7e164ead0465ea551175c7d2dd6c1cd1d98a154 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:26:13 +0200 Subject: [PATCH 1/7] FIX: Use ManuallyDrop instead of forget in data_repr ManuallyDrop has the benefit that we use it up front, instead of after any "critical" operations. --- src/data_repr.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/data_repr.rs b/src/data_repr.rs index ccaed85c0..4839a8439 100644 --- a/src/data_repr.rs +++ b/src/data_repr.rs @@ -1,5 +1,6 @@ use std::mem; +use std::mem::ManuallyDrop; use std::ptr::NonNull; use std::slice; use crate::extension::nonnull; @@ -17,11 +18,11 @@ pub struct OwnedRepr { } impl OwnedRepr { - pub(crate) fn from(mut v: Vec) -> Self { + pub(crate) fn from(v: Vec) -> Self { + let mut v = ManuallyDrop::new(v); let len = v.len(); let capacity = v.capacity(); let ptr = nonnull::nonnull_from_vec_data(&mut v); - mem::forget(v); Self { ptr, len, @@ -29,10 +30,8 @@ impl OwnedRepr { } } - pub(crate) fn into_vec(mut self) -> Vec { - let v = self.take_as_vec(); - mem::forget(self); - v + pub(crate) fn into_vec(self) -> Vec { + ManuallyDrop::new(self).take_as_vec() } pub(crate) fn as_slice(&self) -> &[A] { From abc1813c3c23f4616851fc9a74b771a7b90fce76 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:36:07 +0200 Subject: [PATCH 2/7] FEAT: Add constructor Array::maybe_uninit for an uninitialized array --- src/impl_constructors.rs | 59 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 54b3a8f14..804ad417e 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -524,7 +524,64 @@ where S: DataOwned>, D: Dimension, { - pub(crate) fn maybe_uninit(shape: Sh) -> Self + /// 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. + /// + /// 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. + /// + /// ### Examples + /// + /// It is possible to assign individual values through `*elt = MaybeUninit::new(value)` + /// and so on. + /// + /// ``` + /// use ndarray::{s, Array2}; + /// use ndarray::Zip; + /// use ndarray::Axis; + /// + /// // Example Task: Let's create a transposed copy of the input + /// + /// fn shift_by_two(a: &Array2) -> Array2 { + /// // create an uninitialized array + /// let mut b = Array2::maybe_uninit(a.dim()); + /// + /// // two first columns in b are two last in a + /// // rest of columns in b are the initial columns in a + /// + /// assign_to(a.slice(s![.., -2..]), b.slice_mut(s![.., ..2])); + /// assign_to(a.slice(s![.., 2..]), b.slice_mut(s![.., ..-2])); + /// + /// // Now we can promise that `b` is safe to use with all operations + /// unsafe { + /// b.assume_init() + /// } + /// } + /// + /// use ndarray::{IntoNdProducer, AssignElem}; + /// + /// fn assign_to<'a, P1, P2, A>(from: P1, to: P2) + /// where P1: IntoNdProducer, + /// P2: IntoNdProducer, + /// P2::Item: AssignElem, + /// A: Clone + 'a + /// { + /// Zip::from(from) + /// .apply_assign_into(to, A::clone); + /// } + /// + /// # shift_by_two(&Array2::zeros((8, 8))); + /// ``` + pub fn maybe_uninit(shape: Sh) -> Self where Sh: ShapeBuilder, { From 46b294608ec97a52ed22d089ce2a278f245a6434 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:40:13 +0200 Subject: [PATCH 3/7] FEAT: New method .assume_init() for all array types Add representation trait RawDataSubst that array storage types correctly (emulation of higher kinded types). Implement it as a transmute-like operation, using the representation guarantees that MaybeUninit documents. Make it a public method - avaialable on all array types - owned arrays, arcarray, array views and so on. The safety percussions of exposing this transformation are only explored to an extent, but it's a type change without value conversion so there should not be much that can go wrong. Interesting aspects include being able to have one ArcArray handle with the old type and one with the new type, or one owned array with old and a view with new. --- src/data_traits.rs | 35 ++++++++++++++++++ src/impl_owned_array.rs | 33 ----------------- src/impl_special_element_types.rs | 59 +++++++++++++++++++++++++++++++ src/lib.rs | 2 ++ 4 files changed, 96 insertions(+), 33 deletions(-) create mode 100644 src/impl_special_element_types.rs diff --git a/src/data_traits.rs b/src/data_traits.rs index 7e04561bc..8c22a225c 100644 --- a/src/data_traits.rs +++ b/src/data_traits.rs @@ -528,3 +528,38 @@ unsafe impl<'a, A> Data for CowRepr<'a, A> { } unsafe impl<'a, A> DataMut for CowRepr<'a, A> where A: Clone {} + +/// Array representation trait. +/// +/// The RawDataSubst trait maps the element type of array storage, while +/// keeping the same kind of storage. +/// +/// For example, `RawDataSubst` can map the type `OwnedRepr` to `OwnedRepr`. +pub trait RawDataSubst: RawData { + /// The resulting array storage of the same kind but substituted element type + type Output: RawData; +} + +impl RawDataSubst for OwnedRepr { + type Output = OwnedRepr; +} + +impl RawDataSubst for OwnedArcRepr { + type Output = OwnedArcRepr; +} + +impl RawDataSubst for RawViewRepr<*const A> { + type Output = RawViewRepr<*const B>; +} + +impl RawDataSubst for RawViewRepr<*mut A> { + type Output = RawViewRepr<*mut B>; +} + +impl<'a, A: 'a, B: 'a> RawDataSubst for ViewRepr<&'a A> { + type Output = ViewRepr<&'a B>; +} + +impl<'a, A: 'a, B: 'a> RawDataSubst for ViewRepr<&'a mut A> { + type Output = ViewRepr<&'a mut B>; +} diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 8ea10501d..f219b7b69 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -1,8 +1,5 @@ -use std::mem::MaybeUninit; -use std::mem::transmute; use crate::imp_prelude::*; -use crate::OwnedRepr; /// Methods specific to `Array0`. /// @@ -61,33 +58,3 @@ where self.data.into_vec() } } - -/// Methods specific to `Array` of `MaybeUninit`. -/// -/// ***See also all methods for [`ArrayBase`]*** -/// -/// [`ArrayBase`]: struct.ArrayBase.html -impl Array, D> -where - D: Dimension, -{ - /// Assert that the array's storage's elements are all fully initialized, and conver - /// the array from element type `MaybeUninit` to `A`. - pub(crate) unsafe fn assume_init(self) -> Array { - // NOTE: Fully initialized includes elements not reachable in current slicing/view. - // - // Should this method be generalized to all array types? - // (Will need a way to map the RawData to RawData of same kind) - - let Array { data, ptr, dim, strides } = self; - let data = transmute::>, OwnedRepr>(data); - let ptr = ptr.cast::(); - - Array { - data, - ptr, - dim, - strides, - } - } -} diff --git a/src/impl_special_element_types.rs b/src/impl_special_element_types.rs new file mode 100644 index 000000000..9b86625a1 --- /dev/null +++ b/src/impl_special_element_types.rs @@ -0,0 +1,59 @@ +// Copyright 2020 bluss and ndarray developers. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::mem::size_of; +use std::mem::ManuallyDrop; +use std::mem::MaybeUninit; + +use crate::imp_prelude::*; +use crate::RawDataSubst; + + +/// Methods specific to arrays with `MaybeUninit` elements. +/// +/// ***See also all methods for [`ArrayBase`]*** +/// +/// [`ArrayBase`]: struct.ArrayBase.html +impl ArrayBase +where + S: RawDataSubst>, + D: Dimension, +{ + /// **Promise** that the array's elements are all fully initialized, and convert + /// the array from element type `MaybeUninit` to `A`. + /// + /// For owned arrays, the promise must include all of the Array's storage + pub unsafe fn assume_init(self) -> ArrayBase<>::Output, D> { + // NOTE: Fully initialized includes elements not reachable in current slicing/view. + + let ArrayBase { data, ptr, dim, strides } = self; + + // transmute from storage of MaybeUninit to storage of A + let data = unlimited_transmute::(data); + let ptr = ptr.cast::(); + + ArrayBase { + data, + ptr, + dim, + strides, + } + } +} + +/// Transmute from A to B. +/// +/// Like transmute, but does not have the compile-time size check which blocks +/// using regular transmute for "S to S::Output". +/// +/// **Panics** if the size of A and B are different. +unsafe fn unlimited_transmute(data: A) -> B { + assert_eq!(size_of::(), size_of::()); + let old_data = ManuallyDrop::new(data); + (&*old_data as *const A as *const B).read() +} diff --git a/src/lib.rs b/src/lib.rs index 0df209cbb..eb09d31ea 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,6 +159,7 @@ pub use crate::aliases::*; #[allow(deprecated)] pub use crate::data_traits::{ Data, DataClone, DataMut, DataOwned, DataShared, RawData, RawDataClone, RawDataMut, + RawDataSubst, }; mod free_functions; @@ -1483,6 +1484,7 @@ mod impl_constructors; mod impl_methods; mod impl_owned_array; +mod impl_special_element_types; /// Private Methods impl ArrayBase From c4f153fcba1579426959570aff73ecdad80bd725 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:43:32 +0200 Subject: [PATCH 4/7] TEST: Add test for Array::maybe_uninit and .assume_init() --- tests/array-construct.rs | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/array-construct.rs b/tests/array-construct.rs index 6c1caf1ef..97e4ef491 100644 --- a/tests/array-construct.rs +++ b/tests/array-construct.rs @@ -7,6 +7,7 @@ use defmac::defmac; use ndarray::prelude::*; +use ndarray::Zip; #[test] fn test_from_shape_fn() { @@ -194,3 +195,46 @@ fn deny_wraparound_uninit() { let _five_large = Array::::uninitialized((3, 7, 29, 36760123, 823996703)); } } + + +#[test] +fn maybe_uninit_1() { + use std::mem::MaybeUninit; + + unsafe { + // Array + type Mat = Array, D>; + + let mut a = Mat::maybe_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>; + + let mut a = ArcMat::maybe_uninit((10, 10)); + a.mapv_inplace(|_| MaybeUninit::new(1.)); + let a2 = a.clone(); + + let a_init = a.assume_init(); + assert_eq!(a_init, Array2::from_elem(a_init.dim(), 1.)); + + // ArrayView + let av_init = a2.view().assume_init(); + assert_eq!(av_init, Array2::from_elem(a_init.dim(), 1.)); + + // RawArrayViewMut + let mut a = Mat::maybe_uninit((10, 10)); + let v = a.raw_view_mut(); + Zip::from(v) + .apply(|ptr| *(*ptr).as_mut_ptr() = 1.); + + let u = a.raw_view_mut().assume_init(); + + Zip::from(u) + .apply(|ptr| assert_eq!(*ptr, 1.)); + + } +} From ba55e4bf3f20ea205842e7b235ed126b79359109 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:49:07 +0200 Subject: [PATCH 5/7] DOC: Array::uninitialized doc update --- src/impl_constructors.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 804ad417e..a4e236e28 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -467,6 +467,9 @@ where /// Create an array with uninitalized elements, shape `shape`. /// + /// Prefer to use [`maybe_uninit()`](ArrayBase::maybe_uninit) if possible, because it is + /// easier to use correctly. + /// /// **Panics** if the number of elements in `shape` would overflow isize. /// /// ### Safety From a172258080250f8432b5da7353556540985bb849 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 20:51:50 +0200 Subject: [PATCH 6/7] DOC: Update doc for maybe_uninit and .assume_init() --- src/impl_constructors.rs | 6 ++++-- src/impl_special_element_types.rs | 10 +++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index a4e236e28..964003dd8 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -533,20 +533,22 @@ where /// an easier way to handle uninit values correctly. /// /// Only *when* the array is completely initialized with valid elements, can it be - /// converted to an array of `A` elements using [`assume_init()`]. + /// 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. + /// using [`.assume_init()`] or otherwise traversed. /// /// ### Examples /// /// It is possible to assign individual values through `*elt = MaybeUninit::new(value)` /// and so on. /// + /// [`.assume_init()`]: ArrayBase::assume_init + /// /// ``` /// use ndarray::{s, Array2}; /// use ndarray::Zip; diff --git a/src/impl_special_element_types.rs b/src/impl_special_element_types.rs index 9b86625a1..40cba5822 100644 --- a/src/impl_special_element_types.rs +++ b/src/impl_special_element_types.rs @@ -27,7 +27,15 @@ where /// **Promise** that the array's elements are all fully initialized, and convert /// the array from element type `MaybeUninit` to `A`. /// - /// For owned arrays, the promise must include all of the Array's storage + /// For example, it can convert an `Array, D>` to `Array`. + /// + /// ## Safety + /// + /// Safe to use if all the array's elements have been initialized. + /// + /// Note that for owned and shared ownership arrays, the promise must include all of the + /// array's storage; it is for example possible to slice these in place, but that must + /// only be done after all elements have been initialized. pub unsafe fn assume_init(self) -> ArrayBase<>::Output, D> { // NOTE: Fully initialized includes elements not reachable in current slicing/view. From 684b9c4c40778785a39bd26df8229c610c881c53 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 14:38:11 +0200 Subject: [PATCH 7/7] TEST: Update example sort-axis to use MaybeUninit --- examples/sort-axis.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/examples/sort-axis.rs b/examples/sort-axis.rs index 4465464b8..55d20950c 100644 --- a/examples/sort-axis.rs +++ b/examples/sort-axis.rs @@ -102,26 +102,31 @@ where assert_eq!(axis_len, perm.indices.len()); debug_assert!(perm.correct()); - let mut v = Vec::with_capacity(self.len()); - let mut result; + let mut result = Array::maybe_uninit(self.dim()); // panic-critical begin: we must not panic unsafe { - v.set_len(self.len()); - result = Array::from_shape_vec_unchecked(self.dim(), v); + // logically move ownership of all elements from self into result + // the result realizes this ownership at .assume_init() further down + let mut moved_elements = 0; for i in 0..axis_len { let perm_i = perm.indices[i]; Zip::from(result.index_axis_mut(axis, perm_i)) .and(self.index_axis(axis, i)) - .apply(|to, from| copy_nonoverlapping(from, to, 1)); + .apply(|to, from| { + copy_nonoverlapping(from, to.as_mut_ptr(), 1); + moved_elements += 1; + }); } // forget moved array elements but not its vec + // old_storage drops empty let mut old_storage = self.into_raw_vec(); old_storage.set_len(0); - // old_storage drops empty + + debug_assert_eq!(result.len(), moved_elements); + result.assume_init() } // panic-critical end - result } }