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.

Sign up for GitHub

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

Jump to bottom

Add autoreleasepool functionality. #67

Merged
merged 1 commit into from
Jul 22, 2018
Merged

Add autoreleasepool functionality. #67

merged 1 commit into from
Jul 22, 2018

Conversation

jrmuizel
Copy link
Contributor

This replicates the behaviour of @autoreleasepool blocks in
Objective-C and allows higher performance creation of autorelease pools.

@jrmuizel jrmuizel mentioned this pull request Jul 11, 2018
Open
kvark
kvark reviewed Jul 11, 2018
View reviewed changes
src/autorelease.rs Outdated
}

impl AutoReleaseHelper {
fn new() -> AutoReleaseHelper {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Self

src/autorelease.rs Outdated
use runtime::{objc_autoreleasePoolPush, objc_autoreleasePoolPop};

struct AutoReleaseHelper {
context: *mut c_void
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be something more specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the runtime returns.

src/autorelease.rs Outdated
}
}

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to be a doc comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

src/autorelease.rs Outdated
after the execution of `f` completes. This corresponds to @autoreleasepool blocks
in Objective-c and Swift.
*/
pub fn autoreleasepool<F: FnOnce()>(f: F) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps, we should just make it a method of AutoReleaseHelper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. That would be unsafe because we can't guarantee that drop() will be called.

@kvark
Copy link

kvark commented Jul 11, 2018

CI went crazy, eh

@jrmuizel jrmuizel closed this Jul 11, 2018
@jrmuizel jrmuizel reopened this Jul 11, 2018
@jrmuizel
Copy link
Contributor Author

It looks the ci failure is unrelated to the change.

@SSheldon
Copy link
Owner

SSheldon commented Jul 12, 2018 •
edited
Loading

@jrmuizel this looks great, but I'm not sure about adding it to objc right now. I don't completely understand the relationship between libobjc and reference counting. These functions aren't defined in the public headers for libobjc. The gnustep- implementation of libobjc therefore doesn't include them, but instead implements them as part of Foundation. I think the objc crate's gnustep support might be broken currently, but it's supposed to be supported to some extent 😛

Although Apple's libobjc bakes in some parts of Foundation (like the implementation of NSObject and this), for now I've kept reference-counting stuff out of the objc crate (though it used to be here and #24 was filed when it was removed).

I'm not sure where the best home for this is then. If y'all would be interested in adding this to objc_id, I'd be happy to accept it there! If that's no good for you, maybe in the foundation module of the cocoa crate to replace its NSAutoreleasePool?

Let me know what you think!

SSheldon
SSheldon reviewed Jul 12, 2018
View reviewed changes
src/autorelease.rs Outdated

impl AutoReleaseHelper {
unsafe fn new() -> Self {
AutoReleaseHelper{ context: objc_autoreleasePoolPush()}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIt: spacing got a little inconsistent here :)

@ngrewe
Copy link
Contributor

ngrewe commented Jul 12, 2018 •
edited
Loading

These functions aren't defined in the public headers for libobjc.

IIRC, the functions are part of the interface between the compiler and the runtime. They're not normally called from user code, hence not exposed in the normal runtime headers.

The gnustep implementation of libobjc therefore doesn't include them, but instead implements them as part of Foundation.

There might be a compatibiltiy shim for the legacy gcc runtime in Gnustep's Foundation library, but the modern runtime definitely implements them directly.

@SSheldon
Copy link
Owner

There might be a compatibiltiy shim for the legacy gcc runtime in Gnustep's Foundation library, but the modern runtime definitely implements them directly.

Ah, I must've confused objc_retain with NSObject! I remembered having to remove references to NSObject in #32 because it was defined in libobjc on mac but not in gnustep.

@jrmuizel
Copy link
Contributor Author

I've fixed the spacing and it sounds like everyone agrees that this rust-objc is the right place for this?

@jrmuizel
Copy link
Contributor Author

Review ping.

@kvark kvark mentioned this pull request Jul 21, 2018
Closed
22 tasks
@kvark
Copy link

kvark commented Jul 21, 2018

@SSheldon ping

@SSheldon
Copy link
Owner

Sorry, procrastinated responding because I wasn't sure what to say without more time to think, and last weekend I was out of town. This weekend I've had some time though!

It feels very odd to have autorelease pool functionality without retain/release functionality in the objc crate, so after adding this I'm also going to add retain/release functionality. I think I've gotten some ideas of how I want to do that.

Current problem, though, is how to test any reference counting functionality. The problem is that the reference counting implementations basically require that your object subclass NSObject (like having retain/release/autorelease/dealloc methods). And unfortunately the tests can't currently use NSObject, because it's not available from the GNUstep runtime. So I'm currently investigating if I can get the GNUstep Foundation working.

When that's figured out, we can merge this and write some tests for it.

@SSheldon
Copy link
Owner

Okay, after a very long struggle: I've gotten to an okay place with the GNUstep tests and I've added some reference counting utils on master.

Can you rebase this change and:

  1. make sure the tests are still passing on Travis
  2. move autoreleasepool into the rc module

@jrmuizel
Add autoreleasepool functionality.
2f906ee 
This replicates the behaviour of @autoreleasepool blocks in
Objective-C and allows higher performance creation of autorelease pools.
@jrmuizel
Copy link
Contributor Author

Done. I added a test of the autorelease functionality as well.

@SSheldon SSheldon merged commit 2f906ee into SSheldon:master Jul 22, 2018
@SSheldon
Copy link
Owner

Thank you! Looks great.

@kvark
Copy link

kvark commented Jul 23, 2018

What would be even better if autoreleasepool closure could return a value. Otherwise, it doesn't seem to be applicable much to us, i.e. more often then not we want ARP around some object creation that we retain.

@kvark
Copy link

kvark commented Jul 23, 2018

@SSheldon how do you feel about yanking the version, fixing autoreleaespool to return a value, and releasing a new patch version? I don't think anybody is using the new function yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SSheldon SSheldon SSheldon left review comments

@kvark kvark kvark left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrmuizel @kvark @SSheldon @ngrewe