Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

jhorstmann · 2022-03-28T09:17:00Z

Which issue does this PR close?

Rationale for this change

I'd like to process data that is coming from outside arrow using arrow compute kernels without copying the data.

The FFI api does not seem to support this use case at the moment. The only way I found to create an FFI_ArrowArray was with an already existing ArrayData struct. Creating ArrayData requires at least one Buffer, and creating a Buffer from foreign memory again needs an FFI_ArrowArray.

What changes are included in this PR?

The owner of an allocation is tracked via a trait object instead of an FFI_ArrowArray. This allows zero-copy interoperability between arrow buffers and most collection types from rust stdlib or other crates. The most common example would be Vec or String, and it should also be possible to access arrow2 buffers this way.

Are there any user-facing changes?

The function from_unowned was marked as deprecated and delegates to a new function from_custom_allocation. The "unowned" part is abit misleading since the data is owned by the FFI_ArrayArray.

The other changes should be api compatible. The bytes module is pub(crate), so moving the Deallocation enum should be fine.

…tion from rust vectors or strings

alamb

I think this is really cool -- nice work @jhorstmann

In terms of naming, I would like to suggest we keep it backwards compatible for a while (so as not to have to bump major versions for a bit)

So that would mean

Keep the old interface named from_unowned, mark it deprecated, change impl to call from_foreign
Add new from_unowned() that does what you suggest

cc @viirya and @sunchao

arrow/src/alloc/mod.rs

alamb · 2022-03-29T19:05:28Z

arrow/src/array/data.rs

+            Buffer::from_foreign(
+                NonNull::new_unchecked(strings.as_mut_ptr()),
+                strings.len(),
+                Arc::new(strings),


It seems pretty neat to me that the ownership of the Vec is transferred to Arc which is then stored (as dyn Allocation)

alamb · 2022-03-29T19:10:46Z

BTW I ran this under MIRI and it looks good:

cargo +nightly miri test -p arrow -- test_string_data_from_foreigntest_string_data_from_foreign
...
running 1 test
test array::data::tests::test_string_data_from_foreign ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 1072 filtered out

   Doc-tests arrow

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 145 filtered out; finished in 0.03s

🎉

jhorstmann · 2022-03-29T19:52:18Z

arrow/src/alloc/mod.rs

+
+/// The owner of an allocation, that is not natively allocated.
+/// The trait implementation is responsible for dropping the allocations once no more references exist.
+pub trait Allocation: RefUnwindSafe {}


The trait bound on RefUnwindSafe was needed to make one test compile. I'll need to check that in more detail and document why it is required.

I added an explicit test that Buffer is UnwindSafe to make this requirement clearer.

arrow/src/alloc/mod.rs

viirya

This looks good idea to me. Thanks.

arrow/src/alloc/mod.rs

codecov-commenter · 2022-03-30T10:46:40Z

Codecov Report

Merging #1494 (d23e744) into master (6bf3b3a) will increase coverage by 0.01%.
The diff coverage is 75.11%.

❗ Current head d23e744 differs from pull request most recent head 55ddb82. Consider uploading reports for the commit 55ddb82 to get more accurate results

@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
+ Coverage   82.72%   82.74%   +0.01%     
==========================================
  Files         188      188              
  Lines       54286    54389     +103     
==========================================
+ Hits        44908    45002      +94     
- Misses       9378     9387       +9

Impacted Files	Coverage Δ
arrow/src/alloc/mod.rs	`78.04% <0.00%> (-13.38%)`	⬇️
arrow/src/array/array_binary.rs	`92.62% <ø> (ø)`
arrow/src/array/array_string.rs	`97.65% <ø> (ø)`
...ion-testing/src/bin/arrow-json-integration-test.rs	`0.00% <0.00%> (ø)`
arrow/src/array/equal/list.rs	`79.38% <59.57%> (-9.85%)`	⬇️
arrow/src/bytes.rs	`73.07% <80.00%> (+16.82%)`	⬆️
parquet/src/arrow/array_reader.rs	`86.70% <80.00%> (+2.34%)`	⬆️
arrow/src/buffer/immutable.rs	`98.46% <92.85%> (-1.00%)`	⬇️
arrow/src/array/equal/utils.rs	`77.68% <95.45%> (+3.68%)`	⬆️
arrow/src/array/data.rs	`83.12% <100.00%> (+0.32%)`	⬆️
... and 10 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 6bf3b3a...55ddb82. Read the comment docs.

alamb

I think this looks great @jhorstmann -- thank you. @viirya / @tustvold / @sunchao any last thoughts before I merge it in?

I believe it is an API change, so we will bump the major version of arrow to use it (but that is probably ok given stuff like #1510 which will require a new version as well

alamb · 2022-04-06T18:00:30Z

arrow/src/buffer/immutable.rs

    ///
    /// # Safety
    ///
    /// This function is unsafe as there is no guarantee that the given pointer is valid for `len`
    /// bytes and that the foreign deallocator frees the region.
+    #[deprecated(


alamb · 2022-04-06T18:02:48Z

There is a minor doctest failure: https://github.com/apache/arrow-rs/runs/5756218032?check_suite_focus=true

viirya

Looks good to me. Thanks @jhorstmann

sunchao

LGTM2

…cation

alamb · 2022-04-07T19:53:51Z

I merged this PR with master and fixed a rustdoc issue with e6c6c07 https://github.com/apache/arrow-rs/runs/5756218032?check_suite_focus=true -- thanks @jhorstmann

alamb · 2022-04-07T21:01:16Z

I believe the clippy errors are due to rust 1.60 being released. I will create a new PR

Decouple buffer deallocation from ffi and allow zero-copy buffer crea…

db72917

…tion from rust vectors or strings

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

Move allocation owner to alloc module

2cc581a

alamb reviewed Mar 29, 2022

View reviewed changes

jhorstmann commented Mar 29, 2022

View reviewed changes

viirya reviewed Mar 29, 2022

View reviewed changes

arrow/src/alloc/mod.rs Show resolved Hide resolved

viirya reviewed Mar 29, 2022

View reviewed changes

arrow/src/alloc/mod.rs Outdated Show resolved Hide resolved

Rename and comment Deallocation variants

55ddb82

jhorstmann added 2 commits March 30, 2022 16:38

Fix doc link

a919566

Explicitly assert that Buffer is UnwindSafe

246c3e5

jhorstmann marked this pull request as ready for review April 1, 2022 13:27

jhorstmann changed the title ~~RFC: Decouple buffer deallocation from ffi and allow creating buffers from rust vec~~ Decouple buffer deallocation from ffi and allow creating buffers from rust vec Apr 1, 2022

alamb approved these changes Apr 6, 2022

View reviewed changes

viirya approved these changes Apr 6, 2022

View reviewed changes

sunchao approved these changes Apr 6, 2022

View reviewed changes

alamb added 2 commits April 7, 2022 15:51

Merge remote-tracking branch 'apache/master' into decouple-ffi-deallo…

9db69cd

…cation

fix: doc comment

e6c6c07

alamb merged commit 688dd4c into apache:master Apr 7, 2022

jhorstmann mentioned this pull request Jun 24, 2022

Require Send+Sync bounds for Allocation trait #1945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

jhorstmann commented Mar 28, 2022 •

edited

Loading

alamb left a comment

alamb Mar 29, 2022

alamb commented Mar 29, 2022

jhorstmann Mar 29, 2022

jhorstmann Apr 1, 2022

viirya left a comment

codecov-commenter commented Mar 30, 2022

alamb left a comment

alamb Apr 6, 2022

alamb commented Apr 6, 2022

viirya left a comment

sunchao left a comment

alamb commented Apr 7, 2022

alamb commented Apr 7, 2022

Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

Decouple buffer deallocation from ffi and allow creating buffers from rust vec #1494

Conversation

jhorstmann commented Mar 28, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 29, 2022

Choose a reason for hiding this comment

alamb commented Mar 29, 2022

jhorstmann Mar 29, 2022

Choose a reason for hiding this comment

jhorstmann Apr 1, 2022

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 30, 2022

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 6, 2022

Choose a reason for hiding this comment

alamb commented Apr 6, 2022

viirya left a comment

Choose a reason for hiding this comment

sunchao left a comment

Choose a reason for hiding this comment

alamb commented Apr 7, 2022

alamb commented Apr 7, 2022

jhorstmann commented Mar 28, 2022 •

edited

Loading