Zero-copy Vec conversion (#3516) (#1176) #3756

tustvold · 2023-02-23T14:32:52Z

Which issue does this PR close?

Closes #3516
Relates to #1176

Rationale for this change

This starts the process of migrating away from custom mutable buffer abstractions, in favour of Vec. This will allow eventually deprecating and removing MutableBuffer and BufferBuilder, and eventually Buffer itself.

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-02-23T14:33:38Z

arrow-buffer/src/buffer/immutable.rs

@@ -69,6 +71,21 @@ impl Buffer {
        }
    }

+    /// Create a [`Buffer`] from the provided `Vec` without copying
+    #[inline]
+    pub fn from_vec<T: ArrowNativeType>(vec: Vec<T>) -> Self {


We can't implement From as this would conflict with the blanket From<AsRef<[u8]>>

tustvold · 2023-02-23T15:06:40Z

arrow-buffer/src/buffer/immutable.rs

@@ -632,4 +690,83 @@ mod tests {
        let buffer = Buffer::from(MutableBuffer::from_len_zeroed(12));
        buffer.slice_with_length(2, usize::MAX);
    }
+
+    #[test]
+    fn test_vec_interop() {


Adding Vec conversion at the Buffer level does allow for transmuting types provided they have the same layout, this is perhaps not ideal but isn't harmful.

Long-term I hope to deprecate and remove Buffer and MutableBuffer, just leaving ScalarBuffer which statically prevents this sort of type-conversion, but doing it this way allows for a gradual migration.

this is perhaps not ideal but isn't harmful.

Is it a problem for something like converting integers to (invalid) floats 🤔

Nope, there is a good justification for why this is perfectly fine here. The only place where this becomes problematic is where types have bit sequences that illegal, e.g. NonZeroU32 or bool, all ArrowNativeType are valid for all bit sequences.

alamb · 2023-02-23T14:44:06Z

arrow-buffer/src/buffer/immutable.rs

@@ -269,6 +289,43 @@ impl Buffer {
                length,
            })
    }
+
+    /// Returns `Vec` for mutating the buffer if this buffer is not offset and was
+    /// allocated with the correct layout for `Vec<T>`


I think it would be good to explicitly say here an error is returned if the buffer can't be converted to a Vec (and ideally hint how to get a Vec out of it anyways (perhaps by copying)

alamb

I went through this code and the tests carefully and it looked good to me. I have a few minor doc and test suggestions but all in all 👍

Nice work @tustvold

alamb · 2023-02-27T13:36:26Z

arrow-buffer/src/alloc/mod.rs

-    /// See [allocate_aligned] and [free_aligned].
-    Arrow(usize),
+    /// An allocation using [`std::alloc`]
+    Standard(Layout),
    /// An allocation from an external source like the FFI interface or a Rust Vec.


I wonder if the comment for Custom should be updated to say like "external Rust Vec" or maybe remove the reference to Rust Vec entirely

alamb · 2023-02-27T13:47:15Z

arrow-buffer/src/buffer/immutable.rs

+    pub fn from_vec<T: ArrowNativeType>(vec: Vec<T>) -> Self {
+        // Safety
+        // Vec::as_ptr guaranteed to not be null and ArrowNativeType are trivially transmutable
+        let ptr = unsafe { NonNull::new_unchecked(vec.as_ptr() as _) };


It seems like into_raw_parts would be ideal here, but sadly it appears to not yet be stabalized

https://doc.rust-lang.org/beta/std/vec/struct.Vec.html#method.into_raw_parts

I note that the docs say

After calling this function, the caller is responsible for the memory previously managed by the Vec. The only way to do this is to convert the raw pointer, length, and capacity back into a Vec with the from_raw_parts function, allowing the destructor to perform the cleanup.

However, this code uses Deallocation::Standard(layout) to deallocate memory, which seems ok, I just wanted to point out it doesn't match the docs (though maybe this is fine)

The docs for dealloc are authoritative here

ptr must denote a block of memory currently allocated via this allocator
layout must be the same layout that was used to allocate that block of memory

The layout of arrays, vecs, etc... is fixed and defined by Layout::array. As such we are fine provided we obey the same logic.

Do we plan to store/create from layouts other than Vec? Otherwise we can just create from Vec, forget the owned Vector.

On drop we recreate the Vec so that the drop sequence is executed.

This would defer all this logic to the implementation in std.

Do we plan to store/create from layouts other than Vec? Otherwise we can just create from Vec, forget the owned Vector.

Eventually we may deprecate and remove support for other layouts, but at least for a period we need to support aligned layouts such as those created by MutableBuffer so that we can avoid stop-the-world changes

Right, maybe we could add a comment that once we remove that support we could remove the unsafe layout related code and do something similar to what we do here: https://github.com/DataEngineeringLabs/foreign_vec/blob/0d38968facee8a81748ec380fad78379d806fe1d/src/lib.rs#L25

unsafe layout related code and do something similar to what we do here

What would be the advantage of this? Does polars depend on the foreign vec interface? It doesn't seem to be doing anything materially different?

Edit: In fact I'm fairly sure this line is technically unsound - https://github.com/DataEngineeringLabs/foreign_vec/blob/0d38968facee8a81748ec380fad78379d806fe1d/src/lib.rs#L49, there are a lot of safety constraints on from_raw_parts that appear to be not being checked or documented?

alamb · 2023-02-27T13:49:40Z

arrow-buffer/src/buffer/immutable.rs

+        let len = vec.len() * std::mem::size_of::<T>();
+        // Safety
+        // Layout guaranteed to be valid
+        let layout = unsafe { Layout::array::<T>(vec.capacity()).unwrap_unchecked() };


Is there a reason to use unwrap_unchecked here? Maybe it would be better to panic if the layout was messed up somehow?

Looks like it errors on overflow https://doc.rust-lang.org/beta/std/alloc/struct.Layout.html#method.array

Like maybe the size overflows because someone passed in a giant Vec or something?

🤔

Is there a reason to use unwrap_unchecked here

This is a clone of Vec::current_memory, I will add a link.

The TLDR is that if the Vec is valid, it must have a valid layout (otherwise among other issues Vec wouldn't be able to deallocate itself).

alamb · 2023-02-27T13:51:13Z

arrow-buffer/src/buffer/immutable.rs

@@ -632,4 +690,83 @@ mod tests {
        let buffer = Buffer::from(MutableBuffer::from_len_zeroed(12));
        buffer.slice_with_length(2, usize::MAX);
    }
+
+    #[test]
+    fn test_vec_interop() {


this is perhaps not ideal but isn't harmful.

Is it a problem for something like converting integers to (invalid) floats 🤔

alamb · 2023-02-27T13:53:30Z

arrow-buffer/src/buffer/immutable.rs

+        let mut b: Vec<i128> = Vec::with_capacity(4);
+        b.extend_from_slice(&[1, 2, 3]);
+        let b = Buffer::from_vec(b);
+        let back = b.into_vec::<i256>().unwrap();


I think it is also worth a test when the capacity doesn't equally divide equally into the transmuted size -- like maybe change this test to have capacity 5 with i128 and verify the i256 output Vec only has capacity 2

A capacity of 5 is tested above and results in an error, as the layout of the underlying allocation is invalid

alamb · 2023-02-27T13:56:07Z

arrow-buffer/src/buffer/immutable.rs

+        let slice = b.slice_with_length(0, 34);
+        drop(b);
+
+        let back = slice.into_vec::<i128>().unwrap();


Is slicing with a non zero offset also covered? Maybe slice(2) below covers that 🤔

Yeah slice(2) covers this

alamb · 2023-02-27T13:56:41Z

arrow-buffer/src/buffer/mutable.rs

@@ -95,12 +102,14 @@ impl MutableBuffer {

    /// Allocates a new [MutableBuffer] from given `Bytes`.


maybe it is worth updating these docs to explain when an Err is returned

alamb · 2023-02-27T13:57:11Z

arrow-buffer/src/buffer/scalar.rs

@@ -90,6 +90,15 @@ impl<T: ArrowNativeType> From<Buffer> for ScalarBuffer<T> {
    }
 }

+impl<T: ArrowNativeType> From<Vec<T>> for ScalarBuffer<T> {


alamb · 2023-02-27T13:58:27Z

It might be worth it to have @viirya give this PR a once over before merge.

Not sure if @ritchie46 or @jorgecarleitao are interested or want to comment either.

tustvold · 2023-02-27T16:12:09Z

Integration failure is unrelated - apache/arrow#34367

tustvold · 2023-02-28T21:00:33Z

Unless I hear anything further, I plan to merge this tomorrow morning. Please let me know if you need more time to review

ursabot · 2023-03-01T10:12:33Z

Benchmark runs are scheduled for baseline = 5edc954 and contender = d440c24. d440c24 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Zero-copy Vec conversion (apache#3516) (apache#1176)

4cbad4d

github-actions bot added the arrow Changes to the arrow crate label Feb 23, 2023

tustvold commented Feb 23, 2023

View reviewed changes

Fix doc

d84e499

tustvold commented Feb 23, 2023

View reviewed changes

tustvold mentioned this pull request Feb 23, 2023

Discussion: relationship / unification of arrow-rs and arrow2 going forward #1176

Closed

tustvold added 2 commits February 23, 2023 15:27

More tests

a4f1435

Merge remote-tracking branch 'upstream/master' into zero-copy-vec

0abef5f

alamb reviewed Feb 24, 2023

View reviewed changes

alamb approved these changes Feb 27, 2023

View reviewed changes

tustvold added 2 commits February 27, 2023 14:33

Review feedback

f07f396

More tests

4a882f5

tustvold merged commit d440c24 into apache:master Mar 1, 2023

tustvold mentioned this pull request Mar 10, 2023

Support Zero-Copy Conversion from Vec to/from MutableBuffer #3516

Closed

tustvold mentioned this pull request Mar 23, 2023

Add Zero-Copy Conversion between Vec and MutableBuffer #3920

Merged

tustvold mentioned this pull request Dec 6, 2023

Replace ScalarBuffer in Parquet with Vec (#1849) (#5177) #5178

Merged

mbrobbel mentioned this pull request Apr 18, 2024

Use ScalarBuffer<u8> instead of Buffer for FixedSizeBinaryArray #5667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zero-copy Vec conversion (#3516) (#1176) #3756

Zero-copy Vec conversion (#3516) (#1176) #3756

tustvold commented Feb 23, 2023 •

edited

Loading

tustvold Feb 23, 2023

tustvold Feb 23, 2023 •

edited

Loading

alamb Feb 27, 2023

tustvold Feb 27, 2023

alamb Feb 23, 2023

alamb left a comment

alamb Feb 27, 2023

alamb Feb 27, 2023

tustvold Feb 27, 2023

ritchie46 Feb 27, 2023

tustvold Feb 27, 2023 •

edited

Loading

ritchie46 Feb 27, 2023

tustvold Feb 27, 2023 •

edited

Loading

alamb Feb 27, 2023

tustvold Feb 27, 2023 •

edited

Loading

alamb Feb 27, 2023

alamb Feb 27, 2023

tustvold Feb 27, 2023 •

edited

Loading

alamb Feb 27, 2023

tustvold Feb 27, 2023

alamb Feb 27, 2023

alamb Feb 27, 2023

alamb commented Feb 27, 2023

tustvold commented Feb 27, 2023

tustvold commented Feb 28, 2023

ursabot commented Mar 1, 2023

		@@ -95,12 +102,14 @@ impl MutableBuffer {

		/// Allocates a new [MutableBuffer] from given `Bytes`.

Zero-copy Vec conversion (#3516) (#1176) #3756

Zero-copy Vec conversion (#3516) (#1176) #3756

Conversation

tustvold commented Feb 23, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

tustvold Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 27, 2023

tustvold commented Feb 27, 2023

tustvold commented Feb 28, 2023

ursabot commented Mar 1, 2023

tustvold commented Feb 23, 2023 •

edited

Loading

tustvold Feb 23, 2023 •

edited

Loading

tustvold Feb 27, 2023 •

edited

Loading

tustvold Feb 27, 2023 •

edited

Loading

tustvold Feb 27, 2023 •

edited

Loading

tustvold Feb 27, 2023 •

edited

Loading