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

Author: nickolas-pohilets

stdlib/public/CompatibilityOverride/CompatibilityOverrideConcurrency.def

@@ -142,6 +142,11 @@ OVERRIDE_ACTOR(task_switch, void,
                TaskContinuationFunction *resumeFunction, SerialExecutorRef newExecutor),
                (resumeToContext, resumeFunction, newExecutor))
 
+OVERRIDE_ACTOR(task_performOnExecutor, void,
+              SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift), swift::,
+              (void *context, AdHocWorkFunction *work, SerialExecutorRef newExecutor),
+              (context, work, newExecutor))
+
 OVERRIDE_TASK(task_create_common, AsyncTaskAndContext,
               SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift), swift::,
               (size_t taskCreateFlags,
@@ -200,11 +205,6 @@ OVERRIDE_TASK(task_createNullaryContinuationJob, NullaryContinuationJob *,
               SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift), swift::,
               (size_t priority,
                AsyncTask *continuation), (priority, continuation))
-               
-OVERRIDE_TASK(task_performOnExecutor, void,
-              SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift), swift::,
-              (void *context, AdHocWorkFunction *work, SerialExecutorRef newExecutor),
-              (context, work, newExecutor))
 
 HOOKED_OVERRIDE_TASK_NORETURN(task_asyncMainDrainQueue,
               SWIFT_EXPORT_FROM(swift_Concurrency), SWIFT_CC(swift),

stdlib/public/Concurrency/Actor.cpp

@@ -2140,6 +2140,92 @@ static void swift_task_switchImpl(SWIFT_ASYNC_CONTEXT AsyncContext *resumeContex
   _swift_task_clearCurrent();
 }
 
+namespace {
+/// Job that allows to use executor API to schedule a block of task-less
+/// synchronous code.
+class AdHocJob : public Job {
+private:
+  void *Context;
+  AdHocWorkFunction *Work;
+
+public:
+  AdHocJob(JobPriority priority, void *context, AdHocWorkFunction *work)
+      : Job({JobKind::AdHoc, priority}, &process), Context(context),
+        Work(work) {}
+
+  SWIFT_CC(swiftasync)
+  static void process(Job *_job) {
+    auto *job = cast<AdHocJob>(_job);
+    void *ctx = job->Context;
+    AdHocWorkFunction *work = job->Work;
+    delete job;
+    return work(ctx);
+  }
+
+  static bool classof(const Job *job) {
+    return job->Flags.getKind() == JobKind::AdHoc;
+  }
+};
+} // namespace
+
+SWIFT_CC(swift)
+static void swift_task_performOnExecutorImpl(void *context,
+                                             AdHocWorkFunction *work,
+                                             SerialExecutorRef newExecutor) {
+  // If the current executor is compatible with running the new executor,
+  // we can just immediately continue running with the resume function
+  // we were passed in.
+  //
+  // Note that swift_task_isCurrentExecutor() returns true for @MainActor
+  // when running on the main thread without any executor
+  if (swift_task_isCurrentExecutor(newExecutor)) {
+    return work(context); // 'return' forces tail call
+  }
+
+  // Optimize deallocation of the default actors
+  if (context == newExecutor.getIdentity() && newExecutor.isDefaultActor()) {
+    // Try to take the lock. This should always succeed, unless someone is
+    // running the actor using unsafe unowned reference.
+    if (asImpl(newExecutor.getDefaultActor())->tryLock(false)) {
+
+      // Don't unlock current executor, because we must preserve it when
+      // returning. If we release the lock, we might not be able to get it back.
+      // It cannot produce deadlocks, because:
+      //   * we use tryLock(), not lock()
+      //   * each object can be deinitialized only once, so call graph of
+      //   deinit's cannot have cycles.
+
+      // Function runOnAssumedThread() tries to reuse existing tracking info,
+      // but we don't have a tail call anyway, so this does not help much here.
+      // Always create new tracking info to keep code simple.
+      ExecutorTrackingInfo trackingInfo;
+      trackingInfo.enterAndShadow(newExecutor, TaskExecutorRef::undefined());
+
+      // Run the work.
+      work(context);
+
+      // `work` is a synchronous function, it cannot call swift_task_switch()
+      // If it calls any synchronous API that may change executor inside
+      // tracking info, that API is also responsible for changing it back.
+      assert(newExecutor == trackingInfo.getActiveExecutor());
+
+      // Leave the tracking frame
+      trackingInfo.leave();
+
+      // Give up the current actor.
+      asImpl(newExecutor.getDefaultActor())->unlock(true);
+      return;
+    }
+  }
+
+  auto currentTask = swift_task_getCurrent();
+  auto priority = currentTask ? swift_task_currentPriority(currentTask)
+                              : swift_task_getCurrentThreadPriority();
+
+  auto job = new AdHocJob(priority, context, work);
+  swift_task_enqueue(job, newExecutor);
+}
+
 /*****************************************************************************/
 /************************* GENERIC ACTOR INTERFACES **************************/
 /*****************************************************************************/

stdlib/public/Concurrency/Task.cpp

@@ -1674,56 +1674,6 @@ swift_task_createNullaryContinuationJobImpl(
   return job;
 }
 
-/// Job that allows to use executor API to schedule a block of task-less
-/// synchronous code.
-class AdHocJob : public Job {
-private:
-  void *Context;
-  AdHocWorkFunction *Work;
-
-public:
-  AdHocJob(JobPriority priority, void *context, AdHocWorkFunction *work)
-      : Job({JobKind::AdHoc, priority}, &process), Context(context),
-        Work(work) {}
-
-  SWIFT_CC(swiftasync)
-  static void process(Job *_job) {
-    auto *job = cast<AdHocJob>(_job);
-    void *ctx = job->Context;
-    AdHocWorkFunction *work = job->Work;
-    delete job;
-    return work(ctx);
-  }
-
-  static bool classof(const Job *job) {
-    return job->Flags.getKind() == JobKind::AdHoc;
-  }
-};
-
-SWIFT_CC(swift)
-static void swift_task_performOnExecutorImpl(void *context,
-                                             AdHocWorkFunction *work,
-                                             SerialExecutorRef newExecutor) {
-  auto currentExecutor = swift_task_getCurrentExecutor();
-
-  // If the current executor is compatible with running the new executor,
-  // we can just immediately continue running with the resume function
-  // we were passed in.
-  //
-  // Note that swift_task_isCurrentExecutor() returns true for @MainActor
-  // when running on the main thread without any executor
-  if (swift_task_isCurrentExecutor(newExecutor)) {
-    return work(context); // 'return' forces tail call
-  }
-
-  auto currentTask = swift_task_getCurrent();
-  auto priority = currentTask ? swift_task_currentPriority(currentTask)
-                              : swift_task_getCurrentThreadPriority();
-
-  auto job = new AdHocJob(priority, context, work);
-  swift_task_enqueue(job, newExecutor);
-}
-
 SWIFT_CC(swift)
 void swift::swift_continuation_logFailedCheck(const char *message) {
   swift_reportError(0, message);

test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift

@@ -55,6 +55,21 @@ actor ActorNoOp {
   }
 }
 
+actor ProxyActor: Actor {
+  private let impl: ActorNoOp
+
+  init(expectedNumber: Int, group: DispatchGroup) {
+    self.impl = ActorNoOp(expectedNumber: expectedNumber, group: group)
+  }
+
+  // Explicit deinit to force isolation
+  deinit {}
+
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    return impl.unownedExecutor
+  }
+}
+
 @globalActor actor AnotherActor: GlobalActor {
   static let shared = AnotherActor()
 
@@ -107,7 +122,7 @@ class ClassNoOp: Probe {
 let tests = TestSuite("Isolated Deinit")
 
 if #available(SwiftStdlib 5.1, *) {
-  tests.test("fast path") {
+  tests.test("class sync fast path") {
     let group = DispatchGroup()
     group.enter(1)
     Task {
@@ -120,20 +135,42 @@ if #available(SwiftStdlib 5.1, *) {
     group.wait()
   }
 
-  tests.test("slow path") {
+  tests.test("class sync slow path") {
     let group = DispatchGroup()
-    group.enter(2)
+    group.enter(1)
     Task {
-      TL.$number.withValue(37) {
-        _ = ActorNoOp(expectedNumber: 0, group: group)
-      }
       TL.$number.withValue(99) {
         _ = ClassNoOp(expectedNumber: 0, group: group)
       }
     }
     group.wait()
   }
 
+  tests.test("actor sync fast path") {
+    let group = DispatchGroup()
+    group.enter(1)
+    Task {
+      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.
+        _ = ActorNoOp(expectedNumber: 99, group: group)
+      }
+    }
+    group.wait()
+  }
+
+  tests.test("actor sync slow path") {
+    let group = DispatchGroup()
+    group.enter(1)
+    Task {
+      TL.$number.withValue(99) {
+        // Using ProxyActor breaks optimization
+        _ = ProxyActor(expectedNumber: 0, group: group)
+      }
+    }
+    group.wait()
+  }
+
   tests.test("no TLs") {
     let group = DispatchGroup()
     group.enter(2)