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.

Sign up for GitHub

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

Remove Clone and copy source structs internally #1449

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fa2ec44
Remove Clone and copy source structs internally
viirya Mar 16, 2022
44bafd3
Remove drop_in_place and add more comment
viirya Mar 16, 2022
0705d33
Add export_into_raw
viirya Mar 18, 2022
27c075d
Fix format
viirya Mar 18, 2022
29157db
Fix clippy
viirya Mar 18, 2022
ac804bc
Move to export_array_into_raw
viirya Mar 18, 2022
3fc6fab
Fix clippy
viirya Mar 18, 2022
888cbe0
Fix doc
viirya Mar 18, 2022
6007aed
Use write_unaligned
viirya Mar 18, 2022
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
20 changes: 14 additions & 6 deletions arrow/src/ffi.rs
View file
Open in desktop
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 @@ -781,11 +781,19 @@ 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());

std::ptr::drop_in_place(array_mut);
std::ptr::drop_in_place(schema_mut);

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

Expand Down