[Distributed] Implement whenLocal properly #59598

ktoso · 2022-06-21T07:19:09Z

Resolves rdar://89082275

This aims to properly implement whenLocal so that it works properly in the type system, not by side stepping it. Almost there but need to consult a few pieces.

Replaces #42098

include/swift/AST/Decl.h

test/Distributed/Runtime/distributed_actor_whenLocal.swift

include/swift/AST/Decl.h

DougGregor · 2022-06-22T06:24:41Z

include/swift/AST/Decl.h

  };

  /// The default value, if any, along with flags.
  llvm::PointerIntPair<StoredDefaultArgument *, 3, OptionSet<Flags>>
      DefaultValueAndFlags;

+  // FIXME: can we pack this somewhere as flags, rather than ad hoc bool?
+  bool isDistributedKnownLocal = false;


You already have it packed in the flags via IsDistributedKnownToBeLocal, so you can remove this.

Ok cool, I was very iffy about how I packed it into the flags but seems that's fine -- removing the bool then, thanks!

DougGregor · 2022-06-22T06:25:27Z

include/swift/AST/Decl.h

+  /// Whether or not this parameter is marked with 'isolated'.
+  bool isDistributedKnownToBeLocal() const {
+    auto yes = getAttrs().hasAttribute<DistributedKnownToBeLocalAttr>() ||
+        ArgumentNameAndFlags.getInt().contains(ArgumentNameFlags::IsDistributedKnownToBeLocal);


I think you just want to use ArgumentNameAndFlags.getInt().contains(ArgumentNameFlags::IsDistributedKnownToBeLocal), and set it elsewhere based on the attribute. Let isolated be your guide here.

Done and cleaned up!

lib/Sema/TypeCheckType.cpp

DougGregor · 2022-06-22T06:43:41Z

lib/Sema/TypeCheckType.cpp

+  // TODO(distributed): more diagnosis here, prevent from use in props, enums etc
+
+  // isolated parameters must be of 'distributed actor' type
+  if (!type->hasTypeParameter() &&


Presumably it's okay for it to have a type parameter, but then the archetype would have to be a distributed actor (i.e., conform to the DistributedActor protocol).

Good point, I think that's right... will think about it and try

Actually, moving this logic into TypeResolver, as I suggested above, means that it'll be checked on the archetype automatically for you, rendering my comment above uninteresting.

Ok awesome :)

lib/Serialization/ModuleFormat.h

DougGregor · 2022-06-22T06:45:17Z

stdlib/public/Distributed/DistributedActor.swift

@@ -140,8 +140,18 @@ extension DistributedActor {
  /// state.
  ///
  /// When the actor is remote, the closure won't be executed and this function will return nil.
+//  public nonisolated func whenLocal<T: Sendable>(


I assume you want to remove this?

I have to figure out how to introduce the "good ones" without breaking existing code; this one has unfortunate semantics that it does "await x.whenLocal { isolated X in .... }whereas the other one this_localenables allows us to not hop unless we have to:x.whenLocal { _local X in xx.someConstant }` so there's no hop and no async there...

We also want to allow await x.whenLocal { _local X in await xx.something } though; so we can either do this by keeping the old version which does the HOP once (could be good?), or making some new overload.

So in other words... perhaps keeping the "old" signature is good because:

won't break API/ABI

is could be okey...? we do one hop when the function is async and run on the actor...?

If we could get away with removing this function though I think we would want to -- can we though?

Open question what to do here, the "I'd love those" signatures are:

public nonisolated func whenLocal<T: Sendable>( // FIXME: do we need sendable req here? _ body: @Sendable (_local Self) async throws -> T ) async rethrows -> T? { if __isLocalActor(self) { return try await body(self) } else { return nil } } public nonisolated func whenLocal<T: Sendable>( _ body: (_local Self) throws -> T ) rethrows -> T? { if __isLocalActor(self) { return try body(self) } else { return nil } }

so we can avoid the "one hop" at the beginning, but I guess we COULD keep the isolated one as well hmm

These signatures look fine to me, but I don't think you need the Sendable constraints because you're never actually crossing the actor boundary. However, you do need to decide to either have the _local versions, or the isolated versions, but if you want both you're going to need to give them different names.

test/Distributed/Runtime/distributed_actor_whenLocal.swift

xedin · 2022-06-22T16:21:57Z

lib/Sema/CSSimplify.cpp

+          distributedKnownLocalParams.insert(paramDecl);
+        }
+        if (contextualParam->isDistributedKnownToBeLocal() &&
+            !flags.isDistributedKnownToBeLocal() && paramDecl) {


Nit: It’s not necessary to check paramDecl here since there is a access on it earlier which would crash if it was nullptr.

ktoso · 2022-06-23T14:13:48Z

I think I managed to implement this for methods -- properties are hard... I'll need to consult a bit with you @DougGregor but they can also come later I think.

ktoso · 2022-06-23T14:48:06Z

@swift-ci please smoke test

DougGregor · 2022-06-23T17:06:11Z

FYI, Windows is telling us there's a real issue here:

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift\lib\AST\Expr.cpp(282) : error C4716: 'swift::Expr::isDistributedKnownToBeLocal': must return a value
[401/1275] Building CXX object lib\AST\CMakeFiles\swiftAST.dir\RequirementMachine\ConcreteContraction.cpp.obj

I don't think we need Expr::isDistributedKnownToBeLocal at all, right?

DougGregor · 2022-06-23T17:07:55Z

include/swift/AST/Decl.h

@@ -5906,6 +5900,10 @@ class ParamDecl : public VarDecl {

  void setDistributedKnownToBeLocal(bool value = true) {
    auto flags = ArgumentNameAndFlags.getInt();
+    if (value && !getAttrs().hasAttribute<DistributedKnownToBeLocalAttr>()) {
+      getAttrs().add(new (getASTContext())


You shouldn't need to add an attribute here, and please don't because it's an additional unnecessary mutation on the AST. Nobody should be looking at the attribute except code that validates it when present, and sets this flag when needed.

Right, thank you!

DougGregor · 2022-06-23T17:09:55Z

lib/Sema/CSSimplify.cpp

          isolatedParams.insert(paramDecl);

+          // an 'isolated' param is also implicitly '_local',
+          // since in order to run "on" an actor, it must be a real local instance.
+          distributedKnownLocalParams.insert(paramDecl);


Where do we check distributedKnownLocalParams to determine whether it's known-to-be-local? Those places should check both the isolatedParams and distributedKnownLocalParams to answer the question.

DougGregor · 2022-06-23T17:11:51Z

lib/Sema/TypeCheckAttr.cpp

-  if (!D->isImplicit()) {
+  if (D->isImplicit()) {
+    // Implicit code is fine, this is how we use '_local' in synthesized thunks.
+    return;


Implicit code should always follow the rules below, right? And if they don't, it's a bug in either the generation of synthesized thunks or in the check below. I recommend removing this early return. You could add an assert(!D->isImplicit()); along the error paths below if you want to get earlier warning when the synthesis and the check are out of sync.

Hmm... I think that means we'll have to change synthesis of thunks to call whenLocal, because the thunks do "if local ... call the real func" (without using the whenLocal -- at least today), and they do so from nonisolated func -- so we'll have to change thunk synthesis to use whenLocal to get the _local self in order to do this 🤔

That probably makes sense though, so thunks should become to use the normal function from here I suppose 🤔

lib/Sema/TypeCheckAttr.cpp

DougGregor · 2022-06-23T17:13:47Z

lib/Sema/TypeCheckAttr.cpp

+  // param, since we're not able to safely carry it around and propagate the
+  // "local-ness" just yet.
+  // TODO(distributed): unlock this once '_local' is also implemented in properties
+  if (auto closureExpr = dyn_cast<AbstractClosureExpr>(D->getDeclContext())) {


I don't understand this restriction. Why should _local not be permitted anywhere that isolated is?

Per my comment about this attribute probably not being needed, this validation could occur in TypeResolver, like TypeResolver::resolveIsolatedTypeRepr does for isolated.

Why should _local not be permitted anywhere that isolated is?

Just because I don't think the assignments are handled properly, but maybe it'll just work when implemented in TypeResolver::resolveIsolatedTypeRepr hmmm

DougGregor · 2022-06-23T17:28:19Z

lib/Parse/ParsePattern.cpp

              param->setIsolated(true);
+            } else if (isa<DistributedKnownToBeLocalTypeRepr>(STR)) {
+              param->setDistributedKnownToBeLocal(true);
+              auto localAttr = new (parser.Context)


You know, part of me wonders why we even have DistributedKnownToBeLocalAttr anymore. We have a bit on ParamDecl that provides this information. We have the source location in DistributedKnownToBeLocalTypeRepr that we can use for diagnostics.

Yeah that may be true -- perhaps it is indeed much more like isolated than the const attribute I based a lot of this work on... I'll try if we can get rid of the attr

lib/Sema/TypeCheckConcurrency.h

DougGregor · 2022-06-23T17:47:49Z

lib/Sema/TypeCheckEffects.cpp

        Ctx.Diags.diagnose(E->getAwaitLoc(), diag::no_async_in_await);
-      else
+        E->dump();


Extraneous debug dumps.

DougGregor · 2022-06-23T17:49:37Z

lib/Sema/TypeCheckType.cpp

+  // TODO(distributed): more diagnosis here, prevent from use in props, enums etc
+
+  // isolated parameters must be of 'distributed actor' type
+  if (!type->hasTypeParameter() &&


Actually, moving this logic into TypeResolver, as I suggested above, means that it'll be checked on the archetype automatically for you, rendering my comment above uninteresting.

DougGregor · 2022-06-23T20:04:48Z

stdlib/public/Distributed/DistributedActor.swift

  ///
  /// When the actor is remote, the closure won't be executed and this function will return nil.
-  public nonisolated func whenLocal<T: Sendable>(
-    _ body: @Sendable (isolated Self) async throws -> T
+  public nonisolated func whenLocal<T: Sendable>( // FIXME: do we need sendable req here?


As noted elsewhere, I don't think we need a Sendable requirements here.

Should these be @_alwaysEmitIntoClient?

Probably good idea, these impls will never change 👍

ktoso · 2022-06-23T23:08:58Z

I don't think we need Expr::isDistributedKnownToBeLocal at all, right?

Yeah I think you're right, we do it in one centralized spot anyway, will remove this.

ktoso · 2022-06-25T01:56:22Z

This will need to be redone since we're changing back how thunks work.

ktoso mentioned this pull request Jun 21, 2022

[WIP][Distributed] Implement _local, towards removing __secretlyKnownToBeLocal #42098

Closed

ktoso commented Jun 21, 2022

View reviewed changes

include/swift/AST/Decl.h Show resolved Hide resolved

ktoso commented Jun 21, 2022

View reviewed changes

test/Distributed/Runtime/distributed_actor_whenLocal.swift Outdated Show resolved Hide resolved

ktoso mentioned this pull request Jun 21, 2022

Convert SWIMActorShell to distributed actor apple/swift-distributed-actors#977

Merged

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

DougGregor reviewed Jun 22, 2022

View reviewed changes

ktoso force-pushed the wip-revive-local-attr branch from 9527413 to 8af1975 Compare June 22, 2022 12:11

xedin reviewed Jun 22, 2022

View reviewed changes

ktoso added 5 commits June 23, 2022 23:09

revive local attr for whenLocal

f81e2f1

initial _local impl

98bf270

carry the info into the solver

f87ded4

everything except properties

f6a2bed

remove println debugging

8c368c5

ktoso force-pushed the wip-revive-local-attr branch from 8af1975 to 8c368c5 Compare June 23, 2022 14:11

DougGregor reviewed Jun 23, 2022

View reviewed changes

ktoso marked this pull request as draft June 25, 2022 01:56

ktoso changed the title ~~[Distributed] Implement whenLocal properly~~ [Needs rewrite now][Distributed] Implement whenLocal properly Jun 26, 2022

ktoso changed the title ~~[Needs rewrite now][Distributed] Implement whenLocal properly~~ [Distributed] Implement whenLocal properly Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Distributed] Implement whenLocal properly #59598

[Distributed] Implement whenLocal properly #59598

ktoso commented Jun 21, 2022

DougGregor Jun 22, 2022

ktoso Jun 22, 2022

DougGregor Jun 22, 2022

ktoso Jun 23, 2022

DougGregor Jun 22, 2022

ktoso Jun 22, 2022

DougGregor Jun 23, 2022

ktoso Jun 23, 2022

DougGregor Jun 22, 2022

ktoso Jun 22, 2022

ktoso Jun 23, 2022

DougGregor Jun 23, 2022 •

edited

Loading

xedin Jun 22, 2022

ktoso commented Jun 23, 2022

ktoso commented Jun 23, 2022

DougGregor commented Jun 23, 2022

DougGregor Jun 23, 2022

ktoso Jun 23, 2022

DougGregor Jun 23, 2022

DougGregor Jun 23, 2022

ktoso Jun 23, 2022

DougGregor Jun 23, 2022

DougGregor Jun 23, 2022

ktoso Jun 23, 2022 •

edited

Loading

DougGregor Jun 23, 2022

ktoso Jun 24, 2022

DougGregor Jun 23, 2022

DougGregor Jun 23, 2022

DougGregor Jun 23, 2022

DougGregor Jun 23, 2022

ktoso Jun 23, 2022

ktoso commented Jun 23, 2022

ktoso commented Jun 25, 2022

[Distributed] Implement whenLocal properly #59598

Are you sure you want to change the base?

[Distributed] Implement whenLocal properly #59598

Conversation

ktoso commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Jun 23, 2022

ktoso commented Jun 23, 2022

DougGregor commented Jun 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Jun 23, 2022

ktoso commented Jun 25, 2022

DougGregor Jun 23, 2022 •

edited

Loading

ktoso Jun 23, 2022 •

edited

Loading