Add the ability to borrow values from the array. #147

jrmuizel · 2018-01-23T03:44:36Z

This is a replacement for #143. It allows iteration while avoiding the reference count churn of the previous approach. It uses a TCFTypeBorrow (could probably use a better name) to hold a sort of reference to the array data. I'm interested in people's thoughts and opinions on this approach.

This change is

jdm · 2018-01-23T18:53:23Z

I like this approach. It doesn't appear to have any downsides that I can see.

faern · 2018-01-23T23:17:31Z

core-foundation/src/array.rs

-        TCFType::wrap_under_get_rule(mem::transmute(x))
+unsafe impl<T: TCFType> FromVoid for T where T::Ref: FromVoid {
+    unsafe fn from_void(x: *const c_void) -> Self {
+        TCFType::wrap_under_get_rule(T::Ref::from_void(x))


TCFType::Ref is already forced to implement TCFTypeRef, which has a from_void_ptr method. So you could remove where T::Ref: FromVoid and use T::Ref::from_void_ptr(x) instead. Would work the same, but would be slightly simpler/more minimal maybe?

I went with this approach

faern · 2018-01-23T23:17:31Z

core-foundation/src/array.rs

+
+unsafe impl FromVoidBorrow for u32 {
+    unsafe fn from_void_borrow<'a>(x: *const c_void) -> TCFTypeBorrow<'a, Self> {
+        TCFTypeBorrow(x as usize as u32, PhantomData)


I do not understand this casting. Maybe I'm just too tired. There is no dereferencing of the pointer, so would the resulting u32 not contain the pointer address? Quite likely a truncated version of it as well.

Yes, it's intentionally not dereferencing it. I've added a comment to explain it more and dropped the 'as usize' as it doesn't seem to be necessary.

jrmuizel · 2018-01-24T02:47:38Z

I've cleaned this up by renaming TCFTypeBorrow to ItemRef, replacing FromVoid with FromVoidBorrow and making get() return an ItemRef instead of having a separate get_borrow()

faern · 2018-01-24T10:11:30Z

core-foundation/src/array.rs

-        TCFType::wrap_under_get_rule(mem::transmute(x))
+unsafe impl<T: TCFType> FromVoid for T {
+    unsafe fn from_void<'a>(x: *const c_void) -> ItemRef<'a, Self> {
+        ItemRef(TCFType::wrap_under_create_rule(T::Ref::from_void_ptr(x)), PhantomData)


Won't the drop of the constructed T do a CFRelease and make the retain count wrong? If you construct a new T from a void pointer, should you not do it under the get rule?

Could you add a test that checks retain counts are correct before and after borrowing values from the array?

Also, nitpicking: T::wrap_under_... should work as well? Making it a bit more clear which type we create here.

Yep. I've messed up the reference counts here. I need to think about how to fix it. wraping with the get rule will increase the retain count which defeats the point of this patch.

The underlying ffi function, CFArrayGetValueAtIndex specifies that values returned should follow the get rule. So if you would use CFArray directly in Objective-C you would likely bump the retain count? So is there any reason we should be allowed not to? If we convert the value into a CF object and hold a reference to that then should the retain count not reflect how many handles there are to that object?

Rust, in theory, should let us use the existing reference in the array safely. Since its lifetime will be tied to the array we should be able to interact with those objects without needing to bump the retain count.

i.e. imagine if you have Vec<Rc>. You can iterate over them without having to bump the reference counts on all of the objects.

Yes true. As long as the borrow can't outlive the array yes indeed!

How about you just mem::forget the T value in the Drop impl for ItemRef?

fn drop takes &mut self so it won't let me just mem::forget() the T value

But using ManuallyDrop let's me do it.

faern · 2018-01-24T17:07:50Z

@jrmuizel Would it not be nice to get rid of the possible panic in CFArray::get? Replace it with returning an Option<...>? Or at least have one get variant that has this behavior?

If we talk about performance, the current CFArrayIterator use CFArray::get which means you have a bounds check for every call to next on the iterator. We actually have two since CFArrayIterator::next is also checking the bounds itself.

Is the array not guaranteed to be immutable? If so, the length of the array can be queried once and stored in the iterator for comparison without doing the ffi call to CFArrayGetCount every time. The iterator could then use a non-bounds checking (maybe unsafe?) version of the get method.

jrmuizel · 2018-01-24T22:14:47Z

I implemented the bounds checking in #148


        Add the ability to borrow values from the array.

This uses borrowed values from the array to avoid having to Retain/Release during iteration. It also implements borrowing for all TCFType.

jrmuizel · 2018-01-25T15:10:13Z

This is ready for review. It would be good to get this into the 0.5 release so we avoid breaking compat twice.

jdm · 2018-01-25T15:32:46Z

@bors-servo: r+
Thanks for working on the details!

bors-servo · 2018-01-25T15:32:47Z

📌 Commit df7df6b has been approved by jdm

bors-servo · 2018-01-25T15:32:51Z

⌛ Testing commit df7df6b with merge 4c7e92c...


        Auto merge of #147 - jrmuizel:borrow, r=jdm

Add the ability to borrow values from the array. This is a replacement for #143. It allows iteration while avoiding the reference count churn of the previous approach. It uses a TCFTypeBorrow (could probably use a better name) to hold a sort of reference to the array data. I'm interested in people's thoughts and opinions on this approach.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/147)

bors-servo · 2018-01-25T15:55:04Z

☀️ Test successful - status-travis
Approved by: jdm
Pushing 4c7e92c to master...

faern reviewed Jan 23, 2018

View changes

jrmuizel force-pushed the jrmuizel:borrow branch from dbabba4 to 922da73 Jan 24, 2018

faern reviewed Jan 24, 2018

View changes

jrmuizel force-pushed the jrmuizel:borrow branch 2 times, most recently from 7d34492 to be0bdd5 Jan 24, 2018

jrmuizel changed the title ~~[RFC] Add the ability to borrow values from the array.~~ Add the ability to borrow values from the array. Jan 24, 2018

jrmuizel force-pushed the jrmuizel:borrow branch from be0bdd5 to d7e214c Jan 25, 2018

Add the ability to borrow values from the array.

Loading status checks…

df7df6b

This uses borrowed values from the array to avoid having to Retain/Release during iteration. It also implements borrowing for all TCFType.

jrmuizel force-pushed the jrmuizel:borrow branch from d7e214c to df7df6b Jan 25, 2018

bors-servo merged commit df7df6b into servo:master Jan 25, 2018
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

homu Test successful
Details

bors-servo mentioned this pull request Jan 25, 2018

Lift getting the length of the array out. #148

Merged

servo / core-foundation-rs

Add the ability to borrow values from the array. #147

Add the ability to borrow values from the array. #147

jrmuizel commented Jan 23, 2018 •

edited by larsbergstrom

jdm commented Jan 23, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Jan 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

faern commented Jan 24, 2018

jrmuizel commented Jan 24, 2018

jrmuizel commented Jan 25, 2018

jdm commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

servo / core-foundation-rs

Join GitHub today

Add the ability to borrow values from the array. #147

Add the ability to borrow values from the array. #147

Conversation

jrmuizel commented Jan 23, 2018 • edited by larsbergstrom

jdm commented Jan 23, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Jan 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

faern commented Jan 24, 2018

jrmuizel commented Jan 24, 2018

jrmuizel commented Jan 25, 2018

jdm commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

jrmuizel commented Jan 23, 2018 •

edited by larsbergstrom