-
Notifications
You must be signed in to change notification settings - Fork 839
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
Changes from 4 commits
fa2ec44
44bafd3
0705d33
27c075d
29157db
ac804bc
3fc6fab
888cbe0
6007aed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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( | ||
|
@@ -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()); | ||
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), | ||
}) | ||
} | ||
|
||
|
@@ -802,6 +810,41 @@ impl ArrowArray { | |
pub fn into_raw(this: ArrowArray) -> (*const FFI_ArrowArray, *const FFI_ArrowSchema) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 fn export_into_raw( | ||
this: ArrowArray, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have this is 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved. Please take another look. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, I changed |
||
out_array: *mut FFI_ArrowArray, | ||
out_schema: *mut FFI_ArrowSchema, | ||
) { | ||
let array = Arc::into_raw(this.array); | ||
let schema = Arc::into_raw(this.schema); | ||
unsafe { | ||
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> { | ||
|
@@ -1164,4 +1207,36 @@ 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); | ||
|
||
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(()) | ||
} | ||
} |
There was a problem hiding this comment.
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
onFFI_ArrowArray
(which is empty), but doesn't free the memory pointed byarray
andschema
.For instance, if
array
andschema
are fromArc::into_raw
, then the memory allocated for theArc
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
andschema
(e.g., exported byArc::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
andFFI_ArrowSchema
, and set the content of the originalarray
andschema
to beFFI_ArrowArray::empty()
andFFI_ArrowSchema::empty()
so that the exporter can just safely free the memory later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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 forFFI_ArrowArray
. However this can only be done after the exported array is imported viaFFI_ArrowArray::try_from_raw
, which we don't know when.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 bystd::ptr::replace
here doesn't look safe to me. When the pointer is coming from anArc
that seems to violate rust's unique ownership rules.