-
Notifications
You must be signed in to change notification settings - Fork 189
Make MPSCAsyncChannel #isolation methods use nonisolated(nonsending)`.
#384
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
base: main
Are you sure you want to change the base?
Make MPSCAsyncChannel #isolation methods use nonisolated(nonsending)`.
#384
Conversation
We can perform more aggressive optimizer optimizations with nonisolated(nonsending) than #isolation. So use that instead to get some easy perf wins.
| } | ||
|
|
||
| @inlinable | ||
| mutating func next( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can change this one since this is a protocol requirement of AsyncSequence unless nonisolated(nonsending) methods are capable of fulfilling isolated protocol requirement
|
|
||
| @inlinable | ||
| func suspendNext(isolation: isolated (any Actor)? = #isolation) async throws -> Element? { | ||
| nonisolated(nonsending) func suspendNext() async throws -> Element? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my recent testing, it seems nonisolated(nonsending) does not grant the ability to actually procure a reference to the isolated parameter in a way that allows isolating closures in the same manner that capturing an explicit isolated parameter does. i suppose in this case it doesn't matter since the parameter isn't being captured by the body closures, but that does make me wonder if those closures are actually not being isolated in the intended manner currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you briefly expand what closures you are worried are not isolated in particular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, the concern would be that even though the method inherits isolation by default (in the original formulation), the isolated parameter is unused within the body closures. this is true in the prior version with an isolated parameter, and seems to also be true even after converting to implicit caller isolation inheritance. this means the actual 'work' invoked by the with* runtime methods isn't isolated, so IIUC it's going to require an executor hop if the caller is actor-isolated. this can be seen in this sort of example: https://swift.godbolt.org/z/GP4dPhfKv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so you are talking about the withTaskCancellationHandler and withCheckedContinuation handler. Both of them have open PRs to address these issues swiftlang/swift#85191 & swiftlang/swift#85518
Edit: I see that you left a comment on the latter PR. The hop should be eliminated and I think @gottesmm is looking into that.
We can perform more aggressive optimizer optimizations with nonisolated(nonsending) than #isolation. So use that instead to get some easy perf wins.