Skip to content

Isolated synchronous deinit #60057

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

Jump to bottom
Merged
merged 57 commits into from
Sep 3, 2024
Merged

Isolated synchronous deinit #60057

merged 57 commits into from
Sep 3, 2024

Conversation

nickolas-pohilets
Copy link
Contributor

@nickolas-pohilets nickolas-pohilets commented Jul 14, 2022 •
edited
Loading

Implementation of the Isolated asynchronous deinit

Still pending:

  • I suspect that logic for adding attributes for inferred isolation is not correct, would like some input from the Core team about this.
  • Still need to integrate with distributed actors, waiting for [Distributed] prevent remote distributed actor's from running deinit bodies #60050 to be merged.
  • Didn't copy changes to BackDeployConcurrency yet. I expect that I'll have to make changes based on review comments, and want to apply all changes to BackDeployConcurrency in one go. I would appreciate some advice for maintaining the two in sync.
  • Investigate fast path for self-isolated default actor's deinit

@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 14fe1cd to 0417ee9 Compare July 15, 2022 13:25
DougGregor
DougGregor reviewed Jul 16, 2022
View reviewed changes
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is fantastic! A couple of comments and questions, both in design and implementation, but it's very much going in the right direction.

@DougGregor
Copy link
Member

  • Didn't copy changes to BackDeployConcurrency yet. I expect that I'll have to make changes based on review comments, and want to apply all changes to BackDeployConcurrency in one go. I would appreciate some advice for maintaining the two in sync.

BackDeployConcurrency is actually supposed to be frozen in time, and not updated with new features. For back-deployment of this feature, we'll have to bring up another statically-linked back-deployment library that would be linked into all problems that build with a deployment target that predates this work. @etcwilde is starting to bring up such a library to back-deploy other concurrency-related fixes. Once that's in place, this PR can build on that work.

@nickolas-pohilets
Copy link
Contributor Author

BackDeployConcurrency is actually supposed to be frozen in time, and not updated with new features. For back-deployment of this feature, we'll have to bring up another statically-linked back-deployment library that would be linked into all problems that build with a deployment target that predates this work. @etcwilde is starting to bring up such a library to back-deploy other concurrency-related fixes. Once that's in place, this PR can build on that work.

I would really appreciate a document about how to work with BackDeployConcurrency. Could not find anything in the repo.

@ktoso ktoso self-requested a review July 19, 2022 11:18
@ktoso
Copy link
Contributor

ktoso commented Jul 20, 2022

AFAICS the distributed adjustment looks good 👍
We only resignID as before in the local branch, and the remote does only destroy fields properly, looks good to me.

@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 8e3201c to facc0ca Compare July 25, 2022 11:12
@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from a4486cc to 292f3e1 Compare July 29, 2022 18:35
nickolas-pohilets
nickolas-pohilets commented Jul 30, 2022
View reviewed changes
@jumhyn-duolingo jumhyn-duolingo mentioned this pull request Jul 31, 2022
Merged
@DougGregor
Copy link
Member

@swift-ci please test

1 similar comment
@DougGregor
Copy link
Member

@swift-ci please test

@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch 6 times, most recently from 5877715 to 47c4b24 Compare August 1, 2022 13:00
@xwu
Copy link
Collaborator

xwu commented Aug 1, 2022

@swift-ci please test

@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 47c4b24 to 4793499 Compare August 2, 2022 10:56
@xwu
Copy link
Collaborator

xwu commented Aug 2, 2022

@swift-ci please test

@stephencelis
Copy link
Contributor

I asked on the review thread, but maybe it'll get more visibility here. Any chance we can get a toolchain to test?

@theblixguy
Copy link
Collaborator

@swift-ci please build toolchain

1 similar comment
@Jumhyn
Copy link
Member

Jumhyn commented Aug 26, 2022

@swift-ci please build toolchain

@nickolas-pohilets nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch 2 times, most recently from 324c20d to dedd1e4 Compare August 26, 2022 19:16
@Jumhyn
Copy link
Member

Jumhyn commented Aug 26, 2022

@swift-ci please build toolchain macOS

@Jumhyn
Copy link
Member

Jumhyn commented Aug 26, 2022

@swift-ci please build toolchain

@DougGregor
Copy link
Member

@swift-ci please build toolchain macOS

@hborla
Copy link
Member

hborla commented Jul 11, 2024

@swift-ci please test

@hborla
Copy link
Member

hborla commented Jul 11, 2024

@swift-ci please build toolchain

@hborla
Copy link
Member

hborla commented Jul 11, 2024

@swift-ci please test

@hborla
Copy link
Member

hborla commented Jul 19, 2024

@swift-ci please build toolchain

ktoso
ktoso approved these changes Jul 24, 2024
View reviewed changes
@ktoso
Copy link
Contributor

ktoso commented Aug 15, 2024

@swift-ci please smoke test

@nickolas-pohilets
Merge branch 'main' into mpokhylets/isolated-deinit
50b1313 
# Conflicts:
#	lib/SILGen/SILGenDistributed.cpp
#	lib/Sema/TypeCheckConcurrency.cpp
nickolas-pohilets
nickolas-pohilets commented Aug 19, 2024
View reviewed changes
@nickolas-pohilets
Added swift_retainCount to EmbeddedRuntime.swift
0412597 
Fixes test/embedded/concurrency-actors.swift
@nickolas-pohilets
Don't add swift_retainCount() to RuntimeFunctions.def, but explicitly…
9a0df5b 
… link it in linkEmbeddedRuntimeFromStdlib()
@nickolas-pohilets
Revert allowing @preconcurrency on deinit declarations
6fa62f7 
Magically fixed
@nickolas-pohilets
Disabled isolated deinit for SWIFT_CONCURRENCY_ACTORS_AS_LOCKS in the…
a7ac32f 
… runtime
@nickolas-pohilets
Implemented zombie actor state in SWIFT_CONCURRENCY_ACTORS_AS_LOCKS m…
d6673c7 
…ode using additional reference counter
@nickolas-pohilets
Merge branch 'main' into mpokhylets/isolated-deinit
e0ad7bd 
# Conflicts:
#	include/swift/Basic/Features.def
#	lib/AST/ASTPrinter.cpp
#	lib/AST/FeatureSet.cpp
@ktoso
Copy link
Contributor

ktoso commented Sep 1, 2024

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Sep 2, 2024

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Sep 3, 2024

@swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Sep 3, 2024

@swift-ci please smoke test

@ktoso ktoso merged commit c86e4a8 into swiftlang:main Sep 3, 2024
3 checks passed
@ahoppen ahoppen mentioned this pull request Sep 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

@ahoppen ahoppen ahoppen left review comments

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@hyp hyp Awaiting requested review from hyp

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@kavon kavon Awaiting requested review from kavon kavon is a code owner

@shahmishal shahmishal Awaiting requested review from shahmishal

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@rintaro rintaro Awaiting requested review from rintaro rintaro is a code owner

@CodaFi CodaFi Awaiting requested review from CodaFi CodaFi is a code owner

@hborla hborla Awaiting requested review from hborla hborla is a code owner

@xedin xedin Awaiting requested review from xedin xedin is a code owner

@slavapestov slavapestov Awaiting requested review from slavapestov slavapestov is a code owner

@adrian-prantl adrian-prantl Awaiting requested review from adrian-prantl

@AnthonyLatsis AnthonyLatsis Awaiting requested review from AnthonyLatsis

@LucianoPAlmeida LucianoPAlmeida Awaiting requested review from LucianoPAlmeida

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

@tshortli tshortli Awaiting requested review from tshortli tshortli is a code owner

@hamishknight hamishknight Awaiting requested review from hamishknight hamishknight is a code owner

@mikeash mikeash Awaiting requested review from mikeash

@al45tair al45tair Awaiting requested review from al45tair

@xymus xymus Awaiting requested review from xymus xymus is a code owner

@jckarter jckarter Awaiting requested review from jckarter jckarter is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

@ian-twilightcoder ian-twilightcoder Awaiting requested review from ian-twilightcoder ian-twilightcoder is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.