[Distributed] Implement distributed computed properties via special accessor #59700

xedin · 2022-06-24T23:50:50Z

This is an alternative implementation of distributed computed properties that introduces
a new DistributedThunk accessor kind, new access strategy, and semantics.

A reference to a getter of a distributed computed property is transparently replaced by
its thunk during SILGen (the semantics are the same for both distributed methods and
distributed computed properties).

rdar://91283164

xedin · 2022-06-24T23:52:26Z

@DougGregor and @ktoso this is still a draft but it's functional. I'd probably need to cherry-pick some of the changes from original implementation to make it complete. Please let me know what you think.

ktoso · 2022-06-27T22:00:07Z

include/swift/AST/StorageImpl.h

+
+    /// The access is to a computed distributed property, and thus the
+    /// get-accessor is a distributed thunk which may perform a remote call.
+    DispatchToDistributedThunk,


lib/SILGen/SILGenLValue.cpp

lib/Sema/CodeSynthesisDistributedActor.cpp

ktoso

Looks good to me! Though needs a @DougGregor review as well to see if it looks good now :)

xedin · 2022-06-27T22:34:30Z

@swift-ci please test

xedin · 2022-06-28T05:12:42Z

@swift-ci please test Linux platform

ktoso · 2022-06-29T07:47:56Z

Cherry picked it over: #59774

DougGregor

I'm concerned about the mangling, but the rest is polish.

DougGregor · 2022-06-29T16:36:55Z

include/swift/AST/Attr.def

@@ -735,6 +735,12 @@ CONTEXTUAL_SIMPLE_DECL_ATTR(_local, KnownToBeLocal,
  APIBreakingToAdd | APIBreakingToRemove,
  130)

+SIMPLE_DECL_ATTR(_distributed_thunk, DistributedThunk,


Why is this an attribute rather than a bit on the Decl? It'll never be user-spelled, right?

Yes, cannot be spelled. It could be an attribute on the decl, wasn't sure which one is better.

Switched to use a bit in the declaration and changed serialization accordingly.

DougGregor · 2022-06-29T16:42:51Z

include/swift/AST/TypeCheckRequests.h

+          GetDistributedThunkRequest,
+          FuncDecl *(llvm::PointerUnion<VarDecl *, AbstractFunctionDecl *>),
+          RequestFlags::Cached> {
+  using Originator = llvm::PointerUnion<VarDecl *, AbstractFunctionDecl *>;


I suspect we'll be replacing VarDecl with AbstractStorageDecl to get subscripts at some point :)

Generalized Originator to accept AbstractStorageDecl and AbstractFunctionDecl.

DougGregor · 2022-06-29T16:53:40Z

lib/AST/ASTMangler.cpp

+      appendContextOf(decl);
+      appendDeclName(accessor->getStorage());
+      appendDeclType(accessor, FunctionMangling);
+      appendOperator("F");


Mangling changes need to update docs/ABI/Mangling.rst, the demangler, the remanglers, and add a test (e.g., in test/Demangle/Inputs/manglings.txt) to make sure the whole round-trip works. Also, is F here actually unused? I would have expected that the ACCESSOR production would be extended for this case, so distributed thunks would go through appendAccessorEntity

Distributed thunk accessors are mangled the same way as distributed funcs because runtime needs a function type, if we want to add a new mangling for accessor thunks we'd need to rework a runtime aspects of remote calls. I'm going to add a comment here explaining this.

Added a comment about this.

DougGregor · 2022-06-29T16:58:40Z

lib/Sema/TypeCheckConcurrency.cpp

+        // which are allowed, but subject to the same implicit throws/async
+        // as all other distributed function declarations.
+        if (var->isDistributed()) {
+          return true;


This surprises me, and I don't think we should make this change here. varIsSafeAcrossActors is meant to mean "you can access this without hopping to the actor", which is not the case for distributed properties.

@ktoso I removed the check because as Doug mentioned it doesn't seem correct.

Right that seemed wrong -- thank you!

DougGregor · 2022-06-29T17:07:01Z

lib/Sema/TypeCheckConcurrency.cpp

-          .fixItInsert(decl->getAttributeInsertionLoc(true), "distributed ");
+      // Cannot reference subscripts, or stored properties.
+      auto var = dyn_cast<VarDecl>(decl);
+      if (isa<SubscriptDecl>(decl) || var) {


The rewritten logic here silently accepts declaration kinds it doesn't understand. I'm not sure if I can trigger a bug based on this, but the prior formulation (!func || !func->isDistributed()) was more robust. Can we use more straightforward logic here like:

if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) { handle funcs and return } if (auto var = dyn_cast<VarDecl>(decl)) { handle vars and return } diagnose and return None

… code

…thesize a thunk

`DistributedThunk` is to be used while accessing 'distributed' computed property outside of its actor context.

This strategy is used to dispatch accesses to 'distributed' computed property to distributed thunk accessor instead of a regular getter when access happen outside actor isolation context.

The flag is used to distinguish between regular functions/accessors and synthesized distributed thunks.

… computed properties

…uted thunks Distributed thunks have to refer to the "local" version of the property directly with implicit `self.` base.

…accessors

…Decl` One step towards future distributed subscripts.

Only distributed functions and computed properties should be accepted, everything else is invalid.

xedin · 2022-06-29T21:52:28Z

@swift-ci please test

DougGregor

Looks good, thank you!

xedin · 2022-06-30T00:55:51Z

@swift-ci please test

ktoso · 2022-06-30T00:56:47Z

lib/AST/ASTMangler.cpp

+    // appendOperator("F");
+    // appendSymbolKind(SymbolKind::DistributedThunk);
+    // return finalize();
+    // ```


thanks for the TODO, that's on me after we fixed the SILDeclRefs

Yeah, no worries!

It used to be an accessor but that is not required because SILDeclRef controls mangling which is the most imprortant and could be used to emit the right reference.

xedin · 2022-06-30T01:00:13Z

@swift-ci please test

ktoso · 2022-06-30T01:54:18Z

I'll cherry pick it over to 5.7 👍

xedin · 2022-06-30T03:49:48Z

@swift-ci please test Windows platform

xedin requested review from ktoso and DougGregor June 24, 2022 23:50

ktoso mentioned this pull request Jun 24, 2022

[Distributed] Revert "#59481 distributed-computed-properties" #59699

Merged

ktoso added the distributed Feature → concurrency: distributed actor label Jun 24, 2022

xedin force-pushed the distributed-computed-properties-via-accessor-thunk branch from 97d1f15 to c0cea20 Compare June 27, 2022 20:30

xedin marked this pull request as ready for review June 27, 2022 21:31

xedin force-pushed the distributed-computed-properties-via-accessor-thunk branch from ff3c70f to b9a616d Compare June 27, 2022 21:42

ktoso reviewed Jun 27, 2022

View reviewed changes

lib/SILGen/SILGenLValue.cpp Outdated Show resolved Hide resolved

ktoso reviewed Jun 27, 2022

View reviewed changes

lib/Sema/CodeSynthesisDistributedActor.cpp Outdated Show resolved Hide resolved

xedin force-pushed the distributed-computed-properties-via-accessor-thunk branch from b9a616d to f67b09f Compare June 27, 2022 22:24

ktoso approved these changes Jun 27, 2022

View reviewed changes

ktoso mentioned this pull request Jun 29, 2022

🍒[5.7][Distributed] Implement distributed computed properties via special accessor #59774

Closed

DougGregor reviewed Jun 29, 2022

View reviewed changes

ktoso and others added 12 commits June 29, 2022 14:49

[Distributed] Skeleton implementation of distributed computed properties

febfef9

[Distributed] Remove commented out code and print statements

5bdf94f

fix checkDistributedTargetResultType to also return when not diagnosing

927a58f

[Distributed] NFC: remove a couple of unused variables from sythensis…

c0479a5

… code

[Distributed] Synthesize thunks for distributed computed properties

d68961d

[TypeChecker] Distributed: Verify properties before attempting to syn…

a016fae

…thesize a thunk

[AST] Add new member access semantics - DistributedThunk

fce3b85

`DistributedThunk` is to be used while accessing 'distributed' computed property outside of its actor context.

[Distributed] Add a new access strategy - DispatchToDistributedThunk

e8987b4

This strategy is used to dispatch accesses to 'distributed' computed property to distributed thunk accessor instead of a regular getter when access happen outside actor isolation context.

[Distributed] SILGen: Remove isDistributed flags from accessor code

59ec5ab

[Distributed] Decl: Add a new distributed-thunk bit

b2b1efd

The flag is used to distinguish between regular functions/accessors and synthesized distributed thunks.

[Distributed] Synthesize 'distributed thunk' accessor for distributed…

84fa232

… computed properties

[MiscDiagnostics] Don't diagnose recursive property access in distrib…

13dbd37

…uted thunks Distributed thunks have to refer to the "local" version of the property directly with implicit `self.` base.

xedin added 5 commits June 29, 2022 14:49

[Distributed] Teach HasIsolatedSelfRequest about distributed thunk …

9d5f546

…accessors

[Distributed] NFC: XFAIL a couple of incorrect attr use tests

3006e92

[Distributed] Mangling: Add a comment about distributed accessor thunks

3f3410d

[Distributed] Allow requesting distributed thunks on `AbstractStorage…

8d9962e

…Decl` One step towards future distributed subscripts.

[TypeChecker] Distributed: rework checkDistributedAccess

d9a0130

Only distributed functions and computed properties should be accepted, everything else is invalid.

xedin force-pushed the distributed-computed-properties-via-accessor-thunk branch from f67b09f to d9a0130 Compare June 29, 2022 21:50

DougGregor approved these changes Jun 30, 2022

View reviewed changes

ktoso reviewed Jun 30, 2022

View reviewed changes

[Distributed] Generate thunk for accessor as a regular method

2480c99

It used to be an accessor but that is not required because SILDeclRef controls mangling which is the most imprortant and could be used to emit the right reference.

xedin force-pushed the distributed-computed-properties-via-accessor-thunk branch from ac582b2 to 2480c99 Compare June 30, 2022 00:58

ktoso merged commit ef52542 into swiftlang:main Jun 30, 2022

ktoso mentioned this pull request Jun 30, 2022

🍒[5.7][Distributed] Implement distributed computed properties via special accessor #59809

Merged

[Distributed] Implement distributed computed properties via special accessor #59700

[Distributed] Implement distributed computed properties via special accessor #59700

Uh oh!

Conversation

xedin commented Jun 24, 2022 • edited by ktoso Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Jun 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 27, 2022

Uh oh!

xedin commented Jun 28, 2022

Uh oh!

ktoso commented Jun 29, 2022

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 29, 2022

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 30, 2022

Uh oh!

ktoso commented Jun 30, 2022

Uh oh!

xedin commented Jun 30, 2022

Uh oh!

Uh oh!

xedin commented Jun 24, 2022 •

edited by ktoso

Loading