Skip to content

🍒[5.9][Executors] Clean up how we unwrap the local executor of distributed custom actor #64740

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions include/swift/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,9 @@ class ASTContext final {
/// Get the back-deployed availability for concurrency.
AvailabilityContext getBackDeployedConcurrencyAvailability();

/// The the availability since when distributed actors are able to have custom executors.
AvailabilityContext getConcurrencyDistributedActorWithCustomExecutorAvailability();

/// Get the runtime availability of support for differentiation.
AvailabilityContext getDifferentiationAvailability();

Expand Down Expand Up @@ -934,6 +937,14 @@ class ASTContext final {
/// compiler for the target platform.
AvailabilityContext getSwift57Availability();

/// Get the runtime availability of features introduced in the Swift 5.8
/// compiler for the target platform.
AvailabilityContext getSwift58Availability();

/// Get the runtime availability of features introduced in the Swift 5.9
/// compiler for the target platform.
AvailabilityContext getSwift59Availability();

// Note: Update this function if you add a new getSwiftXYAvailability above.
/// Get the runtime availability for a particular version of Swift (5.0+).
AvailabilityContext
Expand Down
5 changes: 5 additions & 0 deletions include/swift/AST/Availability.h
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,11 @@ class AvailabilityContext {
bool isAvailableAsSPI() const {
return SPI && *SPI;
}

/// Returns a representation of this range as a string for debugging purposes.
std::string getAsString() const {
return "AvailabilityContext(" + OSVersion.getAsString() + (isAvailableAsSPI() ? ", spi" : "") + ")";
}
};


Expand Down
1 change: 1 addition & 0 deletions include/swift/AST/KnownSDKDecls.def
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

KNOWN_SDK_FUNC_DECL(Distributed, IsRemoteDistributedActor, "__isRemoteActor")
KNOWN_SDK_FUNC_DECL(Distributed, IsLocalDistributedActor, "__isLocalActor")
KNOWN_SDK_FUNC_DECL(Distributed, GetUnwrapLocalDistributedActorUnownedExecutor, "_getUnwrapLocalDistributedActorUnownedExecutor")

#undef KNOWN_SDK_FUNC_DECL

28 changes: 28 additions & 0 deletions lib/AST/Availability.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,10 @@ AvailabilityContext ASTContext::getBackDeployedConcurrencyAvailability() {
return getSwift51Availability();
}

AvailabilityContext ASTContext::getConcurrencyDistributedActorWithCustomExecutorAvailability() {
return getSwift59Availability();
}

AvailabilityContext ASTContext::getDifferentiationAvailability() {
return getSwiftFutureAvailability();
}
Expand Down Expand Up @@ -636,6 +640,28 @@ AvailabilityContext ASTContext::getSwift57Availability() {
}
}

AvailabilityContext ASTContext::getSwift58Availability() {
auto target = LangOpts.Target;

if (target.isMacOSX()) {
return AvailabilityContext(
VersionRange::allGTE(llvm::VersionTuple(13, 3, 0)));
} else if (target.isiOS()) {
return AvailabilityContext(
VersionRange::allGTE(llvm::VersionTuple(16, 4, 0)));
} else if (target.isWatchOS()) {
return AvailabilityContext(
VersionRange::allGTE(llvm::VersionTuple(9, 4, 0)));
} else {
return AvailabilityContext::alwaysAvailable();
}
}

AvailabilityContext ASTContext::getSwift59Availability() {
// TODO: Update Availability impl when Swift 5.9 is released
return getSwiftFutureAvailability();
}

AvailabilityContext ASTContext::getSwiftFutureAvailability() {
auto target = LangOpts.Target;

Expand Down Expand Up @@ -665,6 +691,8 @@ ASTContext::getSwift5PlusAvailability(llvm::VersionTuple swiftVersion) {
case 5: return getSwift55Availability();
case 6: return getSwift56Availability();
case 7: return getSwift57Availability();
case 8: return getSwift58Availability();
case 9: return getSwift59Availability();
default: break;
}
}
Expand Down
61 changes: 29 additions & 32 deletions lib/SILOptimizer/Mandatory/LowerHopToActor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "swift/SIL/SILFunction.h"
#include "swift/SIL/Dominance.h"
#include "swift/SILOptimizer/Analysis/DominanceAnalysis.h"
#include "swift/SILOptimizer/Utils/SILOptFunctionBuilder.h"
#include "swift/SILOptimizer/PassManager/Transforms.h"
#include "llvm/ADT/ScopedHashTable.h"

Expand Down Expand Up @@ -49,19 +50,26 @@ namespace {
class LowerHopToActor {
SILFunction *F;
DominanceInfo *Dominance;
SILOptFunctionBuilder &functionBuilder;

/// A map from an actor value to the executor we've derived for it.
llvm::ScopedHashTable<SILValue, SILValue> ExecutorForActor;

bool processHop(HopToExecutorInst *hop);
bool processExtract(ExtractExecutorInst *extract);

SILValue emitGetExecutor(SILBuilderWithScope &B, SILLocation loc,
SILValue emitGetExecutor(SILBuilderWithScope &B,
SILLocation loc,
SILValue actor, bool makeOptional);

public:
LowerHopToActor(SILFunction *f, DominanceInfo *dominance)
: F(f), Dominance(dominance) { }
LowerHopToActor(SILFunction *f,
SILOptFunctionBuilder &FunctionBuilder,
DominanceInfo *dominance)
: F(f),
Dominance(dominance),
functionBuilder(FunctionBuilder)
{ }

/// The entry point to the transformation.
bool run();
Expand Down Expand Up @@ -154,28 +162,6 @@ static AccessorDecl *getUnownedExecutorGetter(ASTContext &ctx,
return nullptr;
}

static AccessorDecl *getUnwrapLocalUnownedExecutorGetter(ASTContext &ctx,
ProtocolDecl *actorProtocol) {
for (auto member: actorProtocol->getAllMembers()) { // FIXME: remove this, just go to the extension
if (auto var = dyn_cast<VarDecl>(member)) {
if (var->getName() == ctx.Id__unwrapLocalUnownedExecutor)
return var->getAccessor(AccessorKind::Get);
}
}

for (auto extension: actorProtocol->getExtensions()) {
for (auto member: extension->getAllMembers()) {
if (auto var = dyn_cast<VarDecl>(member)) {
if (var->getName() == ctx.Id__unwrapLocalUnownedExecutor) {
return var->getAccessor(AccessorKind::Get);
}
}
}
}

return nullptr;
}

SILValue LowerHopToActor::emitGetExecutor(SILBuilderWithScope &B,
SILLocation loc, SILValue actor,
bool makeOptional) {
Expand All @@ -199,6 +185,9 @@ SILValue LowerHopToActor::emitGetExecutor(SILBuilderWithScope &B,
// If the actor type is a default actor, go ahead and devirtualize here.
auto module = F->getModule().getSwiftModule();
SILValue unmarkedExecutor;

// Determine if the actor is a "default actor" in which case we'll build a default
// actor executor ref inline, rather than calling out to the user-provided executor function.
if (isDefaultActorType(actorType, module, F->getResilienceExpansion())) {
auto builtinName = ctx.getIdentifier(
getBuiltinName(BuiltinValueKind::BuildDefaultActorExecutorRef));
Expand All @@ -212,7 +201,7 @@ SILValue LowerHopToActor::emitGetExecutor(SILBuilderWithScope &B,
} else if (actorType->isDistributedActor()) {
auto actorKind = KnownProtocolKind::DistributedActor;
auto actorProtocol = ctx.getProtocol(actorKind);
auto req = getUnwrapLocalUnownedExecutorGetter(ctx, actorProtocol);
auto req = ctx.getGetUnwrapLocalDistributedActorUnownedExecutor();
assert(req && "Distributed library broken");
SILDeclRef fn(req, SILDeclRef::Kind::Func);

Expand All @@ -222,12 +211,19 @@ SILValue LowerHopToActor::emitGetExecutor(SILBuilderWithScope &B,

auto subs = SubstitutionMap::get(req->getGenericSignature(),
{actorType}, {actorConf});
auto fnType = F->getModule().Types.getConstantFunctionType(*F, fn);

auto witness =
B.createWitnessMethod(loc, actorType, actorConf, fn,
SILType::getPrimitiveObjectType(fnType));
auto witnessCall = B.createApply(loc, witness, subs, {actor});
// Find the unwrap function
FuncDecl *funcDecl = ctx.getGetUnwrapLocalDistributedActorUnownedExecutor();
assert(funcDecl);
Copy link
Member

Choose a reason for hiding this comment

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

You are likely to end up having this code run against a standard library that doesn't have this function declaration, because it's older. Can we fail gracefully in such cases?

More urgently, doesn't the use of this new function break back-deployment for distributed actors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this and I believe we should address this a little bit, the proposed solution is here: #64800

It specifically is about resilient "old" distributed actors being potentially marked as non default while they should still be default

Copy link
Contributor Author

@ktoso ktoso Apr 3, 2023

Choose a reason for hiding this comment

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

Done in #64800 and cherry picked into this PR as well @DougGregor

auto funcDeclRef = SILDeclRef(funcDecl, SILDeclRef::Kind::Func);

SILFunction *unwrapExecutorFun =
functionBuilder.getOrCreateFunction(
loc, funcDeclRef, ForDefinition_t::NotForDefinition);
assert(unwrapExecutorFun && "no sil function!");
auto funcRef =
B.createFunctionRef(loc, unwrapExecutorFun);
auto witnessCall = B.createApply(loc, funcRef, subs, {actor});

// The protocol requirement returns an Optional<UnownedSerialExecutor>;
// extract the Builtin.Executor from it.
Expand Down Expand Up @@ -286,7 +282,8 @@ class LowerHopToActorPass : public SILFunctionTransform {
void run() override {
auto fn = getFunction();
auto domTree = getAnalysis<DominanceAnalysis>()->get(fn);
LowerHopToActor pass(getFunction(), domTree);
auto functionBuilder = SILOptFunctionBuilder(*this);
LowerHopToActor pass(getFunction(), functionBuilder, domTree);
if (pass.run())
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
}
Expand Down
21 changes: 21 additions & 0 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,27 @@ bool IsDefaultActorRequest::evaluate(
if (!classDecl->isActor())
return false;

// Distributed actors were not able to have custom executors until Swift 5.9,
// so in order to avoid wrongly treating a resilient distributed actor from another
// module as not-default we need to handle this case explicitly.
if (classDecl->isDistributedActor()) {
ASTContext &ctx = classDecl->getASTContext();
auto customExecutorAvailability =
ctx.getConcurrencyDistributedActorWithCustomExecutorAvailability();

auto actorAvailability = TypeChecker::overApproximateAvailabilityAtLocation(
classDecl->getStartLoc(),
classDecl);

if (!actorAvailability.isContainedIn(customExecutorAvailability)) {
// Any 'distributed actor' declared with availability lower than the
// introduction of custom executors for distributed actors, must be treated as default actor,
// even if it were to declared the unowned executor property, as older compilers
// do not have the the logic to handle that case.
return true;
Comment on lines +188 to +202
Copy link
Member

Choose a reason for hiding this comment

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

This looks great, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! And thank you for noticing the issue to begin with!

}
}

// If the class is resilient from the perspective of the module
// module, it's not a default actor.
if (classDecl->isForeign() || classDecl->isResilient(M, expansion))
Expand Down
14 changes: 5 additions & 9 deletions stdlib/public/Distributed/DistributedActor.swift
Original file line number Diff line number Diff line change
Expand Up @@ -278,19 +278,15 @@ public protocol DistributedActor: AnyActor, Identifiable, Hashable
/// - Parameter system: `system` which should be used to resolve the `identity`, and be associated with the returned actor
static func resolve(id: ID, using system: ActorSystem) throws -> Self

// FIXME: figure out how to remove this so LowerHopToActor can call the extension method directly on the protocol
@available(SwiftStdlib 5.9, *)
var _unwrapLocalUnownedExecutor: UnownedSerialExecutor { get }
}


@available(SwiftStdlib 5.9, *)
extension DistributedActor {

@available(SwiftStdlib 5.9, *)
public var _unwrapLocalUnownedExecutor: UnownedSerialExecutor {
self.localUnownedExecutor!
public func _getUnwrapLocalDistributedActorUnownedExecutor(_ actor: some DistributedActor) -> UnownedSerialExecutor {
guard let executor = actor.localUnownedExecutor else {
fatalError("Expected distributed actor executor to be not nil!")
}

return executor
}

// ==== Hashable conformance ---------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func check(actor: MainDistributedFriend) {
checkAssumeMainActor(actor: actor)
}

@available(SwiftStdlib 5.9, *)
distributed actor MainDistributedFriend {
nonisolated var localUnownedExecutor: UnownedSerialExecutor? {
print("get unowned executor")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend-emit-module -emit-module-path %t/FakeDistributedActorSystems.swiftmodule -module-name FakeDistributedActorSystems -disable-availability-checking %S/../Inputs/FakeDistributedActorSystems.swift
// RUN: %target-build-swift -Xfrontend -disable-availability-checking -parse-as-library -I %t %s %S/../Inputs/FakeDistributedActorSystems.swift -o %t/a.out
// RUN: %target-codesign %t/a.out
// RUN: %target-run %t/a.out

// REQUIRES: executable_test
// REQUIRES: concurrency
// REQUIRES: distributed
// REQUIRES: concurrency_runtime
// UNSUPPORTED: back_deployment_runtime

// UNSUPPORTED: back_deploy_concurrency
// UNSUPPORTED: use_os_stdlib
// UNSUPPORTED: freestanding

import StdlibUnittest
import Distributed
import FakeDistributedActorSystems

@available(SwiftStdlib 5.7, *)
typealias DefaultDistributedActorSystem = LocalTestingDistributedActorSystem

@available(SwiftStdlib 5.7, *)
distributed actor FiveSevenActor_NothingExecutor {
nonisolated var localUnownedExecutor: UnownedSerialExecutor? {
print("get unowned executor")
return MainActor.sharedUnownedExecutor
}

distributed func test(x: Int) async throws {
print("executed: \(#function)")
defer {
print("done executed: \(#function)")
}
assumeOnMainActorExecutor {
// ignore
}
}
}

@available(SwiftStdlib 5.9, *)
distributed actor FiveNineActor_NothingExecutor {
// @available(SwiftStdlib 5.9, *) // because of `localUnownedExecutor`
nonisolated var localUnownedExecutor: UnownedSerialExecutor? {
print("get unowned executor")
return MainActor.sharedUnownedExecutor
}

distributed func test(x: Int) async throws {
print("executed: \(#function)")
defer {
print("done executed: \(#function)")
}
assumeOnMainActorExecutor {
// ignore
}
}
}

@available(SwiftStdlib 5.7, *)
distributed actor FiveSevenActor_FiveNineExecutor {
@available(SwiftStdlib 5.9, *)
nonisolated var localUnownedExecutor: UnownedSerialExecutor? {
print("get unowned executor")
return MainActor.sharedUnownedExecutor
}

distributed func test(x: Int) async throws {
print("executed: \(#function)")
defer {
print("done executed: \(#function)")
}
assumeOnMainActorExecutor {
// ignore
}
}
}

@main struct Main {
static func main() async {
if #available(SwiftStdlib 5.9, *) {
let tests = TestSuite("DistributedActorExecutorAvailability")

let system = LocalTestingDistributedActorSystem()

#if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
tests.test("5.7 actor, no availability executor property => no custom executor") {
expectCrashLater(withMessage: "Fatal error: Incorrect actor executor assumption; Expected 'MainActor' executor.")
try! await FiveSevenActor_NothingExecutor(actorSystem: system).test(x: 42)
}

tests.test("5.9 actor, no availability executor property => custom executor") {
try! await FiveNineActor_NothingExecutor(actorSystem: system).test(x: 42)
}

tests.test("5.7 actor, 5.9 executor property => no custom executor") {
expectCrashLater(withMessage: "Fatal error: Incorrect actor executor assumption; Expected 'MainActor' executor.")
try! await FiveSevenActor_FiveNineExecutor(actorSystem: system).test(x: 42)
}
#else
// On non-apple platforms the SDK comes with the toolchains,
// so the feature works because we're executing in a 5.9 context already,
// which otherwise could not have been compiled
tests.test("non apple platform: 5.7 actor, no availability executor property => no custom executor") {
try! await FiveSevenActor_NothingExecutor(actorSystem: system).test(x: 42)
}

tests.test("non apple platform: 5.9 actor, no availability executor property => custom executor") {
try! await FiveNineActor_NothingExecutor(actorSystem: system).test(x: 42)
}

tests.test("non apple platform: 5.7 actor, 5.9 executor property => no custom executor") {
try! await FiveSevenActor_FiveNineExecutor(actorSystem: system).test(x: 42)
}
#endif

await runAllTestsAsync()
}
}
}
Loading