Simplified Bytes (internal)

[jorgecarleitao]

This PR decouples Bytes from everything except std.

This ports @jhorstmann's PR in arrow-rs. Although not as relevant here since we use Vec anyways, it enable us to potentially move Bytes to its own crate.

I am not fan of using Arc instead of Box - the struct itself does not need an Arc, will try to address this in a future PR.

Relevant thread in rust-users: https://users.rust-lang.org/t/implementing-a-potentially-foreign-allocated-potentially-mutable-memory-region/77388

[jorgecarleitao]

@jhorstmann I had to require Send + Sync to the owner, or Bytes would not be Send + Sync. unsafe impl Send for Bytes in arrow-rs may be unsound if we do not add Send + Sync to it.

[codecov]

Codecov Report

Merging #1099 (0220531) into main (10b57c3) will increase coverage by 0.02%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
+ Coverage   81.23%   81.25%   +0.02%     
==========================================
  Files         367      366       -1     
  Lines       35357    35336      -21     
==========================================
- Hits        28721    28713       -8     
+ Misses       6636     6623      -13     

Impacted Files	Coverage Δ
src/buffer/bytes.rs	98.11% <95.65%> (+23.97%)	⬆️
src/ffi/array.rs	89.20% <100.00%> (-0.04%)	⬇️
src/io/ipc/read/array/boolean.rs	89.58% <0.00%> (-8.34%)	⬇️
src/array/fixed_size_binary/mod.rs	87.37% <0.00%> (-0.51%)	⬇️
src/io/ipc/read/reader.rs	96.18% <0.00%> (+0.38%)	⬆️
src/array/utf8/mod.rs	82.74% <0.00%> (+0.95%)	⬆️

---

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 10b57c3...0220531. Read the comment docs.

[ritchie46]

So we remove 1 layer of indirection to the data? And only decide at Drop what to do with the Vec we created? That saves a lot of branches!

P.S. which Arc are you referring to?

[jorgecarleitao]

So we remove 1 layer of indirection to the data? And only decide at Drop what to do with the Vec we created? That saves a lot of branches!

I believe so - although it is unclear whether the compiler was not optimizing it already.

P.S. which Arc are you referring to?

The one at Foreign(Arc<dyn RefUnwindSafe + Send + Sync>) - this arc is only here due to some constraint in the FFI that imo can go away.

[ritchie46]

I believe so - although it is unclear whether the compiler was not optimizing it already

I don't think it can. It could be either branch, so it has to check. I think.

The one at Foreign(Arc<dyn RefUnwindSafe + Send + Sync>) - this arc is only here due to some constraint in the FFI that imo can go away.

Right, and if we remove it, the clone call goes over FFI go that smart atomic pointer?

[ritchie46]

Maybe we could also use Box<[T]> and call Vec::from_raw_parts only on get_vec on native allocated data.

[jorgecarleitao]

I tried this but in get_vec we would be creating a temporary Vec and returning &mut Vec, which does not compile :(

[ritchie46]

Oh yeah.. :(

[jorgecarleitao]

Right, and if we remove it, the clone call goes over FFI go that smart atomic pointer?

We do not know about smart pointers in FFI - in this case the idea is that we can Arc the struct containing the ref counter of the FFI struct (i.e. instead of Arc<Arc<, we use Box<Arc<, just like we did for Array.