Isolated synchronous deinit #60057

nickolas-pohilets · 2022-07-14T12:30:12Z

Implementation of the Isolated asynchronous deinit

~~Still pending:~~

~~I suspect that logic for adding attributes for inferred isolation is not correct, would like some input from the Core team about this.~~
~~Still need to integrate with distributed actors, waiting for [Distributed] prevent remote distributed actor's from running deinit bodies #60050 to be merged.~~
Didn't copy changes to BackDeployConcurrency yet. I expect that I'll have to make changes based on review comments, and want to apply all changes to BackDeployConcurrency in one go. I would appreciate some advice for maintaining the two in sync.
~~Investigate fast path for self-isolated default actor's deinit~~

DougGregor

This is fantastic! A couple of comments and questions, both in design and implementation, but it's very much going in the right direction.

include/swift/AST/Decl.h

DougGregor · 2022-07-15T22:45:47Z

include/swift/AST/PrintOptions.h

@@ -606,7 +606,8 @@ struct PrintOptions {
    result.SkipSwiftPrivateClangDecls = true;
    result.SkipPrivateStdlibDecls = true;
    result.SkipUnderscoredStdlibProtocols = true;
-    result.SkipDeinit = true;
+    result.SkipDeinit = false; // Deinit may have isolation attributes, which


This will make interfaces a little more noisy in the common case, but that's okay. The alternative would be to print only when there is something interesting (i.e., a nonisolated or custom attribute).

lib/ClangImporter/ImportDecl.cpp

lib/SILGen/SILGenDestructor.cpp

lib/Sema/TypeCheckConcurrency.cpp

stdlib/public/Concurrency/MainActor.swift

DougGregor · 2022-07-15T23:23:00Z

stdlib/public/Concurrency/Task.cpp

+  AdHocJob(JobPriority priority, void *context, AdHocWorkFunction *work)
+      : Job({JobKind::AdHoc, priority}, &process), Context(context),
+        Work(work) {
+    TaskLocal::copyTo(&Local, nullptr);


There's an interesting design question in here. Do we want the task locals from the task that did the last "release" to be available to the deinitializer? It does match today's semantics, but it could be expensive to copy all of the task locals, which are not likely to be used by many/most deinitializers.

For GAIT, I expect that vast majority of releases to happen within isolating actor, so I'm not very concerned about performance costs of the slow path. But for actors, that's probably not true. Maybe what you mentioned below can help to reduce switching for actors.

I would not depend on having task locals available in a deinit, and especially since we don't guarantee WHICH exact locals those will be, so basing any tracing infra on "object got deallocated and must have right locals" is going to be wrong at some point so not usable "for real" I think...

I'd say losing the locals is expected. For anything that needs things at "end of scope" (like tracing) the usual pattern is to wrap in a closure, not really rely on deinit.

I think some task-locals can still be useful in the deinit even if you are not sure which task/thread performs the last release. For example, if you have a logger applied to entire app or a large subsystem, you can expect that it will be available in any task/thread launched within that scope. So it does not matter which one makes the last release, as long you are confident that last release happens within this coarse-grained scope, you can still meaningfully use logger's task local in the deinit.

test/Concurrency/deinit_isolation.swift

lib/SILGen/SILGenDestructor.cpp

DougGregor · 2022-07-16T00:27:13Z

Didn't copy changes to BackDeployConcurrency yet. I expect that I'll have to make changes based on review comments, and want to apply all changes to BackDeployConcurrency in one go. I would appreciate some advice for maintaining the two in sync.

BackDeployConcurrency is actually supposed to be frozen in time, and not updated with new features. For back-deployment of this feature, we'll have to bring up another statically-linked back-deployment library that would be linked into all problems that build with a deployment target that predates this work. @etcwilde is starting to bring up such a library to back-deploy other concurrency-related fixes. Once that's in place, this PR can build on that work.

nickolas-pohilets · 2022-07-18T11:30:14Z

BackDeployConcurrency is actually supposed to be frozen in time, and not updated with new features. For back-deployment of this feature, we'll have to bring up another statically-linked back-deployment library that would be linked into all problems that build with a deployment target that predates this work. @etcwilde is starting to bring up such a library to back-deploy other concurrency-related fixes. Once that's in place, this PR can build on that work.

I would really appreciate a document about how to work with BackDeployConcurrency. Could not find anything in the repo.

ktoso · 2022-07-20T03:15:49Z

AFAICS the distributed adjustment looks good 👍
We only resignID as before in the local branch, and the remote does only destroy fields properly, looks good to me.

nickolas-pohilets · 2022-07-30T18:40:51Z

test/Concurrency/Runtime/actor_deinit_escaping_self.swift

+    }
+    let r = g.wait(timeout: .now() + .milliseconds(500))
+    expectEqual(r, .timedOut)
+    expectCrashLater(withMessage: "Assertion failed: (!oldState.getFirstJob() && \"actor has queued jobs at destruction\"), function destroy")


I'm not sure if that is even valid to test for assertions in stdlib as an expected behaviour. If stdlib is build without assertions, test will fail

DougGregor · 2022-07-31T16:42:24Z

@swift-ci please test

DougGregor · 2022-08-01T01:14:56Z

@swift-ci please test

xwu · 2022-08-01T18:21:11Z

@swift-ci please test

xwu · 2022-08-02T15:24:45Z

@swift-ci please test

stephencelis · 2022-08-25T22:49:04Z

I asked on the review thread, but maybe it'll get more visibility here. Any chance we can get a toolchain to test?

theblixguy · 2022-08-25T23:36:44Z

@swift-ci please build toolchain

Jumhyn · 2022-08-26T14:49:07Z

@swift-ci please build toolchain

Jumhyn · 2022-08-26T19:46:10Z

@swift-ci please build toolchain macOS

Jumhyn · 2022-08-26T23:06:34Z

@swift-ci please build toolchain

DougGregor · 2022-08-29T23:36:38Z

@swift-ci please build toolchain macOS

…or isolation

…m overriden declaration. But if isolation was inferred from class isolation and matches with isolation of the overriden declaration, attributes are added.

It cannot be used for executing general-purpose work, because such function would need to have a different signature to pass isolated actor instance. And being explicit about using this method only for deinit allows to use object pointer for comparison with executor identity.

* Recursive deallocation with multiple locks held * (UB) Escaping self triggers assertion in stdlib

Failing: Distributed/Runtime/distributed_actor_deinit.swift

…compiler versions

…cal storage.

…deinitOnExecutor()

…-deinit # Conflicts: # include/swift/Basic/Features.def # lib/SILGen/SILGenDestructor.cpp # test/Concurrency/flow_isolation.swift # test/abi/macOS/arm64/concurrency.swift # test/abi/macOS/x86_64/concurrency.swift

nickolas-pohilets · 2024-07-11T11:23:05Z

Rebased and updated implementation:

Removed all code changes related to TL, but updated and preserved unit-tests.
Preserved flags parameter, as an extension point for the future. No strong opinion about this, I don't mind removing it.
Removed changes for async deinit to separate branch (can be recovered later).
Removed changes to python tests (out of scope, see also Fix failures in update_checkout tests because of missing user identity #61510).
Allowed @preconcurrency attribute on deist
Fixed a bug in slow path of the isolated deinit, by removing pair of swift_retain()/swift_release() around defaultActorDrain().
Always use Legacy_NoCheckIsolated_NonCrashing when checking for current executor, even in Swift 6.

Branch is ready to run CI and resume code review process.

hborla · 2024-07-11T14:41:44Z

@swift-ci please build toolchain

hborla · 2024-07-11T14:41:49Z

@swift-ci please test

hborla · 2024-07-11T23:24:50Z

@swift-ci please build toolchain

hborla · 2024-07-11T23:24:55Z

@swift-ci please test

hborla · 2024-07-19T15:29:41Z

@swift-ci please build toolchain

ktoso · 2024-05-02T08:47:26Z

include/swift/ABI/MetadataValues.h

+    Task_EnqueueJob = 12,
+    Task_AddPendingGroupTaskUnconditionally = 13,
+    Task_IsDiscardingTask = 14,
+    Task_FunctionConsumesContext = 15,


formatting nitpick

ktoso · 2024-07-24T03:41:50Z

include/swift/AST/DeclAttr.def

@@ -326,7 +326,7 @@ SIMPLE_DECL_ATTR(_noAllocation, NoAllocation,
  OnAbstractFunction | OnSubscript | UserInaccessible | ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIStableToRemove,
  124)
 SIMPLE_DECL_ATTR(preconcurrency, Preconcurrency,
-  OnFunc | OnConstructor | OnProtocol | OnGenericType | OnVar | OnSubscript | OnEnumElement | OnImport | ABIStableToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,
+  OnFunc | OnConstructor | OnDestructor | OnProtocol | OnGenericType | OnVar | OnSubscript | OnEnumElement | OnImport | ABIStableToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,


do we need preconcurrency on deinit? I think we talked about how it shouldn't really matter there.

ktoso · 2024-07-24T07:03:37Z

include/swift/AST/DiagnosticsSema.def

+
+
+ERROR(isolated_deinit_no_isolation,none,
+      "deinit is marked isolated, but containing class %0 is not isolated to an actor",


this needs to handle struct as well because noncopyable struct can have deinits.

ERROR(isolated_deinit_no_isolation,none, "deinit is marked isolated, but containing %0 %1 is not isolated to an actor", (DescriptiveDeclKind, DeclName))

here's a patch: 03196c5

So that's the good news; the bad news is that making

@SecondActor struct SecondActorIsolatedNoncopyableStruct: ~Copyable { isolated deinit { #if !SILGEN isolatedFunc() // expected-error {{call to global actor 'FirstActor'-isolated global function 'isolatedFunc()' in a synchronous global actor 'SecondActor'-isolated context}} #endif } }

not blow up seems will take more work; First the emitDistributedRemoteActorDeinit cannot assume we're a class, this is still pretty simple:

// auto cd = cast<ClassDecl>(dd->getDeclContext()->getSelfNominalTypeDecl()); // -> auto Nominal = dd->getDeclContext()->getSelfNominalTypeDecl();

and adjust accordingly; there's only one place which needs to check for the class there.

Sadly other code will blow up then, this can be looked into afterwards after we land the initial work perhaps as I don't think this is common, but worth taking note.

ktoso · 2024-07-24T07:18:42Z

lib/Demangling/Demangler.cpp

+    case 'Z':
+      Args = None;
+      Kind = Node::Kind::IsolatedDeallocator;
+      break;


style: nitpick to follow the same style as rest of the switch

ktoso · 2024-07-24T07:35:47Z

lib/SILGen/SILGenDestructor.cpp

+    SILValue selfValue, DestructorDecl *dd, bool isIsolated,
+    llvm::function_ref<void()> emitLocalDeinit) {
+  RegularLocation loc(dd);
+  loc.markAutoGenerated();

  auto cd = cast<ClassDecl>(dd->getDeclContext()->getSelfNominalTypeDecl());


This may fail because ~Copyable sturct's deinit

ktoso · 2024-07-24T07:59:04Z

stdlib/public/Concurrency/Actor.cpp

@@ -402,20 +401,10 @@ extern "C" SWIFT_CC(swift) void _swift_task_enqueueOnExecutor(
    const SerialExecutorWitnessTable *wtable);

 SWIFT_CC(swift)
-static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor) {
+static bool isCurrentExecutor(SerialExecutorRef expectedExecutor,


Oh I see, because we need to check for the optimization... this refactoring overlaps (does the same) as some other ongoing work so we'll conflict a bit but that's fine -- yeah you're doing the right thing to try to use the legacy mode during checking here... I think this will be good enough as it's "just" an optimization

ktoso · 2024-07-24T08:00:09Z

stdlib/public/Concurrency/Actor.cpp

+  //
+  // We always use "legacy" checking mode here, because that's the desired
+  // behaviour for this use case. This does not change with SDK version or
+  // language mode.


Ok, sounds good. Indeed as a best effort the legacy mode will give us sometimes a good answer...

I'd love to get the bool returning APIs "always" but it's been a struggle to get those exposed as there's fears about such API being abused.

ktoso · 2024-07-24T08:08:01Z

test/Concurrency/Runtime/actor_recursive_deinit.swift

+}
+
+func isCurrent(_ a: AnyActor) -> Bool {
+  return isCurrentExecutor(getExecutor(a))


can't rely on the func returning bool because its runtime dependent, you have to either use an env var to make the test forced into legacy mode -- annoying, or make the test expressed in terms of "check" and just check that we don't crash in situations.

I worry about this just coming back to bite us very soon and us having to refactor all those tests that assume the bool returning here.

The test can do the same thing really, but let's not assume we're able to return the bool.

In practice returning a false here would become an assertion error so that's still doing what the test should be doing... but sadly these modes make it more confusing than it should be heh.

// I wish we didnt have this behavior but we do so let's try to not double down more on the -> bool "lie"

ktoso · 2024-07-24T08:55:20Z

test/Concurrency/Runtime/actor_deinit_escaping_self.swift

+    }
+    let r = g.wait(timeout: .now() + .milliseconds(500))
+    expectEqual(r, .timedOut)
+    expectCrashLater(withMessage: "Assertion failed: (!oldState.getFirstUnprioritisedJob() && \"actor has queued jobs at destruction\"), function destroy")


This expect... probably should move to the tests.test() {} block

ktoso · 2024-07-24T08:59:22Z

test/Concurrency/Runtime/actor_deinit_escaping_self.swift

+    // Ideally these tests should be compile-time errors
+    let tests = TestSuite("EscapingSelf")
+    tests.test("escape while locked") {
+      // TODO: Investigate failure on Linux and re-enable this test


I think we can handle this via

// REQUIRES: swift_stdlib_asserts

because we do have some builds which have assertions enabled, so then this test would work, also on linux.

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 14fe1cd to 0417ee9 Compare July 15, 2022 13:25

DougGregor reviewed Jul 16, 2022

View reviewed changes

ktoso self-requested a review July 19, 2022 11:18

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 8e3201c to facc0ca Compare July 25, 2022 11:12

nickolas-pohilets requested a review from DougGregor July 29, 2022 16:36

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from a4486cc to 292f3e1 Compare July 29, 2022 18:35

nickolas-pohilets commented Jul 30, 2022

View reviewed changes

jumhyn-duolingo mentioned this pull request Jul 31, 2022

[SE-0371] Isolated synchronous deinit swiftlang/swift-evolution#1712

Merged

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch 6 times, most recently from 5877715 to 47c4b24 Compare August 1, 2022 13:00

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 47c4b24 to 4793499 Compare August 2, 2022 10:56

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch 2 times, most recently from 324c20d to dedd1e4 Compare August 26, 2022 19:16

nickolas-pohilets added 20 commits July 11, 2024 13:09

Updated Distributed/Runtime/distributed_actor_deinit.swift to check f…

b4a1aab

…or isolation

Fixed some typos

f04f1de

Removed explicit deinit from MainActor - not needed.

cdba7b3

Don't isolate deinit of remote actor proxies

c28df0d

Expanded test for SIL for distributed actor deinit, added isolated cases

5437b31

Don't add isolation attributes for deinit when they were inferred fro…

9dfa174

…m overriden declaration. But if isolation was inferred from class isolation and matches with isolation of the overriden declaration, attributes are added.

Don't switch threads when running isolated deinit of the default actor.

a043ebf

Updated unit tests

7505cd2

Added more tests for isolated actor deinit:

f1e3002

* Recursive deallocation with multiple locks held * (UB) Escaping self triggers assertion in stdlib

Fixed tests on Linux, Windows and older MacOS versions

9dcea15

Make deinit non-isolated by default

4d0b624

Failing: Distributed/Runtime/distributed_actor_deinit.swift

Allow accessing let properties in deinit isolated on different actor

40c7c6b

Added flags to be able to control task-locals behaviour in the future

ab4d338

Implemented suppressing isolated deinit for compatibility with older …

2cb7e8d

…compiler versions

Keep current task executor when taking fast path in isolated deinit

1b69068

Don't read current task executor to avoid extra access of the task-lo…

8dbddec

…cal storage.

Added emitting precondition check in the isolated deinit

6b0ce0b

Make sure legacy executor checking mode is always used in swift_task_…

e9173ff

…deinitOnExecutor()

nickolas-pohilets force-pushed the mpokhylets/isolated-deinit branch from 620fb6d to 816d62c Compare July 11, 2024 11:13

Fixed CI builds

1304f81

Temporary disabled test failing on Linux

da75f5f

ktoso approved these changes Jul 24, 2024

View reviewed changes



		ERROR(isolated_deinit_no_isolation,none,
		"deinit is marked isolated, but containing class %0 is not isolated to an actor",

Isolated synchronous deinit #60057

Are you sure you want to change the base?

Isolated synchronous deinit #60057

Conversation

nickolas-pohilets commented Jul 14, 2022 • edited Loading

DougGregor left a 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 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Jul 16, 2022

nickolas-pohilets commented Jul 18, 2022

ktoso commented Jul 20, 2022

Choose a reason for hiding this comment

DougGregor commented Jul 31, 2022

DougGregor commented Aug 1, 2022

xwu commented Aug 1, 2022

xwu commented Aug 2, 2022

stephencelis commented Aug 25, 2022

theblixguy commented Aug 25, 2022

Jumhyn commented Aug 26, 2022

Jumhyn commented Aug 26, 2022

Jumhyn commented Aug 26, 2022

DougGregor commented Aug 29, 2022

nickolas-pohilets commented Jul 11, 2024 • edited Loading

hborla commented Jul 11, 2024

hborla commented Jul 11, 2024

hborla commented Jul 11, 2024

hborla commented Jul 11, 2024

hborla commented Jul 19, 2024

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

Choose a reason for hiding this comment

nickolas-pohilets commented Jul 14, 2022 •

edited

Loading

ktoso Jul 20, 2022 •

edited

Loading

nickolas-pohilets commented Jul 11, 2024 •

edited

Loading