Move FFI to sub-crates #3687

tustvold · 2023-02-10T10:52:03Z

Which issue does this PR close?

As part of pola-rs/polars#6735 I would like to use the FFI interfaces without needing to bring in the top-level crate, this PR therefore moves FFI into arrow-schema and arrow-data.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-02-10T10:53:30Z

arrow-data/src/ffi.rs

+    /// Returns the buffer at the provided index
+    ///
+    /// # Panic
+    /// Panics if index exceeds the number of buffers or the buffer is not correctly aligned
+    pub fn buffer(&self, index: usize) -> *const u8 {
+        assert!(!self.buffers.is_null());
+        assert!(index < self.num_buffers());
+        // SAFETY:
+        // If buffers is not null must be valid for reads up to num_buffers
+        unsafe { std::ptr::read_unaligned((self.buffers as *mut *const u8).add(index)) }
+    }
+
+    /// Returns the number of buffers
+    pub fn num_buffers(&self) -> usize {
+        self.n_buffers as _
+    }
+
+    /// Returns the child at the provided index
+    pub fn child(&self, index: usize) -> &FFI_ArrowArray {
+        assert!(!self.children.is_null());
+        assert!(index < self.num_children());
+        // Safety:
+        // If children is not null must be valid for reads up to num_children
+        unsafe {
+            let child = std::ptr::read_unaligned(self.children.add(index));
+            child.as_ref().unwrap()
+        }
+    }
+
+    /// Returns the number of children
+    pub fn num_children(&self) -> usize {
+        self.n_children as _
+    }
+
+    /// Returns the dictionary if any
+    pub fn dictionary(&self) -> Option<&Self> {
+        // Safety:
+        // If dictionary is not null should be valid for reads of `Self`
+        unsafe { self.dictionary.as_ref() }
+    }
+}


These methods are new, but just inline what was in arrow::ffi to avoid needing access to private members

tustvold · 2023-02-10T10:54:15Z

arrow-data/src/ffi.rs

+        // Safety:
+        // If children is not null must be valid for reads up to num_children
+        unsafe {
+            let child = std::ptr::read_unaligned(self.children.add(index));


Previously the implementation did a blind dereference here, I couldn't find anything to suggest alignment was actually guaranteed, so changed to use read_unaligned. On x86 there is no difference

Can you explain the rationale for this change?

https://doc.rust-lang.org/std/ptr/fn.read_unaligned.html

Is it because the data (may) come from an unaligned location?

Is it because the data (may) come from an unaligned location?

Yes, the C Data interface does not specify what alignment the various pointers may or may not have

Makes sense.

Ideally I cannot think that how/why the pointers come from unaligned location, but assuming it could be seems safer.

tustvold · 2023-02-10T10:55:15Z

arrow/src/ffi_stream.rs

-                schema.release = None;
+            Ok(schema) => {
+                unsafe { std::ptr::copy(addr_of!(schema), out, 1) };
+                std::mem::forget(schema);


This is a "nicer" way to implement the move, that avoids needing access to the private internals

By forgeting it, will some non-pointer fields like flags and n_children be released? Supposed that it should be but wanted to verify it.

flags and n_children aren't separate allocations, they are all stored inline with the rest of the struct members on the stack allocated schema. So no they aren't freed, but they also don't need to be

Hmm, I suppose that you meant they are deallocated from stack?

arrow/src/ffi.rs

tustvold · 2023-02-10T11:05:30Z

arrow/src/ffi.rs

        .map(|ptr| Buffer::from_custom_allocation(ptr, len, owner))
 }

-fn create_child(


This method was previously incorrect as it coerced non-static borrows to static borrows. Fortunately it can be removed without any change to the public interface

tustvold · 2023-02-10T11:05:53Z

arrow/src/ffi.rs

@@ -827,7 +423,7 @@ pub struct ArrowArray {
 pub struct ArrowArrayChild<'a> {
    array: &'a FFI_ArrowArray,
    schema: &'a FFI_ArrowSchema,
-    owner: Arc<FFI_ArrowArray>,
+    owner: &'a Arc<FFI_ArrowArray>,


This just avoids an unnecessary clone

tustvold · 2023-02-10T11:06:03Z

arrow/src/ffi.rs

@@ -1526,24 +1108,6 @@ mod tests {
        Ok(())
    }

-    #[test]
-    fn null_array_n_buffers() -> Result<()> {


Moved to arrow-data

alamb

can we also add arrow-ffi to the crate topology here https://docs.rs/arrow/32.0.0/arrow/ ?

I think the movement parts of the PR make sense to me,

I am not as confident about the other changes (like read_unaligned and forget) and would like someone else to review them too prior to merge

Another alternative would be split the PR into 2 pices: 1 "code movement into crates" and 2 "change in code" parts

alamb · 2023-02-10T11:38:42Z

arrow-data/src/ffi.rs

+        // Safety:
+        // If children is not null must be valid for reads up to num_children
+        unsafe {
+            let child = std::ptr::read_unaligned(self.children.add(index));


Can you explain the rationale for this change?

https://doc.rust-lang.org/std/ptr/fn.read_unaligned.html

Is it because the data (may) come from an unaligned location?

alamb · 2023-02-10T11:39:42Z

arrow/src/ffi_stream.rs

-                schema.release = None;
+            Ok(schema) => {
+                unsafe { std::ptr::copy(addr_of!(schema), out, 1) };
+                std::mem::forget(schema);


tustvold · 2023-02-10T11:44:17Z

can we also add arrow-ffi to the crate topology here

It doesn't add a new crate, just moves the ffi logic into the existing sub-crates

Another alternative would be split the PR into 2 pices: 1 "code movement into crates" and 2 "change in code" parts

These were workarounds to avoid making mutation methods public, which would be problematic on many levels 😄

alamb

Makes sense to me (✅ ) thanks

ursabot · 2023-02-10T15:22:15Z

Benchmark runs are scheduled for baseline = 560ebaa and contender = 3761ac5. 3761ac5 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

Move FFI to sub-crates

f14c800

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

tustvold commented Feb 10, 2023

View reviewed changes

tustvold added 2 commits February 10, 2023 10:58

Use ptr::write instead of drop_in_place

2774d16

Add inline

2c9acec

tustvold commented Feb 10, 2023

View reviewed changes

arrow/src/ffi.rs Outdated Show resolved Hide resolved

tustvold added 2 commits February 10, 2023 11:04

Avoid unnecessary clone

120e12b

Format

b51ce83

tustvold commented Feb 10, 2023

View reviewed changes

Clippy

1122caf

tustvold force-pushed the move-ffi-sub-crates branch from cbd1c03 to 1122caf Compare February 10, 2023 11:34

alamb approved these changes Feb 10, 2023

View reviewed changes

Remove pub(crate)

c32b0a2

tustvold force-pushed the move-ffi-sub-crates branch from 17e8299 to c32b0a2 Compare February 10, 2023 11:51

alamb approved these changes Feb 10, 2023

View reviewed changes

tustvold merged commit 3761ac5 into apache:master Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move FFI to sub-crates #3687

Move FFI to sub-crates #3687

tustvold commented Feb 10, 2023

tustvold Feb 10, 2023

tustvold Feb 10, 2023

alamb Feb 10, 2023

tustvold Feb 10, 2023

viirya Feb 10, 2023

viirya Feb 10, 2023

tustvold Feb 10, 2023 •

edited

Loading

alamb Feb 10, 2023

viirya Feb 10, 2023 •

edited

Loading

tustvold Feb 10, 2023

viirya Feb 10, 2023

tustvold Feb 10, 2023

tustvold Feb 10, 2023

tustvold Feb 10, 2023

alamb left a comment

alamb Feb 10, 2023

alamb Feb 10, 2023

tustvold commented Feb 10, 2023 •

edited

Loading

alamb left a comment

ursabot commented Feb 10, 2023

Move FFI to sub-crates #3687

Move FFI to sub-crates #3687

Conversation

tustvold commented Feb 10, 2023

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

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 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Feb 10, 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

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

tustvold commented Feb 10, 2023 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

ursabot commented Feb 10, 2023

tustvold Feb 10, 2023 •

edited

Loading

viirya Feb 10, 2023 •

edited

Loading

tustvold commented Feb 10, 2023 •

edited

Loading