Improve performance of "no-broadcasting-needed" scenario in &array +&array operation #965
Conversation
src/impl_ops.rs
Outdated
| @@ -179,7 +179,13 @@ where | |||
| { | |||
| type Output = Array<A, <D as DimMax<E>>::Output>; | |||
| fn $mth(self, rhs: &'a ArrayBase<S2, E>) -> Self::Output { | |||
| let (lhs, rhs) = self.broadcast_with(rhs).unwrap(); | |||
| let (lhs, rhs) = if self.ndim() == rhs.ndim() && self.shape() == rhs.shape() { | |||
| let lhs = self.to_dimensionality::<<D as DimMax<E>>::Output>().unwrap(); | |||
There was a problem hiding this comment.
These to_dimensionality calls seem redundant with the ones already in broadcast_with. Is there a reason this PR shouldn't just do one or the other, not both?
If it was just the calls in this method, here it looks like self.view().into_dimensionality::<...>() is enough, i.e. using the existing method.
There was a problem hiding this comment.
That's right. We should use view().into_dimensionality() here. It is faster in the benchmark test (though I don’t know why)
There was a problem hiding this comment.
Adding the same shape detection in impl_ops can avoid executing co_broadcast in broadcast_with. I think it is worth to reserve.
src/impl_methods.rs
Outdated
| /// Creat an array view from an array with the same shape, but different dimensionality | ||
| /// type. Return None if the numbers of axes mismatch. | ||
| #[inline] | ||
| pub(crate) fn to_dimensionality<D2>(&self) -> Option<ArrayView<'_, A, D2>> |
There was a problem hiding this comment.
If this method is added, it should be just next to (after) into_dimensionality in this file.
|
Is there a benchmark for this case, and does it show an improvement? Benchmarks for non-dyn dimensions would be the most interesting. 🙂 |
|
I did benchmark tests in two cases: 1.no need broadcasting 2. only one side need broadcasting. (Look bench1.rs) The result shows that view().into_dimensionality() is faster than to_dimensionality(). And judging whether broadcasting is not needed before broadcast_with can significantly increase the speed. |
|
Is the "origin" benchmark from before this PR, or with changes in this PR? It's mostly the changes from before this PR to after it that are interesting :) |
yes it is from before this PR |
src/impl_ops.rs
Outdated
| (lhs, rhs) | ||
| } else { | ||
| self.broadcast_with(rhs).unwrap() | ||
| }; | ||
| Zip::from(&lhs).and(&rhs).map_collect(clone_opf(A::$mth)) |
There was a problem hiding this comment.
| Zip::from(&lhs).and(&rhs).map_collect(clone_opf(A::$mth)) | |
| Zip::from(lhs).and(rhs).map_collect(clone_opf(A::$mth)) |
I think here we should just consume the views, saves more redundant view creations in Zip (won't be a very noticeable change, maybe the compiler can remove the difference).
There was a problem hiding this comment.
OK. It has been correct.
|
Thanks! |
This PR does the following:
to_dimensionalitywhich creats an array view from an array with the same shape, but different dimensionality type.to_dimensionalityto avoid unnecessary calls ofbroadcastin &array + &array operation.Updates #936