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

runtime: Move String implementation stubs that want need the auto-rel… #12312

Merged
merged 9 commits into from Oct 8, 2017

Conversation

aschwaighofer
Copy link
Member

…eased return value optimization to an ARC compiled file

String's hashValue function is implemented in terms of Foundation's hash
function in a runtime function on darwin platforms. For non-ASCII strings we
will call str.decomposedStringWithCanonicalMapping inside this runtime function
which will allocate a new NSString and return the result in the current
autorelease pool. We implemented this function in a file compiled without ARC.
This meant that we would leak said NSString into the current active autorelease
pool.
This patch moves the implementation to a file compiled with ARC. ARC will insert
objc_retainAutoreleasedReturnValue call and on platforms that require it an
marker for the hand-off of the autoreleased return value optimization.

SR-4889
rdar://32199117

…eased return value optimization to an ARC compiled file

String's hashValue function is implemented in terms of Foundation's hash
function in a runtime function on darwin platforms. For non-ASCII strings we
will call str.decomposedStringWithCanonicalMapping inside this runtime function
which will allocate a new NSString and return the result in the current
autorelease pool. We implemented this function in a file compiled without ARC.
This meant that we would leak said NSString into the current active autorelease
pool.
This patch moves the implementation to a file compiled with ARC. ARC will insert
objc_retainAutoreleasedReturnValue call and on platforms that require it an
marker for the hand-off of the autoreleased return value optimization.

SR-4889
rdar://32199117
@aschwaighofer
Copy link
Member Author

@swift-ci Please test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test Linux Platform
Git Sha - ec5f40f

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test OS X Platform
Git Sha - ec5f40f

@aschwaighofer
Copy link
Member Author

@swift-ci Please test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test Linux Platform
Git Sha - ec5f40f

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test OS X Platform
Git Sha - ec5f40f

/// The following two routines need to be implemented in ARC because
/// decomposedStringWithCanonicalMapping returns its result autoreleased. And we
/// want ARC to insert 'objc_retainAutoreleasedReturnValue' and the necessary
/// markers for the hand-off.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: this shouldn't be a doc comment.

// REQUIRES: objc_interop

// The autorelease return optimizaion seems to be failing in the simulator.
// XFAIL: CPU=i386
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just don't do it there. @rjmccall?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, seems to be the case. I tried with a pure objective-c app and we also leak there on an i386 simulator rdar://34863458

@@ -0,0 +1,101 @@
// UNSUPPORTED: linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to make this REQUIRES: objc_interop. This won't work on Windows or Android either, after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

@aschwaighofer
Copy link
Member Author

@swift-ci Please test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test Linux Platform
Git Sha - 75a82d7

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 6, 2017

Build failed
Swift Test OS X Platform
Git Sha - 75a82d7

@gottesmm
Copy link
Member

gottesmm commented Oct 6, 2017

@aschwaighofer Please be careful about using the word leak and autorelease pool together. Hitting the autorelease pool is not a "leak". I would say "unnecessary memory growth" or something like that. Leak is misleading.

Or am I misunderstanding the pull request?

@aschwaighofer
Copy link
Member Author

@swift-ci Please clean test linux

@aschwaighofer
Copy link
Member Author

@swift-ci Please clean test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2017

Build failed
Swift Test Linux Platform
Git Sha - f691657

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2017

Build failed
Swift Test OS X Platform
Git Sha - f691657

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants