[GitHub] [arrow] jhorstmann commented on a change in pull request #8796: [Rust] [Experiment] Vec vs current allocations

[GitBox <gi...@apache.org>]

jhorstmann commented on a change in pull request #8796:
URL: https://github.com/apache/arrow/pull/8796#discussion_r546362442



##########
File path: rust/arrow/src/buffer.rs
##########
@@ -208,15 +158,8 @@ impl Buffer {
 /// allocated memory region.
 impl<T: AsRef<[u8]>> From<T> for Buffer {
     fn from(p: T) -> Self {
-        // allocate aligned memory buffer
-        let slice = p.as_ref();
-        let len = slice.len() * mem::size_of::<u8>();
-        let capacity = bit_util::round_upto_multiple_of_64(len);
-        let buffer = memory::allocate_aligned(capacity);
-        unsafe {
-            memory::memcpy(buffer, slice.as_ptr(), len);
-            Buffer::build_with_arguments(buffer, len, Deallocation::Native(capacity))
-        }
+        let bytes = unsafe { Bytes::new(p.as_ref().to_vec(), Deallocation::Native) };

Review comment:
       There could be potential for further optimization here: `to_vec` has to copy the slice contents, a separate implementation of `From<Vec<u8>>` or `From<Vec<ArrowPrimitiveType>>` could avoid that copy and speed up several kernels involving primitives or list offsets.
   
   As a `From` implementation that would give a "conflicting implementations" error, an explicit `from_vec` method could work. I'd suggest trying it in a separate PR as it could change a bunch of code not directly related to the refactoring in this PR.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org