From cb7a8d5cfc070f956baff6db20131cd82c437ea1 Mon Sep 17 00:00:00 2001 From: Mykola Pokhylets Date: Sun, 13 Oct 2024 00:38:24 +0200 Subject: [PATCH 1/2] Disable stack promotion for classes with isolated deinit --- .../Sources/AST/Declarations.swift | 8 +++++++- .../FunctionPasses/StackPromotion.swift | 12 +++++++++++ include/swift/AST/ASTBridging.h | 2 ++ include/swift/AST/ASTBridgingImpl.h | 10 ++++++++++ .../async_task_locals_isolated_deinit.swift | 20 ++++++------------- test/Concurrency/voucher_propagation.swift | 18 +++++------------ 6 files changed, 42 insertions(+), 28 deletions(-) diff --git a/SwiftCompilerSources/Sources/AST/Declarations.swift b/SwiftCompilerSources/Sources/AST/Declarations.swift index d7cf6df70378a..234e6bd29720e 100644 --- a/SwiftCompilerSources/Sources/AST/Declarations.swift +++ b/SwiftCompilerSources/Sources/AST/Declarations.swift @@ -57,6 +57,10 @@ final public class StructDecl: NominalTypeDecl { final public class ClassDecl: NominalTypeDecl { public var superClass: Type? { Type(bridgedOrNil: bridged.Class_getSuperclass()) } + + final public var destructor: DestructorDecl { + bridged.Class_getDestructor().getAs(DestructorDecl.self) + } } final public class ProtocolDecl: NominalTypeDecl {} @@ -85,7 +89,9 @@ public class AbstractFunctionDecl: ValueDecl {} final public class ConstructorDecl: AbstractFunctionDecl {} -final public class DestructorDecl: AbstractFunctionDecl {} +final public class DestructorDecl: AbstractFunctionDecl { + final public var isIsolated: Bool { bridged.Destructor_isIsolated() } +} public class FuncDecl: AbstractFunctionDecl {} diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift index 07ab20f832d28..a39b04eeb8efd 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift @@ -10,6 +10,7 @@ // //===----------------------------------------------------------------------===// +import AST import SIL /// Promotes heap allocated objects to the stack. @@ -74,6 +75,17 @@ private func tryPromoteAlloc(_ allocRef: AllocRefInstBase, return false } + if let dtor = (allocRef.type.nominal as? ClassDecl)?.destructor { + if dtor.isIsolated { + // Classes (including actors) with isolated deinit can escape implicitly. + // + // We could optimize this further and allow promotion if we can prove that + // deinit will take fast path (i.e. it will not schedule a job). + // But for now, let's keep things simple and disable promotion conservatively. + return false + } + } + // The most important check: does the object escape the current function? if allocRef.isEscaping(context) { return false diff --git a/include/swift/AST/ASTBridging.h b/include/swift/AST/ASTBridging.h index e3ba7b44ac738..e7842e730815b 100644 --- a/include/swift/AST/ASTBridging.h +++ b/include/swift/AST/ASTBridging.h @@ -313,6 +313,8 @@ struct BridgedDeclObj { SWIFT_IMPORT_UNSAFE BRIDGED_INLINE OptionalBridgedDeclObj NominalType_getValueTypeDestructor() const; BRIDGED_INLINE bool Struct_hasUnreferenceableStorage() const; SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedASTType Class_getSuperclass() const; + SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedDeclObj Class_getDestructor() const; + BRIDGED_INLINE bool Destructor_isIsolated() const; }; struct BridgedASTNode { diff --git a/include/swift/AST/ASTBridgingImpl.h b/include/swift/AST/ASTBridgingImpl.h index a982a4649d9a2..eb222aa3760ce 100644 --- a/include/swift/AST/ASTBridgingImpl.h +++ b/include/swift/AST/ASTBridgingImpl.h @@ -142,6 +142,16 @@ BridgedASTType BridgedDeclObj::Class_getSuperclass() const { return {getAs()->getSuperclass().getPointer()}; } +BridgedDeclObj BridgedDeclObj::Class_getDestructor() const { + return {getAs()->getDestructor()}; +} + +bool BridgedDeclObj::Destructor_isIsolated() const { + auto dd = getAs(); + auto ai = swift::getActorIsolation(dd); + return ai.isActorIsolated(); +} + //===----------------------------------------------------------------------===// // MARK: BridgedASTNode //===----------------------------------------------------------------------===// diff --git a/test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift b/test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift index 8b8d25c8413af..8485bf37ae4fc 100644 --- a/test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift +++ b/test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift @@ -126,14 +126,6 @@ class ClassNoOp: Probe { let tests = TestSuite("Isolated Deinit") -// Dummy global variable to suppress stack propagation -// TODO: Remove it after disabling allocation on stack for classes with isolated deinit -var x: AnyObject? = nil -func preventAllocationOnStack(_ object: AnyObject) { - x = object - x = nil -} - if #available(SwiftStdlib 5.1, *) { tests.test("class sync fast path") { let group = DispatchGroup() @@ -142,7 +134,7 @@ if #available(SwiftStdlib 5.1, *) { // FIXME: isolated deinit should be clearing task locals await TL.$number.withValue(42) { await AnotherActor.shared.performTesting { - preventAllocationOnStack(ClassNoOp(expectedNumber: 0, group: group)) + _ = ClassNoOp(expectedNumber: 0, group: group) } } } @@ -154,7 +146,7 @@ if #available(SwiftStdlib 5.1, *) { group.enter(1) Task { TL.$number.withValue(99) { - preventAllocationOnStack(ClassNoOp(expectedNumber: 0, group: group)) + _ = ClassNoOp(expectedNumber: 0, group: group) } } group.wait() @@ -168,7 +160,7 @@ if #available(SwiftStdlib 5.1, *) { TL.$number.withValue(99) { // Despite last release happening not on the actor itself, // this is still a fast path due to optimisation for deallocating actors. - preventAllocationOnStack(ActorNoOp(expectedNumber: 0, group: group)) + _ = ActorNoOp(expectedNumber: 0, group: group) } } group.wait() @@ -180,7 +172,7 @@ if #available(SwiftStdlib 5.1, *) { Task { TL.$number.withValue(99) { // Using ProxyActor breaks optimization - preventAllocationOnStack(ProxyActor(expectedNumber: 0, group: group)) + _ = ProxyActor(expectedNumber: 0, group: group) } } group.wait() @@ -190,8 +182,8 @@ if #available(SwiftStdlib 5.1, *) { let group = DispatchGroup() group.enter(2) Task { - preventAllocationOnStack(ActorNoOp(expectedNumber: 0, group: group)) - preventAllocationOnStack(ClassNoOp(expectedNumber: 0, group: group)) + _ = ActorNoOp(expectedNumber: 0, group: group) + _ = ClassNoOp(expectedNumber: 0, group: group) } group.wait() } diff --git a/test/Concurrency/voucher_propagation.swift b/test/Concurrency/voucher_propagation.swift index f67623bb51df8..15d455ecd6c2b 100644 --- a/test/Concurrency/voucher_propagation.swift +++ b/test/Concurrency/voucher_propagation.swift @@ -266,14 +266,6 @@ func adopt(voucher: voucher_t?) { os_release(voucher_adopt(os_retain(voucher))) } -// Dummy global variable to suppress stack propagation -// TODO: Remove it after disabling allocation on stack for classes with isolated deinit -var x: AnyObject? = nil -func preventAllocationOnStack(_ object: AnyObject) { - x = object - x = nil -} - let tests = TestSuite("Voucher Propagation") if #available(SwiftStdlib 5.1, *) { @@ -444,15 +436,15 @@ if #available(SwiftStdlib 5.1, *) { Task { await AnotherActor.shared.performTesting { adopt(voucher: v1) - preventAllocationOnStack(ClassWithIsolatedDeinit(expectedVoucher: v1, group: group)) + _ = ClassWithIsolatedDeinit(expectedVoucher: v1, group: group) } await AnotherActor.shared.performTesting { adopt(voucher: v2) - preventAllocationOnStack(ActorWithSelfIsolatedDeinit(expectedVoucher: v2, group: group)) + _ = ActorWithSelfIsolatedDeinit(expectedVoucher: v2, group: group) } await AnotherActor.shared.performTesting { adopt(voucher: v3) - preventAllocationOnStack(ActorWithDeinitIsolatedOnAnother(expectedVoucher: v3, group: group)) + _ = ActorWithDeinitIsolatedOnAnother(expectedVoucher: v3, group: group) } } group.wait() @@ -467,11 +459,11 @@ if #available(SwiftStdlib 5.1, *) { Task { do { adopt(voucher: v1) - preventAllocationOnStack(ActorWithDeinitIsolatedOnAnother(expectedVoucher: v1, group: group)) + _ = ActorWithDeinitIsolatedOnAnother(expectedVoucher: v1, group: group) } do { adopt(voucher: v2) - preventAllocationOnStack(ClassWithIsolatedDeinit(expectedVoucher: v2, group: group)) + _ = ClassWithIsolatedDeinit(expectedVoucher: v2, group: group) } } group.wait() From e201aa325e3c2fa5df95116f1a0fc91c046563a5 Mon Sep 17 00:00:00 2001 From: Mykola Pokhylets Date: Sun, 27 Oct 2024 20:47:29 +0100 Subject: [PATCH 2/2] Added unit tests --- .../stack_promotion_isolated_deinit.swift | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 test/SILOptimizer/stack_promotion_isolated_deinit.swift diff --git a/test/SILOptimizer/stack_promotion_isolated_deinit.swift b/test/SILOptimizer/stack_promotion_isolated_deinit.swift new file mode 100644 index 0000000000000..b2354120aed0e --- /dev/null +++ b/test/SILOptimizer/stack_promotion_isolated_deinit.swift @@ -0,0 +1,71 @@ +// RUN: %target-swift-frontend -parse-as-library -O -module-name=test %s -emit-sil -enable-experimental-feature IsolatedDeinit | %FileCheck %s +// REQUIRES: swift_in_compiler + +@globalActor actor AnotherActor: GlobalActor { + static let shared = AnotherActor() +} + +final class Inner {} + +final class IsolatedDeinit { + var inner: Inner? + @AnotherActor deinit {} +} + +final class Container { + var ref: IsolatedDeinit? +} + +// CHECK-LABEL: sil [noinline] {{.*}}@$s4test0A16ContainerOutsideyyF : $@convention(thin) () -> () { +// CHECK: [[C:%.*]] = alloc_ref [bare] [stack] $Container +// CHECK: [[ID:%.*]] = alloc_ref $IsolatedDeinit +// CHECK: dealloc_stack_ref [[C]] : $Container +// CHECK: return +@inline(never) +public func testContainerOutside() { + // container can be promoted + let container = Container() + let obj = IsolatedDeinit() + container.ref = obj +} + +// CHECK-LABEL: sil [noinline] @$s4test0A15ContainerInsideyyF : $@convention(thin) () -> () { +// CHECK: [[D:%.*]] = alloc_ref $IsolatedDeinit +// CHECK: [[C:%.*]] = alloc_ref [bare] [stack] $Container +// CHECK: dealloc_stack_ref [[C]] : $Container +// CHECK: return +@inline(never) +public func testContainerInside() { + let obj = IsolatedDeinit() + // container can be promoted + let container = Container() + container.ref = obj +} + +// CHECK-LABEL: sil [noinline] @$s4test0A12InnerOutsideyyF : $@convention(thin) () -> () { +// CHECK: [[I:%.*]] = alloc_ref $Inner +// CHECK: [[D:%.*]] = alloc_ref $IsolatedDeinit +// CHECK: [[DI:%.*]] = end_init_let_ref [[D]] : $IsolatedDeinit +// CHECK: strong_release [[DI]] : $IsolatedDeinit +// CHECK: return +@inline(never) +public func testInnerOutside() { + // inner cannot be promoted, because it escapes to isolated deinit + let inner = Inner() + let obj = IsolatedDeinit() + obj.inner = inner +} + +// CHECK-LABEL: sil [noinline] @$s4test0A11InnerInsideyyF : $@convention(thin) () -> () { +// CHECK: [[D:%.*]] = alloc_ref $IsolatedDeinit +// CHECK: [[DI:%.*]] = end_init_let_ref [[D]] : $IsolatedDeinit +// CHECK: [[I:%.*]] = alloc_ref $Inner +// CHECK: strong_release [[DI]] : $IsolatedDeinit +// CHECK: return +@inline(never) +public func testInnerInside() { + let obj = IsolatedDeinit() + // inner cannot be promoted, because it escapes to isolated deinit + let inner = Inner() + obj.inner = inner +}