Add try_unary_mut#3134
Conversation
viirya
force-pushed
the
add_scalar_mut
branch
from
0d17644 to
4261762
Compare
viirya
force-pushed
the
add_scalar_mut
branch
from
4261762 to
c98d21a
Compare
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
Thank you for working on this, I wonder if you have some benchmarks of how this compares to the allocating version, ideally using a modern allocator like jemalloc.
| pub fn as_slice(&mut self) -> (&mut [T::Native], Option<&[u8]>) { | ||
| ( | ||
| self.values_builder.as_slice_mut(), | ||
| self.null_buffer_builder.as_slice(), | ||
| ) |
There was a problem hiding this comment.
| pub fn as_slice(&mut self) -> (&mut [T::Native], Option<&[u8]>) { | |
| ( | |
| self.values_builder.as_slice_mut(), | |
| self.null_buffer_builder.as_slice(), | |
| ) | |
| pub fn slices_mut(&mut self) -> (&mut [T::Native], Option<&mut [u8]>) { | |
| ( | |
| self.values_builder.as_slice_mut(), | |
| self.null_buffer_builder.as_slice_mut(), | |
| ) |
Or something, it seems a bit unusual for both to not be mutable.
We could then add validity_slice and validity_slice_mut for completeness
There was a problem hiding this comment.
Okay, it is because for this I don't need null buffer slice to be mutable. But yes, it looks a bit strange to have one mutable with one non-mutable. 😄
| pub fn try_unary_mut<F, E>( | ||
| self, | ||
| op: F, | ||
| ) -> Result<PrimitiveArray<T>, Result<PrimitiveArray<T>, E>> |
There was a problem hiding this comment.
| ) -> Result<PrimitiveArray<T>, Result<PrimitiveArray<T>, E>> | |
| ) -> Result<Result<PrimitiveArray<T>, E>, PrimitiveArray<T>> |
I think the reverse result order makes more sense, as it represents the order of fallibility. If we can't convert to a builder is the first error case, then within that we have the error case of ArrowError.
I think it will also make it easier to implement a fallback, e.g.
arr.try_unary_mut(&mut op).unwrap_or_else(|arr| arr.try_unary(&mut op))?
| } | ||
|
|
||
| #[inline] | ||
| pub fn as_slice(&self) -> Option<&[u8]> { |
There was a problem hiding this comment.
👍
Could also add an as_slice_mut for completeness
Not yet on benchmarking. I think it should be better and hard to think that it could be slower. I will revise this based on above comments and run a benchmark later. Thanks. |
Yeah, I'm just curious as it is a non-trivial additional complexity and so I'm curious what difference it makes 😅 |
|
Simply updated slice related functions. I will update the result type later or tomorrow. (occupied occasionally by other matters in recent days, will be a bit slow response) |
|
I ran a simple benchmark which calls let mut arr_a = create_primitive_array::<Float32Type>(5120000, 0.0);
for _ in 0..10 {
let arr_b = create_primitive_array::<Float32Type>(5120000, 0.0);
// arr_a = add(&arr_a, &arr_b).unwrap();
arr_a = add_mut(arr_a, &arr_b).unwrap().unwrap();
} |
|
This seems like quite a lot of additional complexity for only a 3% performance improvement. What do you think about just providing |
|
Yeah, thought about this option too. I think it makes sense. I can remove |
viirya
changed the title
viirya
force-pushed
the
add_scalar_mut
branch
from
f32042c to
9f07fa6
Compare
| } | ||
|
|
||
| /// Returns the current values buffer as a slice | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
I think this shouldn't be necessary, as they are public
|
Benchmark runs are scheduled for baseline = 6f41b95 and contender = 5d84746. 5d84746 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
3 participants
Which issue does this PR close?
Closes #3133.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?