Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Clone and copy source structs internally #1449

Merged
merged 9 commits into from
Mar 19, 2022
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 77 additions & 6 deletions arrow/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ bitflags! {
/// See <https://arrow.apache.org/docs/format/CDataInterface.html#structure-definitions>
/// This was created by bindgen
#[repr(C)]
#[derive(Debug, Clone)]
#[derive(Debug)]
pub struct FFI_ArrowSchema {
format: *const c_char,
name: *const c_char,
Expand Down Expand Up @@ -336,7 +336,7 @@ fn bit_width(data_type: &DataType, i: usize) -> Result<usize> {
/// See <https://arrow.apache.org/docs/format/CDataInterface.html#structure-definitions>
/// This was created by bindgen
#[repr(C)]
#[derive(Debug, Clone)]
#[derive(Debug)]
pub struct FFI_ArrowArray {
pub(crate) length: i64,
pub(crate) null_count: i64,
Expand Down Expand Up @@ -769,6 +769,9 @@ impl ArrowArray {
/// creates a new [ArrowArray] from two pointers. Used to import from the C Data Interface.
/// # Safety
/// See safety of [ArrowArray]
/// Note that this function will copy the content pointed by the raw pointers. Considering
/// the raw pointers can be from `Arc::into_raw` or other raw pointers, users must be responsible
/// on managing the allocation of the structs by themselves.
/// # Error
/// Errors if any of the pointers is null
pub unsafe fn try_from_raw(
Expand All @@ -781,11 +784,16 @@ impl ArrowArray {
.to_string(),
));
};
let ffi_array = (*array).clone();
let ffi_schema = (*schema).clone();

let array_mut = array as *mut FFI_ArrowArray;
let schema_mut = schema as *mut FFI_ArrowSchema;

let array_data = std::ptr::replace(array_mut, FFI_ArrowArray::empty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually after thinking more on this, it seems this won't address the original problem neither. It basically just calls drop on FFI_ArrowArray (which is empty), but doesn't free the memory pointed by array and schema.

  +-------+
  | array |
  +-------+             +----------------------------+
     |                  |                            |
     +----------------->|      FFI_ArrowArray        | <- memory leaked
                        |                            |
                        +----------------------------+

For instance, if array and schema are from Arc::into_raw, then the memory allocated for the Arc will become dangling after this, and thus memory leak.

I'm thinking whether we'll need two APIs, one where we are able to take the ownership of the memory allocated for the array and schema (e.g., exported by Arc::into_raw from Rust itself), and one where we cannot take the ownership (e.g., memory was allocated by other languages such as Java), and thus requires the exporter to free the memory by itself later.

For the latter, we can clone the content for FFI_ArrowArray and FFI_ArrowSchema, and set the content of the original array and schema to be FFI_ArrowArray::empty() and FFI_ArrowSchema::empty() so that the exporter can just safely free the memory later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, if array and schema are from Arc::into_raw, then the memory allocated for the Arc will become dangling after this, and thus memory leak.

Currently if user try to export using into_raw and and don't import using from_raw (we can assume it's a normal case? as they export data to be used by others they don't need to import again), they might have memory leak.

After check the CPP-import implementation, I think this change is fine. We even can remove the two drop_in_place call as it seems unnecessary.

What we need is to redesign the ArrowArray::into_raw(), we can't use Arc::into_raw in the implementation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea the drop_in_place here seems unnecessary.

I'm not sure if it's possible to redesign ArrowArray::into_raw though, since after exporting the array, we need to free up the memory allocated for FFI_ArrowArray. However this can only be done after the exported array is imported via FFI_ArrowArray::try_from_raw, which we don't know when.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct that with one single API, we cannot deal with both cases: raw pointers from Arc and not from Arc.

I'm not sure two separate APIs is good. With a single API, we can ask users to take care of releasing the raw pointers (either Arc or not) by themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However this can only be done after the exported array is imported via FFI_ArrowArray::try_from_raw, which we don't know when.

  1. It may not be imported in rust via FFI_ArrowArray::try_from_raw, it can be imported by other language sdk
  2. We don't need to know, the user should import it somewhere or free them if needed, that's why we can't use Arc::into_raw because we don't know how user might use them. This API should be fired and done, shouldn't expect user always do something like try_from_raw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another one allowing importer to allocate memory for exporter

If Rust side is importer, we already have it as we can do it now by creating empty structs, passing raw pointers to exporter.

If Java side is importer, we may need an export API which takes raw pointers from Java and replaces its content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. Do you plan to add the export API in this PR, or separately?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be straightforward to add, let me add it here. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new API export_into_raw. Please check it. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to catch up with these discussions since I'll soon have a need to create Buffers from foreign memory. The cast to *mut followed by std::ptr::replace here doesn't look safe to me. When the pointer is coming from an Arc that seems to violate rust's unique ownership rules.

let schema_data = std::ptr::replace(schema_mut, FFI_ArrowSchema::empty());

Ok(Self {
array: Arc::new(ffi_array),
schema: Arc::new(ffi_schema),
array: Arc::new(array_data),
schema: Arc::new(schema_data),
})
}

Expand All @@ -802,6 +810,35 @@ impl ArrowArray {
pub fn into_raw(this: ArrowArray) -> (*const FFI_ArrowArray, *const FFI_ArrowSchema) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we delete this API, as this no longer a pair with the try_from_raw() method.

If we leave it here, the only way to avoid memory leak is user use Arc::from_raw() 2 times by themselves

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend not to delete it as its usage is different. We actually use this API internally and we manage the Arc pointers manually. This save us a round trip from Java.

(Arc::into_raw(this.array), Arc::into_raw(this.schema))
}

/// exports [ArrowArray] to raw pointers of the C Data Interface provided by the consumer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "Exports"

/// # Safety
/// See safety of [ArrowArray]
/// This function copies the content of two FFI structs [FFI_ArrowArray] and [FFI_ArrowSchema] in
/// this [ArrowArray] to the location pointed by the raw pointers. Usually the raw pointers are
/// provided by the array data consumer.
pub unsafe fn export_into_raw(
this: ArrowArray,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have this is src/array/array.rs and have it to be a top-level function, paired to make_array_from_raw?

The signature can be something:

pub unsafe fn export_array_into_raw(
  src: &ArrayRef,
  out_array: *mut FFI_ArrowArray,
  out_schema: *mut FFI_ArrowSchema)

It's better to also update the usage doc at the top of this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved. Please take another look. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I changed src: &ArrayRef to src: ArrayRef. It is already a Arc, seems not necessary to take borrowed type on it.

out_array: *mut FFI_ArrowArray,
out_schema: *mut FFI_ArrowSchema,
) {
let array = Arc::into_raw(this.array);
let schema = Arc::into_raw(this.schema);
std::ptr::copy_nonoverlapping(array, out_array, 1);
std::ptr::copy_nonoverlapping(schema, out_schema, 1);

// Clean up the structs to avoid double-dropping
let empty_array = Box::into_raw(Box::new(FFI_ArrowArray::empty()));
let empty_schema = Box::into_raw(Box::new(FFI_ArrowSchema::empty()));
std::ptr::copy_nonoverlapping(empty_array, array as *mut FFI_ArrowArray, 1);
std::ptr::copy_nonoverlapping(empty_schema, schema as *mut FFI_ArrowSchema, 1);

// Drop Box and Arc pointers
Box::from_raw(empty_array);
Box::from_raw(empty_schema);
Arc::from_raw(array);
Arc::from_raw(schema);
}
}

impl<'a> ArrowArrayChild<'a> {
Expand Down Expand Up @@ -1164,4 +1201,38 @@ mod tests {
// (drop/release)
Ok(())
}

#[test]
fn test_export_into_raw() -> Result<()> {
let array = Int32Array::from(vec![1, 2, 3]);
let arrow_array = ArrowArray::try_from(array.data().clone())?;

// Assume two raw pointers provided by the consumer
let out_array = Box::new(FFI_ArrowArray::empty());
let out_schema = Box::new(FFI_ArrowSchema::empty());
let out_array_ptr = Box::into_raw(out_array);
let out_schema_ptr = Box::into_raw(out_schema);

unsafe {
ArrowArray::export_into_raw(arrow_array, out_array_ptr, out_schema_ptr);
}

// (simulate consumer) import it
unsafe {
let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
let data = ArrayData::try_from(array)?;
let array = make_array(data);

// perform some operation
let array = array.as_any().downcast_ref::<Int32Array>().unwrap();
let array = kernels::arithmetic::add(array, array).unwrap();

// verify
assert_eq!(array, Int32Array::from(vec![2, 4, 6]));

Box::from_raw(out_array_ptr);
Box::from_raw(out_schema_ptr);
}
Ok(())
}
}