Clean up the TaskGroup ABI #36838
Merged
Clean up the TaskGroup ABI #36838
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- stop storing the parent task in the TaskGroup at the .swift level - make sure that swift_taskGroup_isCancelled is implied by the parent task being cancelled - make the TaskGroup structs frozen - make the withTaskGroup functions inlinable - remove swift_taskGroup_create - teach IRGen to allocate memory for the task group - don't deallocate the task group in swift_taskGroup_destroy To achieve the allocation change, introduce paired create/destroy builtins. Furthermore, remove the _swiftRetain and _swiftRelease functions and several calls to them. Replace them with uses of the appropriate builtins. I should probably change the builtins to return retained, since they're working with a managed type, but I'll do that in a separate commit.
ktoso
reviewed
Apr 9, 2021
ktoso
reviewed
Apr 9, 2021
ktoso
reviewed
Apr 9, 2021
| @@ -591,7 +591,7 @@ public func withUnsafeCurrentTask<T>(body: (UnsafeCurrentTask?) throws -> T) ret | |||
| // FIXME: This retain seems pretty wrong, however if we don't we WILL crash | |||
| // with "destroying a task that never completed" in the task's destroy. | |||
| // How do we solve this properly? | |||
| _swiftRetain(_task) | |||
| Builtin.retain(_task) | |||
There was a problem hiding this comment.
The remaining need for these is just that Builtin.currentAsyncTask() is returning an unretained pointer. It probably simply shouldn't, but I wanted to leave that for a different PR.
There was a problem hiding this comment.
Yeah that probably makes sense 👍 All uses of the task will then fall into place properly I think
ktoso
reviewed
Apr 9, 2021
ktoso
reviewed
Apr 9, 2021
ktoso
reviewed
Apr 9, 2021
ktoso
reviewed
Apr 9, 2021
ktoso
approved these changes
Apr 9, 2021
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please smoke test Linux |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TaskGroupat the .swift levelswift_taskGroup_isCancelledis implied by the parent task being cancelledTaskGroupstructs@frozenwithTaskGroupfunctions@inlinableswift_taskGroup_createswift_taskGroup_destroyTo achieve the allocation change, introduce paired create/destroy builtins.
Furthermore, remove the
_swiftRetainand_swiftReleasefunctions and several calls to them. Replace them with uses of the appropriate builtins. I should probably change the builtins to return retained, since they're working with a managed type, but I'll do that in a separate commit.