[Distributed] dist. isolation checking aware of Task and closures etc #39521
Conversation
ktoso
requested review from
DougGregor and
kavon
and removed request for
DougGregor
|
@swift-ci please smoke test |
|
@swift-ci please build toolchain macOS platform |
ktoso
force-pushed
the
wip-tasks-distribution-isolation
branch
5 times, most recently
from
d82bae9 to
3e35829
Compare
ktoso
force-pushed
the
wip-tasks-distribution-isolation
branch
from
3e35829 to
e133dad
Compare
|
@swift-ci please smoke test and merge |
|
@swift-ci please build toolchain macOS |
|
|
||
| // distributed func reference, that passes all checks, great! | ||
| continueToCheckingLocalIsolation = true; | ||
| tryMarkImplicitlyAsync(memberLoc, memberRef, context, |
There was a problem hiding this comment.
I think our refactoring goal here should be to unify tryMarkImplicitlyAsync and tryMarkImplicitlyThrows, and give it enough information to determine when to add the qualifiers. For example, if isPotentiallyIsolated is true for a function on a distributed actor, we don't need the function to be distributed and should not mark it throws, but we might need to mark it async.
There was a problem hiding this comment.
Yeah that sounds right... they are applied in similar situations, just need to feed it enough flags.
Noted!
| // if it is explicitly marked distributed actor independent, | ||
| // it is synchronously accessible, no implicit async needed. | ||
| if (decl->getAttrs().hasAttribute<DistributedActorIndependentAttr>() || | ||
| decl->getAttrs().hasAttribute<NonisolatedAttr>()) { |
There was a problem hiding this comment.
I still suspect we can remove DistributedActorIndependentAttr and settle for implicitly-introduced NonisolatedAttr, but we can tackle that elsewhere.
There was a problem hiding this comment.
yup! DistributedActorIndependentAttr is a bit of debt from the early impls -- nowadays nonisolated is enough.
I'll make a specific radar to follow up on the removal
|
|
||
| LLVM_FALLTHROUGH; | ||
| return diagnoseNonSendableTypesInReference( |
There was a problem hiding this comment.
This brings an interesting question to mind. If we're going through the distributed entry point, do we even need Sendable checking? Or is that only needed in the local case?
There was a problem hiding this comment.
I think it still matters -- the transport may use a different thread to serialize things (one of my impls does so), so the thread safety of the payloads is still useful
There was a problem hiding this comment.
Even through I guess the only thing it does to them is invoke encode... 🤔
Previously distributed isolation rules were too aggressive, or rather just not right, when facing closures and for example
Task {}andTask.detached {}.This PR fixes the simple mistakes, and also makes use of local knowlage to skip adding throwing behavior when we KNOW it cannot throw because it actually must have been a local reference. This is the case for
isolateddistributed actor references as well as calls onselfbut from OUTSIDE the actor's context, like for example inTask.detached {}.This also converges CrossDistributedActorSelf with CrossActorSelf, but we'll finish that convergence in rdar://83713366
Resolves rdar://83609197