From 876c5cb80d2e400056428f3e108d31b14bacad79 Mon Sep 17 00:00:00 2001 From: bluss Date: Tue, 14 Apr 2020 22:36:55 +0200 Subject: [PATCH 1/7] FEAT: Implement internal Array::maybe_uninit and assume_init Use MaybeUninit to handle uninitialized data safely. --- src/impl_constructors.rs | 20 ++++++++++++++++++++ src/impl_owned_array.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 024a1e3b4..54b3a8f14 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -13,6 +13,7 @@ #![allow(clippy::match_wild_err_arm)] use num_traits::{Float, One, Zero}; +use std::mem::MaybeUninit; use crate::dimension; use crate::error::{self, ShapeError}; @@ -517,3 +518,22 @@ where Self::from_shape_vec_unchecked(shape, v) } } + +impl ArrayBase +where + S: DataOwned>, + D: Dimension, +{ + pub(crate) fn maybe_uninit(shape: Sh) -> Self + 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); + Self::from_shape_vec_unchecked(shape, v) + } + } +} diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 96075593f..174a67031 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -1,4 +1,8 @@ +use std::mem::MaybeUninit; +use std::mem::transmute; + use crate::imp_prelude::*; +use crate::OwnedRepr; /// Methods specific to `Array0`. /// @@ -57,3 +61,33 @@ where self.data.0 } } + +/// 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, + } + } +} From 5d0159e40db141aec14574201209f11abe907392 Mon Sep 17 00:00:00 2001 From: bluss Date: Mon, 13 Apr 2020 17:20:37 +0200 Subject: [PATCH 2/7] FEAT: Add method Zip::apply_collect to collect results into Array Use Array::maybe_uniit to not need to zero result elements; Restrict to Copy, because we don't implement correct drop on panic yet. --- src/zip/mod.rs | 23 +++++++++++++++++++++++ tests/azip.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 33317253e..f140a12d9 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -982,6 +982,29 @@ macro_rules! map_impl { dimension: self.dimension, } } + + /// Apply and collect the results into a new array, which has the same size as the + /// inputs. + /// + /// If all inputs are c- or f-order respectively, that is preserved in the output. + /// + /// Restricted to functions that produce copyable results for technical reasons; other + /// cases are not yet implemented. + pub fn apply_collect(self, mut f: impl FnMut($($p::Item,)* ) -> R) -> Array + where R: Copy, + { + unsafe { + let is_c = self.layout.is(CORDER); + let is_f = !is_c && self.layout.is(FORDER); + let mut output = Array::maybe_uninit(self.dimension.clone().set_f(is_f)); + self.and(&mut output) + .apply(move |$($p, )* output_| { + std::ptr::write(output_.as_mut_ptr(), f($($p ),*)); + }); + output.assume_init() + } + } + ); /// Split the `Zip` evenly in two. diff --git a/tests/azip.rs b/tests/azip.rs index 885db85b7..b969d5365 100644 --- a/tests/azip.rs +++ b/tests/azip.rs @@ -49,6 +49,34 @@ fn test_azip2_3() { assert!(a != b); } +#[test] +#[cfg(feature = "approx")] +fn test_zip_collect() { + use approx::assert_abs_diff_eq; + + // test Zip::apply_collect and that it preserves c/f layout. + + let b = Array::from_shape_fn((5, 10), |(i, j)| 1. / (i + 2 * j + 1) as f32); + let c = Array::from_shape_fn((5, 10), |(i, j)| f32::exp((i + j) as f32)); + + { + let a = Zip::from(&b).and(&c).apply_collect(|x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); + assert_eq!(a.strides(), b.strides()); + } + + { + let b = b.t(); + let c = c.t(); + + let a = Zip::from(&b).and(&c).apply_collect(|x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); + assert_eq!(a.strides(), b.strides()); + } +} + #[test] fn test_azip_syntax_trailing_comma() { let mut b = Array::::zeros((5, 5)); From 07cf42dc02407b1c92bcae0306a55960f60d6499 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 15 Apr 2020 19:43:03 +0200 Subject: [PATCH 3/7] TEST: Update benchmarks for addition to also test Zip::apply_collect() --- benches/bench1.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/benches/bench1.rs b/benches/bench1.rs index 190ab5065..d7fa9f163 100644 --- a/benches/bench1.rs +++ b/benches/bench1.rs @@ -258,23 +258,35 @@ fn add_2d_zip(bench: &mut test::Bencher) { } #[bench] -fn add_2d_alloc(bench: &mut test::Bencher) { +fn add_2d_alloc_plus(bench: &mut test::Bencher) { let a = Array::::zeros((ADD2DSZ, ADD2DSZ)); let b = Array::::zeros((ADD2DSZ, ADD2DSZ)); bench.iter(|| &a + &b); } #[bench] -fn add_2d_zip_alloc(bench: &mut test::Bencher) { +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::uninitialized(a.dim()); - azip!((&a in &a, &b in &b, c in &mut c) *c = a + b); + azip!((&a in &a, &b in &b, c in c.raw_view_mut()) + std::ptr::write(c, a + b) + ); c }); } +#[bench] +fn add_2d_alloc_zip_collect(bench: &mut test::Bencher) { + let a = Array::::zeros((ADD2DSZ, ADD2DSZ)); + let b = Array::::zeros((ADD2DSZ, ADD2DSZ)); + bench.iter(|| { + Zip::from(&a).and(&b).apply_collect(|&x, &y| x + y) + }); +} + + #[bench] fn add_2d_assign_ops(bench: &mut test::Bencher) { let mut a = Array::::zeros((ADD2DSZ, ADD2DSZ)); From 5c3bc3df2880c26beea6485349a01ef6032c4e55 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 15 Apr 2020 21:38:05 +0200 Subject: [PATCH 4/7] FEAT: Add Zip::apply_assign_into --- src/argument_traits.rs | 31 +++++++++++++++++++++++++++++++ src/lib.rs | 2 ++ src/zip/mod.rs | 33 +++++++++++++++++++++++++-------- src/zip/zipmacro.rs | 6 ++++++ tests/azip.rs | 31 +++++++++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 src/argument_traits.rs diff --git a/src/argument_traits.rs b/src/argument_traits.rs new file mode 100644 index 000000000..a93e33a12 --- /dev/null +++ b/src/argument_traits.rs @@ -0,0 +1,31 @@ +use std::cell::Cell; +use std::mem::MaybeUninit; + + +/// A producer element that can be assigned to once +pub trait AssignElem { + /// Assign the value `input` to the element that self represents. + fn assign_elem(self, input: T); +} + +/// Assignable element, simply `*self = input`. +impl<'a, T> AssignElem for &'a mut T { + fn assign_elem(self, input: T) { + *self = input; + } +} + +/// Assignable element, simply `self.set(input)`. +impl<'a, T> AssignElem for &'a Cell { + fn assign_elem(self, input: T) { + self.set(input); + } +} + +/// Assignable element, the item in the MaybeUninit is overwritten (prior value, if any, is not +/// read or dropped). +impl<'a, T> AssignElem for &'a mut MaybeUninit { + fn assign_elem(self, input: T) { + *self = MaybeUninit::new(input); + } +} diff --git a/src/lib.rs b/src/lib.rs index f9a208df1..c8d750d7a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,6 +149,8 @@ mod array_approx; mod array_serde; mod arrayformat; mod arraytraits; +mod argument_traits; +pub use crate::argument_traits::AssignElem; mod data_traits; pub use crate::aliases::*; diff --git a/src/zip/mod.rs b/src/zip/mod.rs index f140a12d9..a2fd08502 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -10,6 +10,7 @@ mod zipmacro; use crate::imp_prelude::*; +use crate::AssignElem; use crate::IntoDimension; use crate::Layout; use crate::NdIndex; @@ -579,6 +580,7 @@ pub struct Zip { layout: Layout, } + impl Zip<(P,), D> where D: Dimension, @@ -990,21 +992,36 @@ macro_rules! map_impl { /// /// Restricted to functions that produce copyable results for technical reasons; other /// cases are not yet implemented. - pub fn apply_collect(self, mut f: impl FnMut($($p::Item,)* ) -> R) -> Array + pub fn apply_collect(self, f: impl FnMut($($p::Item,)* ) -> R) -> Array where R: Copy, { + // To support non-Copy elements, implementation of dropping partial array (on + // panic) is needed + let is_c = self.layout.is(CORDER); + let is_f = !is_c && self.layout.is(FORDER); + let mut output = Array::maybe_uninit(self.dimension.clone().set_f(is_f)); + self.apply_assign_into(&mut output, f); unsafe { - let is_c = self.layout.is(CORDER); - let is_f = !is_c && self.layout.is(FORDER); - let mut output = Array::maybe_uninit(self.dimension.clone().set_f(is_f)); - self.and(&mut output) - .apply(move |$($p, )* output_| { - std::ptr::write(output_.as_mut_ptr(), f($($p ),*)); - }); output.assume_init() } } + /// Apply and assign the results into the producer `into`, which should have the same + /// size as the other inputs. + /// + /// The producer should have assignable items as dictated by the `AssignElem` trait, + /// for example `&mut R`. + pub fn apply_assign_into(self, into: Q, mut f: impl FnMut($($p::Item,)* ) -> R) + where Q: IntoNdProducer, + Q::Item: AssignElem + { + self.and(into) + .apply(move |$($p, )* output_| { + output_.assign_elem(f($($p ),*)); + }); + } + + ); /// Split the `Zip` evenly in two. diff --git a/src/zip/zipmacro.rs b/src/zip/zipmacro.rs index ea616a05e..3ac73b8fe 100644 --- a/src/zip/zipmacro.rs +++ b/src/zip/zipmacro.rs @@ -122,6 +122,12 @@ macro_rules! azip { $(.and($prod))* .$apply(|$first_pat, $($pat),*| $body) }; + + // Unindexed with one or more producer, no loop body + (@build $apply:ident $first_prod:expr $(, $prod:expr)* $(,)?) => { + $crate::Zip::from($first_prod) + $(.and($prod))* + }; // catch-all rule (@build $($t:tt)*) => { compile_error!("Invalid syntax in azip!()") }; ($($t:tt)*) => { diff --git a/tests/azip.rs b/tests/azip.rs index b969d5365..90da41853 100644 --- a/tests/azip.rs +++ b/tests/azip.rs @@ -77,6 +77,37 @@ fn test_zip_collect() { } } +#[test] +#[cfg(feature = "approx")] +fn test_zip_assign_into() { + use approx::assert_abs_diff_eq; + + let mut a = Array::::zeros((5, 10)); + let b = Array::from_shape_fn((5, 10), |(i, j)| 1. / (i + 2 * j + 1) as f32); + let c = Array::from_shape_fn((5, 10), |(i, j)| f32::exp((i + j) as f32)); + + Zip::from(&b).and(&c).apply_assign_into(&mut a, |x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); +} + +#[test] +#[cfg(feature = "approx")] +fn test_zip_assign_into_cell() { + use approx::assert_abs_diff_eq; + use std::cell::Cell; + + let a = Array::, _>::default((5, 10)); + let b = Array::from_shape_fn((5, 10), |(i, j)| 1. / (i + 2 * j + 1) as f32); + let c = Array::from_shape_fn((5, 10), |(i, j)| f32::exp((i + j) as f32)); + + Zip::from(&b).and(&c).apply_assign_into(&a, |x, y| x + y); + let a2 = a.mapv(|elt| elt.get()); + + assert_abs_diff_eq!(a2, &b + &c, epsilon = 1e-6); +} + + #[test] fn test_azip_syntax_trailing_comma() { let mut b = Array::::zeros((5, 5)); From f6d6f01897807529bdeb72b036a3f05ec506372c Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 16 Apr 2020 11:58:13 +0200 Subject: [PATCH 5/7] FIX: Factor out making the uninit array in Zip --- src/zip/mod.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index a2fd08502..52f98699f 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -9,6 +9,8 @@ #[macro_use] mod zipmacro; +use std::mem::MaybeUninit; + use crate::imp_prelude::*; use crate::AssignElem; use crate::IntoDimension; @@ -737,6 +739,12 @@ where self.dimension[unroll_axis] = inner_len; FoldWhile::Continue(acc) } + + pub(crate) fn uninitalized_for_current_layout(&self) -> Array, D> + { + let is_f = !self.layout.is(CORDER) && self.layout.is(FORDER); + Array::maybe_uninit(self.dimension.clone().set_f(is_f)) + } } /* @@ -997,9 +1005,7 @@ macro_rules! map_impl { { // To support non-Copy elements, implementation of dropping partial array (on // panic) is needed - let is_c = self.layout.is(CORDER); - let is_f = !is_c && self.layout.is(FORDER); - let mut output = Array::maybe_uninit(self.dimension.clone().set_f(is_f)); + let mut output = self.uninitalized_for_current_layout::(); self.apply_assign_into(&mut output, f); unsafe { output.assume_init() From 739dd6aa541c89ac9b5d5ebbf0a3f807ce664d4b Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 16 Apr 2020 12:56:36 +0200 Subject: [PATCH 6/7] FEAT: Add Zip::par_apply_collect, par_apply_assign_into --- src/parallel/impl_par_methods.rs | 53 +++++++++++++++++++++++++++----- tests/par_zip.rs | 42 +++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/src/parallel/impl_par_methods.rs b/src/parallel/impl_par_methods.rs index 88fe769bf..867cd07ad 100644 --- a/src/parallel/impl_par_methods.rs +++ b/src/parallel/impl_par_methods.rs @@ -1,4 +1,5 @@ -use crate::{ArrayBase, DataMut, Dimension, NdProducer, Zip}; +use crate::{Array, ArrayBase, DataMut, Dimension, IntoNdProducer, NdProducer, Zip}; +use crate::AssignElem; use crate::parallel::prelude::*; @@ -43,7 +44,7 @@ where // Zip macro_rules! zip_impl { - ($([$($p:ident)*],)+) => { + ($([$notlast:ident $($p:ident)*],)+) => { $( #[allow(non_snake_case)] impl Zip<($($p,)*), D> @@ -63,16 +64,52 @@ macro_rules! zip_impl { { self.into_par_iter().for_each(move |($($p,)*)| function($($p),*)) } + + expand_if!(@bool [$notlast] + + /// Apply and collect the results into a new array, which has the same size as the + /// inputs. + /// + /// If all inputs are c- or f-order respectively, that is preserved in the output. + /// + /// Restricted to functions that produce copyable results for technical reasons; other + /// cases are not yet implemented. + pub fn par_apply_collect(self, f: impl Fn($($p::Item,)* ) -> R + Sync + Send) -> Array + where R: Copy + Send + { + let mut output = self.uninitalized_for_current_layout::(); + self.par_apply_assign_into(&mut output, f); + unsafe { + output.assume_init() + } + } + + /// Apply and assign the results into the producer `into`, which should have the same + /// size as the other inputs. + /// + /// The producer should have assignable items as dictated by the `AssignElem` trait, + /// for example `&mut R`. + pub fn par_apply_assign_into(self, into: Q, f: impl Fn($($p::Item,)* ) -> R + Sync + Send) + where Q: IntoNdProducer, + Q::Item: AssignElem + Send, + Q::Output: Send, + { + self.and(into) + .par_apply(move |$($p, )* output_| { + output_.assign_elem(f($($p ),*)); + }); + } + ); } )+ } } zip_impl! { - [P1], - [P1 P2], - [P1 P2 P3], - [P1 P2 P3 P4], - [P1 P2 P3 P4 P5], - [P1 P2 P3 P4 P5 P6], + [true P1], + [true P1 P2], + [true P1 P2 P3], + [true P1 P2 P3 P4], + [true P1 P2 P3 P4 P5], + [false P1 P2 P3 P4 P5 P6], } diff --git a/tests/par_zip.rs b/tests/par_zip.rs index f10f3acde..a24dc8e80 100644 --- a/tests/par_zip.rs +++ b/tests/par_zip.rs @@ -70,3 +70,45 @@ fn test_zip_index_4() { assert_eq!(*elt, j); } } + +#[test] +#[cfg(feature = "approx")] +fn test_zip_collect() { + use approx::assert_abs_diff_eq; + + // test Zip::apply_collect and that it preserves c/f layout. + + let b = Array::from_shape_fn((M, N), |(i, j)| 1. / (i + 2 * j + 1) as f32); + let c = Array::from_shape_fn((M, N), |(i, j)| f32::ln((1 + i + j) as f32)); + + { + let a = Zip::from(&b).and(&c).par_apply_collect(|x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); + assert_eq!(a.strides(), b.strides()); + } + + { + let b = b.t(); + let c = c.t(); + + let a = Zip::from(&b).and(&c).par_apply_collect(|x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); + assert_eq!(a.strides(), b.strides()); + } +} + +#[test] +#[cfg(feature = "approx")] +fn test_zip_assign_into() { + use approx::assert_abs_diff_eq; + + let mut a = Array::::zeros((M, N)); + let b = Array::from_shape_fn((M, N), |(i, j)| 1. / (i + 2 * j + 1) as f32); + let c = Array::from_shape_fn((M, N), |(i, j)| f32::ln((1 + i + j) as f32)); + + Zip::from(&b).and(&c).par_apply_assign_into(&mut a, |x, y| x + y); + + assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6); +} From f7319b31ce7c84cb2a34532fe9d98f24a2d2115f Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 18 Apr 2020 13:11:30 +0200 Subject: [PATCH 7/7] FIX: Use pub(crate) methods instead of internal trait for Layout (This is just a remainder from before pub(crate) was a feature we could use) --- src/layout/layoutfmt.rs | 1 - src/layout/mod.rs | 19 ++++++------------- src/zip/mod.rs | 1 - 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/layout/layoutfmt.rs b/src/layout/layoutfmt.rs index cb799ca81..d5049512e 100644 --- a/src/layout/layoutfmt.rs +++ b/src/layout/layoutfmt.rs @@ -7,7 +7,6 @@ // except according to those terms. use super::Layout; -use super::LayoutPriv; const LAYOUT_NAMES: &[&str] = &["C", "F"]; diff --git a/src/layout/mod.rs b/src/layout/mod.rs index 741a0e054..57983cc3b 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -1,35 +1,28 @@ mod layoutfmt; -// public but users don't interact with it +// public struct but users don't interact with it #[doc(hidden)] /// Memory layout description #[derive(Copy, Clone)] pub struct Layout(u32); -pub trait LayoutPriv: Sized { - fn new(x: u32) -> Self; - fn and(self, flag: Self) -> Self; - fn is(self, flag: u32) -> bool; - fn flag(self) -> u32; -} - -impl LayoutPriv for Layout { +impl Layout { #[inline(always)] - fn new(x: u32) -> Self { + pub(crate) fn new(x: u32) -> Self { Layout(x) } #[inline(always)] - fn is(self, flag: u32) -> bool { + pub(crate) fn is(self, flag: u32) -> bool { self.0 & flag != 0 } #[inline(always)] - fn and(self, flag: Layout) -> Layout { + pub(crate) fn and(self, flag: Layout) -> Layout { Layout(self.0 & flag.0) } #[inline(always)] - fn flag(self) -> u32 { + pub(crate) fn flag(self) -> u32 { self.0 } } diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 52f98699f..0968dff3d 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -18,7 +18,6 @@ use crate::Layout; use crate::NdIndex; use crate::indexes::{indices, Indices}; -use crate::layout::LayoutPriv; use crate::layout::{CORDER, FORDER}; /// Return if the expression is a break value.