From d02b757aca316f96902ebd63ed4d0a71a51acfb3 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 23 May 2020 20:04:55 +0200 Subject: [PATCH] FIX: Use Partial instead of PartialArray Partial is just a contiguous slice, and much simpler than PartialArray; Partial is all that's needed, because the area written will always be contiguous. --- src/lib.rs | 1 + src/parallel/impl_par_methods.rs | 75 +--------------- src/partial.rs | 88 +++++++++++++++++++ src/zip/mod.rs | 21 +++-- src/zip/partial_array.rs | 144 ------------------------------- 5 files changed, 104 insertions(+), 225 deletions(-) create mode 100644 src/partial.rs delete mode 100644 src/zip/partial_array.rs diff --git a/src/lib.rs b/src/lib.rs index b40eee01a..f65e82ec9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -176,6 +176,7 @@ mod linalg_traits; mod linspace; mod logspace; mod numeric_util; +mod partial; mod shape_builder; #[macro_use] mod slice; diff --git a/src/parallel/impl_par_methods.rs b/src/parallel/impl_par_methods.rs index 0aedee9e8..e5470e170 100644 --- a/src/parallel/impl_par_methods.rs +++ b/src/parallel/impl_par_methods.rs @@ -5,6 +5,8 @@ use crate::parallel::prelude::*; use crate::parallel::par::ParallelSplits; use super::send_producer::SendProducer; +use crate::partial::Partial; + /// # Parallel methods /// /// These methods require crate feature `rayon`. @@ -163,76 +165,3 @@ zip_impl! { [true P1 P2 P3 P4 P5], [false P1 P2 P3 P4 P5 P6], } - -/// Partial is a partially written contiguous slice of data; -/// it is the owner of the elements, but not the allocation, -/// and will drop the elements on drop. -#[must_use] -pub(crate) struct Partial { - /// Data pointer - ptr: *mut T, - /// Current length - len: usize, -} - -impl Partial { - /// Create an empty partial for this data pointer - /// - /// Safety: Unless ownership is released, the - /// Partial acts as an owner of the slice of data (not the allocation); - /// and will free the elements on drop; the pointer must be dereferenceable - /// and the `len` elements following it valid. - pub(crate) unsafe fn new(ptr: *mut T) -> Self { - Self { - ptr, - len: 0, - } - } - - pub(crate) fn stub() -> Self { - Self { len: 0, ptr: 0 as *mut _ } - } - - pub(crate) fn is_stub(&self) -> bool { - self.ptr.is_null() - } - - /// Release Partial's ownership of the written elements, and return the current length - pub(crate) fn release_ownership(mut self) -> usize { - let ret = self.len; - self.len = 0; - ret - } - - /// Merge if they are in order (left to right) and contiguous. - /// Skips merge if T does not need drop. - pub(crate) fn try_merge(mut left: Self, right: Self) -> Self { - if !std::mem::needs_drop::() { - return left; - } - // Merge the partial collect results; the final result will be a slice that - // covers the whole output. - if left.is_stub() { - right - } else if left.ptr.wrapping_add(left.len) == right.ptr { - left.len += right.release_ownership(); - left - } else { - // failure to merge; this is a bug in collect, so we will never reach this - debug_assert!(false, "Partial: failure to merge left and right parts"); - left - } - } -} - -unsafe impl Send for Partial where T: Send { } - -impl Drop for Partial { - fn drop(&mut self) { - if !self.ptr.is_null() { - unsafe { - std::ptr::drop_in_place(std::slice::from_raw_parts_mut(self.ptr, self.len)); - } - } - } -} diff --git a/src/partial.rs b/src/partial.rs new file mode 100644 index 000000000..887e93824 --- /dev/null +++ b/src/partial.rs @@ -0,0 +1,88 @@ +// 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::ptr; + +/// Partial is a partially written contiguous slice of data; +/// it is the owner of the elements, but not the allocation, +/// and will drop the elements on drop. +#[must_use] +pub(crate) struct Partial { + /// Data pointer + ptr: *mut T, + /// Current length + pub(crate) len: usize, +} + +impl Partial { + /// Create an empty partial for this data pointer + /// + /// ## Safety + /// + /// Unless ownership is released, the Partial acts as an owner of the slice of data (not the + /// allocation); and will free the elements on drop; the pointer must be dereferenceable and + /// the `len` elements following it valid. + /// + /// The Partial has an accessible length field which must only be modified in trusted code. + pub(crate) unsafe fn new(ptr: *mut T) -> Self { + Self { + ptr, + len: 0, + } + } + + #[cfg(feature = "rayon")] + pub(crate) fn stub() -> Self { + Self { len: 0, ptr: 0 as *mut _ } + } + + #[cfg(feature = "rayon")] + pub(crate) fn is_stub(&self) -> bool { + self.ptr.is_null() + } + + /// Release Partial's ownership of the written elements, and return the current length + pub(crate) fn release_ownership(mut self) -> usize { + let ret = self.len; + self.len = 0; + ret + } + + #[cfg(feature = "rayon")] + /// Merge if they are in order (left to right) and contiguous. + /// Skips merge if T does not need drop. + pub(crate) fn try_merge(mut left: Self, right: Self) -> Self { + if !std::mem::needs_drop::() { + return left; + } + // Merge the partial collect results; the final result will be a slice that + // covers the whole output. + if left.is_stub() { + right + } else if left.ptr.wrapping_add(left.len) == right.ptr { + left.len += right.release_ownership(); + left + } else { + // failure to merge; this is a bug in collect, so we will never reach this + debug_assert!(false, "Partial: failure to merge left and right parts"); + left + } + } +} + +unsafe impl Send for Partial where T: Send { } + +impl Drop for Partial { + fn drop(&mut self) { + if !self.ptr.is_null() { + unsafe { + ptr::drop_in_place(std::slice::from_raw_parts_mut(self.ptr, self.len)); + } + } + } +} diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 57c0e2dc5..f894390e6 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -8,7 +8,6 @@ #[macro_use] mod zipmacro; -mod partial_array; use std::mem::MaybeUninit; @@ -17,13 +16,12 @@ use crate::AssignElem; use crate::IntoDimension; use crate::Layout; use crate::NdIndex; +use crate::partial::Partial; use crate::indexes::{indices, Indices}; use crate::layout::{CORDER, FORDER}; use crate::split_at::{SplitPreference, SplitAt}; -use partial_array::PartialArray; - /// Return if the expression is a break value. macro_rules! fold_while { ($e:expr) => { @@ -1070,7 +1068,7 @@ macro_rules! map_impl { /// inputs. /// /// If all inputs are c- or f-order respectively, that is preserved in the output. - pub fn apply_collect(self, f: impl FnMut($($p::Item,)* ) -> R) -> Array + pub fn apply_collect(self, mut f: impl FnMut($($p::Item,)* ) -> R) -> Array { // Make uninit result let mut output = self.uninitalized_for_current_layout::(); @@ -1078,13 +1076,20 @@ macro_rules! map_impl { // For elements with no drop glue, just overwrite into the array self.apply_assign_into(&mut output, f); } else { - // For generic elements, use a proxy that counts the number of filled elements, + // For generic elements, use a Partial to counts the number of filled elements, // and can drop the right number of elements on unwinding unsafe { - PartialArray::scope(output.view_mut(), move |partial| { - debug_assert_eq!(partial.layout().tendency() >= 0, self.layout_tendency >= 0); - self.apply_assign_into(partial, f); + let mut output = output.raw_view_mut().cast::(); + let mut partial = Partial::new(output.as_mut_ptr()); + let partial_ref = &mut partial; + debug_assert!(output.is_contiguous()); + debug_assert_eq!(output.layout().tendency() >= 0, self.layout_tendency >= 0); + self.and(output) + .apply(move |$($p, )* output_: *mut R| { + output_.write(f($($p ),*)); + partial_ref.len += 1; }); + partial.release_ownership(); } } diff --git a/src/zip/partial_array.rs b/src/zip/partial_array.rs deleted file mode 100644 index d6a2bb9cd..000000000 --- a/src/zip/partial_array.rs +++ /dev/null @@ -1,144 +0,0 @@ -// 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 crate::imp_prelude::*; -use crate::{ - AssignElem, - Layout, - NdProducer, - Zip, - FoldWhile, -}; - -use std::cell::Cell; -use std::mem; -use std::mem::MaybeUninit; -use std::ptr; - -/// An assignable element reference that increments a counter when assigned -pub(crate) struct ProxyElem<'a, 'b, A> { - item: &'a mut MaybeUninit, - filled: &'b Cell -} - -impl<'a, 'b, A> AssignElem for ProxyElem<'a, 'b, A> { - fn assign_elem(self, item: A) { - self.filled.set(self.filled.get() + 1); - *self.item = MaybeUninit::new(item); - } -} - -/// Handles progress of assigning to a part of an array, for elements that need -/// to be dropped on unwinding. See Self::scope. -pub(crate) struct PartialArray<'a, 'b, A, D> - where D: Dimension -{ - data: ArrayViewMut<'a, MaybeUninit, D>, - filled: &'b Cell, -} - -impl<'a, 'b, A, D> PartialArray<'a, 'b, A, D> - where D: Dimension -{ - /// Create a temporary PartialArray that wraps the array view `data`; - /// if the end of the scope is reached, the partial array is marked complete; - /// if execution unwinds at any time before them, the elements written until then - /// are dropped. - /// - /// Safety: the caller *must* ensure that elements will be written in `data`'s preferred order. - /// PartialArray can not handle arbitrary writes, only in the memory order. - pub(crate) unsafe fn scope(data: ArrayViewMut<'a, MaybeUninit, D>, - scope_fn: impl FnOnce(&mut PartialArray)) - { - let filled = Cell::new(0); - let mut partial = PartialArray::new(data, &filled); - scope_fn(&mut partial); - filled.set(0); // mark complete - } - - unsafe fn new(data: ArrayViewMut<'a, MaybeUninit, D>, - filled: &'b Cell) -> Self - { - debug_assert_eq!(filled.get(), 0); - Self { data, filled } - } -} - -impl<'a, 'b, A, D> Drop for PartialArray<'a, 'b, A, D> - where D: Dimension -{ - fn drop(&mut self) { - if !mem::needs_drop::() { - return; - } - - let mut count = self.filled.get(); - if count == 0 { - return; - } - - Zip::from(self).fold_while((), move |(), elt| { - if count > 0 { - count -= 1; - unsafe { - ptr::drop_in_place::(elt.item.as_mut_ptr()); - } - FoldWhile::Continue(()) - } else { - FoldWhile::Done(()) - } - }); - } -} - -impl<'a: 'c, 'b: 'c, 'c, A, D: Dimension> NdProducer for &'c mut PartialArray<'a, 'b, A, D> { - // This just wraps ArrayViewMut as NdProducer and maps the item - type Item = ProxyElem<'a, 'b, A>; - type Dim = D; - type Ptr = *mut MaybeUninit; - type Stride = isize; - - private_impl! {} - fn raw_dim(&self) -> Self::Dim { - self.data.raw_dim() - } - - fn equal_dim(&self, dim: &Self::Dim) -> bool { - self.data.equal_dim(dim) - } - - fn as_ptr(&self) -> Self::Ptr { - NdProducer::as_ptr(&self.data) - } - - fn layout(&self) -> Layout { - self.data.layout() - } - - unsafe fn as_ref(&self, ptr: Self::Ptr) -> Self::Item { - ProxyElem { filled: self.filled, item: &mut *ptr } - } - - unsafe fn uget_ptr(&self, i: &Self::Dim) -> Self::Ptr { - self.data.uget_ptr(i) - } - - fn stride_of(&self, axis: Axis) -> Self::Stride { - self.data.stride_of(axis) - } - - #[inline(always)] - fn contiguous_stride(&self) -> Self::Stride { - self.data.contiguous_stride() - } - - fn split_at(self, _axis: Axis, _index: usize) -> (Self, Self) { - unimplemented!(); - } -} -