Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Id::retain_autoreleased #81

Merged
merged 6 commits into from
Mar 2, 2022
Merged

Conversation

madsmtm
Copy link
Owner

@madsmtm madsmtm commented Nov 23, 2021

An optimized version of Id::retain that uses objc_retainAutoreleasedReturnValue for when the previous caller returns an autoreleased value with objc_autoreleaseReturnValue. This is a common thing in ARC code to avoid unnecessary autorelease/retain pairs, but it is (apart from changing the retain count) purely an optimization.

This uses the newly stabilized inline assembly, which can easily cause very undefined behaviour if used incorrectly. Luckily, the instructions are exceedingly simple (essentially just a special nop), so there's almost no room for error - so fingers crossed I got this right (at least enough that it doesn't cause UB)!

I've tested this on my local machine (both x86 and x86_64), and I've also added assembly tests to ensure that enough optimizations actually happen to make it work. At some point I would really like a helper like Apple's TestRoot instead of the brittle checking of retainCount that we do now, but I'll defer that to later.

Related: gfx-rs/metal-rs#222

@madsmtm madsmtm added enhancement New feature or request help wanted Extra attention is needed labels Nov 23, 2021
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch 4 times, most recently from 6f90766 to ae34940 Compare November 23, 2021 12:49
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch from ae34940 to 419086c Compare January 11, 2022 23:06
This was referenced Jan 12, 2022
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch from 419086c to 608d251 Compare March 1, 2022 00:44
@madsmtm madsmtm mentioned this pull request Mar 1, 2022
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch 2 times, most recently from 8f43ce3 to 5d1b3f9 Compare March 1, 2022 08:50
@madsmtm
Copy link
Owner Author

madsmtm commented Mar 1, 2022

Since objc_retainAutoreleasedReturnValue, objc_retain, and so on return the same argument that they're given with, I briefly tested if it would be useful for LLVM to have this knowledge, encoded as follows.

if ptr != res {
    unsafe { core::hint::unreachable_unchecked() };
}

Conclusion: This was turned in to an @llvm.assume intrinsic, which can apparently have (and did have) detrimental impact on performance. So no.

@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch from b5297b9 to 908f914 Compare March 2, 2022 21:34
@madsmtm
Copy link
Owner Author

madsmtm commented Mar 2, 2022

I considered just changing Id::retain to use objc_retainAutoreleasedReturnValue, but decided against it, even though most of the time you want to use Id::retain_autoreleased. Primary reasons was: to stay closer to Objective-C naming, and because #120 hopefully will mean most users don't have to think about it.

@madsmtm madsmtm removed the help wanted Extra attention is needed label Mar 2, 2022
An optimized version of `Id::retain` for when the previous caller returns an autoreleased value
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch from 908f914 to 597c950 Compare March 2, 2022 21:47
@madsmtm madsmtm force-pushed the optimized-autorelease-retain branch from 597c950 to e149351 Compare March 2, 2022 22:06
@madsmtm
Copy link
Owner Author

madsmtm commented Mar 2, 2022

Note: given that Cranelift doesn't support inline assembly, neither does rustc_codegen_cranelift yet, so that is broken.

A possibility would be to put the assembly stuff inside Id::retain_autoreleased behind a feature flag, I'm up for it if someone finds they need it.

@madsmtm madsmtm merged commit b66594d into master Mar 2, 2022
@madsmtm madsmtm deleted the optimized-autorelease-retain branch March 2, 2022 22:56
@madsmtm madsmtm added this to the objc2 v0.3 milestone Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant