Remove Clone and copy source structs internally #1449

viirya · 2022-03-16T03:09:26Z

Which issue does this PR close?

Closes #1425.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

sunchao · 2022-03-16T03:57:47Z

arrow/src/ffi.rs

+        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());


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.

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

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.

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.

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

It may not be imported in rust via FFI_ArrowArray::try_from_raw, it can be imported by other language sdk

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

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.

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

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

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

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.

codecov-commenter · 2022-03-16T06:26:55Z

Codecov Report

Merging #1449 (7863a6c) into master (717216f) will increase coverage by 0.03%.
The diff coverage is 86.33%.

❗ Current head 7863a6c differs from pull request most recent head 888cbe0. Consider uploading reports for the commit 888cbe0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1449      +/-   ##
==========================================
+ Coverage   82.67%   82.71%   +0.03%     
==========================================
  Files         185      187       +2     
  Lines       53866    54175     +309     
==========================================
+ Hits        44535    44811     +276     
- Misses       9331     9364      +33

Impacted Files	Coverage Δ
arrow/src/array/array_list.rs	`95.52% <ø> (ø)`
arrow/src/array/mod.rs	`100.00% <ø> (ø)`
integration-testing/src/lib.rs	`0.00% <0.00%> (ø)`
arrow/src/array/data.rs	`82.95% <58.33%> (-0.21%)`	⬇️
arrow/src/array/transform/fixed_size_list.rs	`73.68% <73.68%> (ø)`
arrow/src/array/transform/union.rs	`75.00% <75.00%> (ø)`
arrow/src/array/transform/mod.rs	`86.46% <84.21%> (+0.03%)`	⬆️
arrow/src/array/array.rs	`86.92% <100.00%> (+1.51%)`	⬆️
arrow/src/compute/kernels/cast.rs	`95.59% <100.00%> (+0.03%)`	⬆️
arrow/src/compute/kernels/filter.rs	`88.00% <100.00%> (+2.59%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 717216f...888cbe0. Read the comment docs.

wangfenjin · 2022-03-18T10:32:32Z

arrow/src/ffi.rs

@@ -802,6 +810,41 @@ impl ArrowArray {
    pub fn into_raw(this: ArrowArray) -> (*const FFI_ArrowArray, *const FFI_ArrowSchema) {


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

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.

wangfenjin · 2022-03-18T10:34:25Z

Good work, seems this is the minimum change we can make this right

viirya · 2022-03-18T17:21:35Z

@sunchao @alamb Do we need to include this into 11.0.0? Or wait for next release?

sunchao · 2022-03-18T17:40:01Z

It'd be nice if we can include this in 11.0.0 and 10.0.1, although 11.0.0 RC1 is already under voting now.

sunchao · 2022-03-18T17:34:04Z

arrow/src/ffi.rs

@@ -802,6 +810,35 @@ impl ArrowArray {
    pub fn into_raw(this: ArrowArray) -> (*const FFI_ArrowArray, *const FFI_ArrowSchema) {
        (Arc::into_raw(this.array), Arc::into_raw(this.schema))
    }
+
+    /// exports [ArrowArray] to raw pointers of the C Data Interface provided by the consumer.


nit: "Exports"

sunchao · 2022-03-18T17:40:52Z

arrow/src/ffi.rs

+    /// 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,


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.

Moved. Please take another look. Thanks.

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

arrow/src/array/array.rs

sunchao

LGTM, thanks @viirya !

viirya · 2022-03-18T22:39:22Z

Thanks @sunchao @wangfenjin for the detailed review!

sunchao · 2022-03-19T06:26:59Z

Merged, thanks!

viirya · 2022-03-19T06:45:57Z

Thanks @sunchao

alamb · 2022-03-22T22:32:44Z

Since we'll release a new arrow version in ~2 weeks, this will be included there. Hopefully we can avoid API changes and release it as 11.1

Remove Clone and copy source structs internally

fa2ec44

github-actions bot added the arrow Changes to the arrow crate label Mar 16, 2022

viirya mentioned this pull request Mar 16, 2022

Replace Arc with Box in ArrowArray for FFI structs #1432

Closed

sunchao reviewed Mar 16, 2022

View reviewed changes

Remove drop_in_place and add more comment

44bafd3

viirya added 2 commits March 18, 2022 00:24

Add export_into_raw

0705d33

Fix format

27c075d

wangfenjin reviewed Mar 18, 2022

View reviewed changes

Fix clippy

29157db

sunchao reviewed Mar 18, 2022

View reviewed changes

viirya added 3 commits March 18, 2022 11:29

Move to export_array_into_raw

ac804bc

Fix clippy

3fc6fab

Fix doc

888cbe0

sunchao reviewed Mar 18, 2022

View reviewed changes

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

Use write_unaligned

6007aed

sunchao approved these changes Mar 18, 2022

View reviewed changes

sunchao merged commit d02425d into apache:master Mar 19, 2022

jhorstmann mentioned this pull request Jun 14, 2022

Fix memory leak in ffi test #1878

Merged

This was referenced Feb 9, 2023

PyArrowConvert Leaks Memory #3683

Closed

ArrowArray::try_from_raw Misleading Signature #3684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Clone and copy source structs internally #1449

Remove Clone and copy source structs internally #1449

viirya commented Mar 16, 2022

sunchao Mar 16, 2022

wangfenjin Mar 16, 2022

sunchao Mar 16, 2022

viirya Mar 16, 2022

wangfenjin Mar 16, 2022

viirya Mar 17, 2022

sunchao Mar 18, 2022

viirya Mar 18, 2022

viirya Mar 18, 2022

jhorstmann Mar 26, 2022

codecov-commenter commented Mar 16, 2022 •

edited

Loading

wangfenjin Mar 18, 2022

viirya Mar 18, 2022

wangfenjin commented Mar 18, 2022

viirya commented Mar 18, 2022

sunchao commented Mar 18, 2022

sunchao Mar 18, 2022

sunchao Mar 18, 2022

viirya Mar 18, 2022

viirya Mar 18, 2022

sunchao left a comment

viirya commented Mar 18, 2022

sunchao commented Mar 19, 2022

viirya commented Mar 19, 2022

alamb commented Mar 22, 2022

		@@ -802,6 +810,41 @@ impl ArrowArray {
		pub fn into_raw(this: ArrowArray) -> (const FFI_ArrowArray, const FFI_ArrowSchema) {

Remove Clone and copy source structs internally #1449

Remove Clone and copy source structs internally #1449

Conversation

viirya commented Mar 16, 2022

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

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

codecov-commenter commented Mar 16, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangfenjin commented Mar 18, 2022

viirya commented Mar 18, 2022

sunchao commented Mar 18, 2022

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

sunchao left a comment

Choose a reason for hiding this comment

viirya commented Mar 18, 2022

sunchao commented Mar 19, 2022

viirya commented Mar 19, 2022

alamb commented Mar 22, 2022

codecov-commenter commented Mar 16, 2022 •

edited

Loading