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

Isolated synchronous deinit and asynchronous deinit #2035

Merged
merged 13 commits into from
Jul 24, 2024

Conversation

nickolas-pohilets
Copy link
Contributor

@nickolas-pohilets nickolas-pohilets commented May 1, 2023

Second iteration of the proposal:

  • synchronous deinit is nonisolated by default
  • values of task-locals are undefined for best performance and to enable future refinement

- synchronous deinit is nonisolated by default
- async deinit
- attribute to control task-local usage
@nickolas-pohilets nickolas-pohilets marked this pull request as draft May 5, 2023 11:23
@rjmccall rjmccall added the LSG Contains topics under the domain of the Language Steering Group label Apr 1, 2024
@nickolas-pohilets nickolas-pohilets marked this pull request as ready for review May 28, 2024 18:34
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, I think this looks good and let's ask the @swiftlang/language-steering-group to schedule another review round :-)

Copy link
Member

@Jumhyn Jumhyn left a comment

Choose a reason for hiding this comment

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

Just a few editorial notes!

proposals/0371-isolated-synchronous-deinit.md Outdated Show resolved Hide resolved
proposals/0371-isolated-synchronous-deinit.md Outdated Show resolved Hide resolved
proposals/0371-isolated-synchronous-deinit.md Show resolved Hide resolved
class Bar {
@SecondActor
deinit {} // Isolated on SecondActor
If containing class is not isolated, it is still possible to use global actor attribute on `deinit`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If containing class is not isolated, it is still possible to use global actor attribute on `deinit`.
If the containing type is not isolated, it is still possible to use a global actor attribute on `deinit`.

Copy link
Contributor Author

@nickolas-pohilets nickolas-pohilets Jul 22, 2024

Choose a reason for hiding this comment

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

I don't want to use type to avoid giving false impression that this feature can be used with move-only value types.

proposals/0371-isolated-synchronous-deinit.md Outdated Show resolved Hide resolved
proposals/0371-isolated-synchronous-deinit.md Outdated Show resolved Hide resolved
proposals/0371-isolated-synchronous-deinit.md Outdated Show resolved Hide resolved

### Isolated deinit of default actors
For isolated synchronous `deinit`, this cost increases. For each class in the hierarchy `swift_task_deinitOnExecutor()` will be called, but slow path can be taken only for the most derived class. Other classes in the hierarchy would be doing redundant actor checks. To avoid this, isolated deinit bypasses Objective-C runtime when calling isolated `deinit` of Swift base classes, and calls `__isolated_deallocating_deinit` directly. Compatibility with Objective-C is preserved only on Swift<->Objective-C boundary in either direction.
Copy link
Member

Choose a reason for hiding this comment

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

This is also a bit implementation-y, any way we could rephrase in terms of the actual language model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded a bit, but it still contains mentions __isolated_deallocating_deinit.

@Jumhyn Jumhyn merged commit e20fa6d into swiftlang:main Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSG Contains topics under the domain of the Language Steering Group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants