Skip to content

[stdlib] Make _stdlib_initializeReturnAutoreleased non-@inlinable #17909

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

Merged
merged 1 commit into from
Jul 13, 2018
Merged

[stdlib] Make _stdlib_initializeReturnAutoreleased non-@inlinable #17909

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions stdlib/public/core/Runtime.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,31 +544,17 @@ open class _SwiftNativeNSData {
// Support for reliable testing of the return-autoreleased optimization
//===----------------------------------------------------------------------===//

@_fixed_layout // FIXME(sil-serialize-all)
@usableFromInline // FIXME(sil-serialize-all)
@objc
internal class _stdlib_ReturnAutoreleasedDummy {
@inlinable // FIXME(sil-serialize-all)
@objc
internal init() {}

@inlinable // FIXME(sil-serialize-all)
deinit {}

// Use 'dynamic' to force Objective-C dispatch, which uses the
// return-autoreleased call sequence.
@objc
internal dynamic func returnsAutoreleased(_ x: AnyObject) -> AnyObject {
return x
}

// Use 'dynamic' to prevent this call to be duplicated into other modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment worries me a little. Are these pieces of code really equivalent?

Copy link
Member Author

Choose a reason for hiding this comment

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

We lose a call through the Objective-C runtime, but the comment indicates this particular one was only there to defeat implicit inlinability in previous versions of the stdlib.

The important call is the one to returnsAutoreleased; that needs to go through objc_msgSend or else this turns into a messy noop. I verified that the important call still gets compiled to use message passing after this change.

Copy link
Member Author

@lorentey lorentey Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Update: This may still turn out to be a messy no-op; disabling the call to _stdlib_initializeReturnAutoreleased() in StdlibUnittest seemingly has no effect on any of the tests.

Copy link
Contributor

@jckarter jckarter Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This affects only the i386 slice, where because of the way the autorelease return value optimization was implemented, the first attempt would always fail due to the lazy PLT stub. The intent here is to make sure at least one return-autorelease occurs before the test suite so that this behavior doesn't cause behavior variation in tests. Make sure you test a 32-bit simulator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, the #if arch(x86_64) makes this a no-op on the 32-bit simulators. Is that condition wrong?

(In #17907, I found that ARC on x86-64 does require a warmup like this -- except I had to make it call into Foundation.)

Copy link
Contributor

@jckarter jckarter Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, maybe I misremembered and it's x86_64 that's affected (@gparker42 would know). It could be that these days something else triggers an autorelease-return early enough to happen before this one, maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

My comment above was mistaken: re-running the tests with the warmup disabled actually led to some newly reported leaks. (Well, one leak, in Array.BridgedToObjC/Custom/BridgeBack/Cast, when the test was compiled without optimizations.) So the warmup is actually necessary, and the code does work as intended.

Testing with lldb confirms that _stdlib_initializeReturnAutoreleased does indeed perform the first call to objc_autoreleaseReturnValue, at least in the handful of tests I tried.

TL;DR: All is well. 🎉

@objc
internal dynamic func initializeReturnAutoreleased() {
// On x86_64 it is sufficient to perform one cycle of return-autoreleased
// call sequence in order to initialize all required PLT entries.
_ = self.returnsAutoreleased(self)
}
}

/// This function ensures that the return-autoreleased optimization works.
Expand All @@ -583,11 +569,12 @@ internal class _stdlib_ReturnAutoreleasedDummy {
/// separate for each dylib.
///
/// Call this function in a fresh autorelease pool.
@inlinable // FIXME(sil-serialize-all)
public func _stdlib_initializeReturnAutoreleased() {
// _stdlib_initializeReturnAutoreleasedImpl()
#if arch(x86_64)
_stdlib_ReturnAutoreleasedDummy().initializeReturnAutoreleased()
// On x86_64 it is sufficient to perform one cycle of return-autoreleased
// call sequence in order to initialize all required PLT entries.
let dummy = _stdlib_ReturnAutoreleasedDummy()
_ = dummy.returnsAutoreleased(dummy)
#endif
}
#else
Expand Down