Add consuming cheap cast #142

faern · 2018-01-14T13:12:23Z

There are a lot of places where we increment and decrement the retain count when we would not have to. For example, a very common use-case with PropertyLists is to just directly cast them to their correct sub type and then throw the PropertyList away (because there is nothing you can do with the PropertyList instance directly.)

The reason we do this, I guess, is mostly because all Drop implementations force a retain count decrement. So we have to keep incrementing it to keep it correct. But if we add a flag to types that can be used to disable the decrement, then we can also get rid of the increment when we virtually just want to cast a pointer between two types.

This PR implements this by adding a boolean flag to all CF structs that is used by the Drop impl to determine if it should CFRelease the instance or not. Then this flag is modified in appropriate places where we cast to other types and consume ourselves.

I added some benchmarks locally. I did not want to commit them since benchmarking only works on nighty, and I did not feel like adding a feature flag for it etc. Anyhow, the following benchmarks:

#[bench]
fn bench_before(b: &mut Bencher) {
   let string = CFString::from_static_string("Bar");
    b.iter(|| unsafe {
        string.clone()
            .to_CFPropertyList()
            .downcast::<_, CFString>()
            .unwrap()
            .disable_release();
    })
}

#[bench]
fn bench_after(b: &mut Bencher) {
    let string = CFString::from_static_string("Bar");
    b.iter(|| unsafe {
        string.clone()
            .into_CFPropertyList()
            .downcast_into::<_, CFString>()
            .unwrap()
            .disable_release();
    })
}

Given the following results:

test propertylist::test::bench_after         ... bench:          16 ns/iter (+/- 3)
test propertylist::test::bench_before        ... bench:          73 ns/iter (+/- 14)

The final call to disable_release() in the benchmarks are to prevent the type coming out of the unwrap() from calling CFRelease on drop, and thus avoid measuring that. I want to try to measure only {to,into}_CFPropertyList, downcast and downcast_into. The clone is needed because the into version requires it and it would then not be fair to not have it on both.

I would say that speeding it up by a factor of ~4 can be well worth it? (Well, that 4 is of course no good measurement of anything, microbenchmarks etc. etc.. The benchmark mostly shows that a lot can be gained from avoiding some retain counting.)

This change is

faern · 2018-01-14T13:14:16Z

Other than the cheap into casts this PR also adds the normal downcast to CFType making it easy to downcast it directly to a subtype. As you can see this makes scenarios such as is the CFArray tests much easier to write (and without need for unsafe)

jrmuizel · 2018-01-15T01:37:30Z

I haven't looked at this in detail so don't understand it yet but I'm not really a fan of growing the size of the CFTypes

jrmuizel · 2018-01-15T01:52:05Z

Can't we just transmute between Ref types to avoid Drop?

jrmuizel · 2018-01-15T02:10:38Z

Or rather transmute the wrapper types?

jrmuizel · 2018-01-15T02:15:26Z

Or perhaps add a method on the wrapper type that returns the Ref type but doesn't drop()? Most wrapper types could implement this using transmute to avoid the drop.

faern · 2018-01-15T08:23:53Z

@jrmuizel I guess it would be possible to use mem::forget(self) in the consuming methods that are now utilizing disable_release instead. I just felt this was a bit more general and safe though, since a mem::forget(self) would rely on the fact that the drop only did the release. If any type introduced any other drop logic it would also not run that. But I guess we might be able to assume avoiding drop only does the release after all, since I now moved the drop impl to the macro, so all CF types automatically have the same code there. Except the special ones who does not yet use declare_TCFType.

What are your arguments against growing the structs by one bool?

faern · 2018-01-15T08:45:26Z

I have now pushed a commit that removes disable_release and instead uses mem::forget(self) to avoid the drop impl and thus the CFRelease call. This leaves all structs at their original size (one pointer). If people agree this is a preferred way I can squash this last fix into the main commit doing this change.

jrmuizel · 2018-01-15T19:00:22Z

What are your arguments against growing the structs by one bool?
None of my arguments are amazing, but here they are:

it causes us to lose what's mostly a zero-cost abstraction
it has a small increase of the overhead of drop()
it means that arrays of CFTypes use up twice as much space
it means that we don't have any chance of transmuting the void* array that CFArrayGetValues produces into the wrapper types.

faern · 2018-01-15T19:26:20Z

Yes, keeping them pointer sized is probably very desirable. Especially since it was so easy to achieve the same results without adding the bool.

jdm

I like these changes.

jdm · 2018-01-17T21:13:58Z

core-foundation/src/array.rs

                sum += number.to_i64().unwrap()
            }

            assert!(sum == 15);

            for elem in arr.iter() {
-                let number: CFNumber = TCFType::wrap_under_get_rule(mem::transmute(elem));
+                let number: CFNumber = TCFType::wrap_under_get_rule(mem::transmute(elem.as_concrete_TypeRef()));


Why are these changes necessary?

Ehm.. They are not, any longer. they were before when I actually changed the structs to contain a bool, then the transmute did not work because the types were of different size. I'll remove it! It's removed in the commit after anyway, but anyhow.

jdm · 2018-01-17T21:35:04Z

@bors-servo r+

bors-servo · 2018-01-17T21:35:05Z

📌 Commit cde8e00 has been approved by jdm

faern · 2018-01-18T07:33:30Z

Optimally this should be rebased when #135 is merged and updated, since that PR add's a type that does not make use of the macro here.


        Add macro that declare CF types with Drop impl


        Add cheap casting between CF types


        Simplify CFArray test with new downcast method


        Improve documentation and add #[inline]


        Convert CFMutableDictionary to use declare_TCFType

jdm · 2018-01-20T23:18:30Z

@bors-servo r+

bors-servo · 2018-01-20T23:18:30Z

📌 Commit 3ec0d57 has been approved by jdm

jdm · 2018-01-20T23:19:26Z

@bors-servo p=1

faern · 2018-01-21T00:04:10Z

PR #84 also adds a type without making use of the declare_TCFType! macro. So if that one is merged and I'm not away until this gets auto merged as well then I'll rebase this and fix that.

bors-servo · 2018-01-21T01:13:22Z

⌛ Testing commit 3ec0d57 with merge 8834563...


        Auto merge of #142 - faern:add-consuming-cheap-cast, r=jdm

Add consuming cheap cast There are a lot of places where we increment and decrement the retain count when we would not have to. For example, a very common use-case with PropertyLists is to just directly cast them to their correct sub type and then throw the PropertyList away (because there is nothing you can do with the PropertyList instance directly.) The reason we do this, I guess, is mostly because all `Drop` implementations force a retain count decrement. So we have to keep incrementing it to keep it correct. But if we add a flag to types that can be used to disable the decrement, then we can also get rid of the increment when we virtually just want to cast a pointer between two types. This PR implements this by adding a boolean flag to all CF structs that is used by the `Drop` impl to determine if it should `CFRelease` the instance or not. Then this flag is modified in appropriate places where we cast to other types and consume ourselves. I added some benchmarks locally. I did not want to commit them since benchmarking only works on nighty, and I did not feel like adding a feature flag for it etc. Anyhow, the following benchmarks: ```rust #[bench] fn bench_before(b: &mut Bencher) { let string = CFString::from_static_string("Bar"); b.iter(|| unsafe { string.clone() .to_CFPropertyList() .downcast::<_, CFString>() .unwrap() .disable_release(); }) } #[bench] fn bench_after(b: &mut Bencher) { let string = CFString::from_static_string("Bar"); b.iter(|| unsafe { string.clone() .into_CFPropertyList() .downcast_into::<_, CFString>() .unwrap() .disable_release(); }) } ``` Given the following results: ``` test propertylist::test::bench_after ... bench: 16 ns/iter (+/- 3) test propertylist::test::bench_before ... bench: 73 ns/iter (+/- 14) ``` The final call to `disable_release()` in the benchmarks are to prevent the type coming out of the `unwrap()` from calling `CFRelease` on drop, and thus avoid measuring that. I want to try to measure only `{to,into}_CFPropertyList`, `downcast` and `downcast_into`. The `clone` is needed because the `into` version requires it and it would then not be fair to not have it on both. I would say that speeding it up by a factor of ~4 can be well worth it? (Well, that 4 is of course no good measurement of anything, microbenchmarks etc. etc.. The benchmark mostly shows that a lot can be gained from avoiding some retain counting.)  --- 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/142)

bors-servo · 2018-01-21T02:58:51Z

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

faern mentioned this pull request Jan 15, 2018

Reference type as associated value instead of generic #132

Merged

faern force-pushed the faern:add-consuming-cheap-cast branch 3 times, most recently from 80ab652 to 1433fec Jan 15, 2018

jdm approved these changes Jan 17, 2018

View changes

faern force-pushed the faern:add-consuming-cheap-cast branch from 1433fec to cde8e00 Jan 17, 2018

faern force-pushed the faern:add-consuming-cheap-cast branch from cde8e00 to 01e02dc Jan 18, 2018

faern added 4 commits Jan 14, 2018

Add macro that declare CF types with Drop impl

4fc21d9

Add cheap casting between CF types

b132574

Simplify CFArray test with new downcast method

3f4f6a0

Improve documentation and add #[inline]

8a52a34

faern force-pushed the faern:add-consuming-cheap-cast branch from 01e02dc to 237e838 Jan 20, 2018

Convert CFMutableDictionary to use declare_TCFType

Loading status checks…

3ec0d57

faern force-pushed the faern:add-consuming-cheap-cast branch from 237e838 to 3ec0d57 Jan 20, 2018

bors-servo merged commit 3ec0d57 into servo:master Jan 21, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

faern deleted the faern:add-consuming-cheap-cast branch Jan 21, 2018

servo / core-foundation-rs

Add consuming cheap cast #142

Add consuming cheap cast #142

faern commented Jan 14, 2018 •

edited

faern commented Jan 14, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

faern commented Jan 15, 2018

faern commented Jan 15, 2018 •

edited

jrmuizel commented Jan 15, 2018

faern commented Jan 15, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 17, 2018

bors-servo commented Jan 17, 2018

faern commented Jan 18, 2018

jdm commented Jan 20, 2018

bors-servo commented Jan 20, 2018

jdm commented Jan 20, 2018

faern commented Jan 21, 2018

bors-servo commented Jan 21, 2018

bors-servo commented Jan 21, 2018

servo / core-foundation-rs

Join GitHub today

Add consuming cheap cast #142

Add consuming cheap cast #142

Conversation

faern commented Jan 14, 2018 • edited

faern commented Jan 14, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

faern commented Jan 15, 2018

faern commented Jan 15, 2018 • edited

jrmuizel commented Jan 15, 2018

faern commented Jan 15, 2018

jdm left a comment

This comment has been minimized.

jdm Jan 17, 2018

This comment has been minimized.

faern Jan 17, 2018

jdm commented Jan 17, 2018

bors-servo commented Jan 17, 2018

faern commented Jan 18, 2018

jdm commented Jan 20, 2018

bors-servo commented Jan 20, 2018

jdm commented Jan 20, 2018

faern commented Jan 21, 2018

bors-servo commented Jan 21, 2018

bors-servo commented Jan 21, 2018

faern commented Jan 14, 2018 •

edited

faern commented Jan 15, 2018 •

edited