SE-0371: Isolated synchronous deinit

Jumhyn · 2022-08-18T23:14:09.407Z

Hello Swift community,

The review of SE-0371: "Isolated synchronous deinit" begins now and runs through August 31, 2022.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. When emailing the review manager directly, please keep the proposal link at the top of the message.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/main/process.md

Thank you,

Freddy Kellison-Linn
Review Manager

Jason_Gregori · 2022-08-21T00:35:41.998Z

+1 I think this is great and fills a big hole

ktoso · 2022-08-22T04:17:56.800Z

Very nice work overall. Let's dive into some of the specifics then

Default (not user-declared) deinit semantics looking good, no need to hop there

Overall semantics look right, though I'll harp on a bit about the defaults a bit below, please bear with me

Otherwise, the task-less job copies priority and task-local values from the task/thread that released the last strong reference to the object.

I will say I'm a bit iffy about proactively coping task-locals to deinits... I said that during the pitch already and still am a bit uncertain about it. I don't really see a lot of good use-cases for it, but yes I agree that it would cause a weird difference between sync and async deinits which could be unfortunate.

What I am worried about here is that all those task-local copying going on since most deinits will never actually read them, and if there's some hierarchy of tasks the traversal and copying for every deinit done in their scope can build up quite a bit in total copying/time... seems like a lot of wasted work to me, and I don't really see a good API that would strongly rely on this capability...

Perhaps we should lave this unspecified and not copy eagerly for the time being, and if we ever find a good reason to do so, we can start copying.

Alternatives considered: Explicit opt-in into deinit isolation
&
Future Directions: Asynchronous deinit

I'd also like to push back on this alternative considered, and leaving the deinit async as some separate work.

The proposal as is will make the following deinit hop:

actor A: Identifiable { 
  // nonisolated var id: ID
  deinit { print("\(self.id) deinit") }
}

Even though it doesn't have to. I'm worried about accidentally introducing hops like this, and also since this also causes copying of task locals, so it's potentially double as heavy than it looks like.

It also takes a different approach than actor initializers which I think is a wrong bias to take here, specifically:

The proposal states that there isn't a way to ascribe methods to "opt into isolation" but that's incorrect: initializers do have to opt in. Only members like methods etc don't opt in and get isolated by default.

We should not align deinit with methods, but with init, specifically:

actor A { 
  let x: Int

  init() {
    self.x = 12
    // NO HOP, because not-`async` (!)
    // can't touch isolated properties; body is effectively nonisolated here
  }

  init() async {
    self.x = 12
    // HOP, because `async` (!)
    doThings()
  }
}

so initializers do opt into isolation; and deinitializers should have to do the same IMHO, because they are like initializers and not like "methods"

actor A { 
  // init semantics are as follows:
  init() {} // once fully initialized, body is not isolated, can't refer to isolated state after FI (or `self` even)
  init() async {} // opt-in, task hop; can refer to self and other members after FullyInitialized

  // so deinit should follow it:
  deinit {} // not isolated, can't refer to isolated state
  deinit async {} // opt-in, task hop; can refer to self and all isolated members
}

extension A { 
  // methods on the other hand, opt-out
  func hello() {} // isolated, not async
  func hello() async {} // isolated, async
  nonisolated func hello() {} // isolated, async
}

so IMHO we should model deinit closer to init and not to methods, because they are used differently.

We should work a little bit more to make the deinit async possible and solve two problems with one proper solution, and not jump to the partial "just always hop" which is both the wrong tradeoff and does not mirror how initializers work in the language already.

I understand the proposal states that:

Similarly to this proposal, __deallocating_deinit can be used as a thunk that starts an unstructured task for executing async deinit. But such naïve approach can flood the task scheduler when deallocating large data structure with many async deinit s. More research is needed to understand typical usage of asynchronous operations in deinit , and applicable optimization methods. This is out of scope of this proposal.

But that's not an un-solvable problem, and we should get deinitializers right rather than having to retro-fit the right solution once we get to it later on.

For example, we can create a pool of tasks which we use to submit deinitializations to "please run this deinit" for a deinit pool. Since the global thread pool running tasks is width limited anyway, we can have as many tasks as there are threads, so that in practice we're able to utilize the pool maximally if nothing else is happening on the global executor pool. This way there is a limited number of tasks and we are not putting much memory pressure onto allocating complete new tasks to run the deinits, I believe that's an important part of a deinit design to begin with - we should not need much (preferably none) allocations, to deallocate after all

Nickolas_Pohilets · 2022-08-22T09:17:57.103Z

My assumption is that even if copying task locals is expensive, it should happen very rarely.

For starters, only 3.5% of class declarations have explicit deinit, based on SourceGraph search in open-source databases.

For default actors, latest implementation attempts to take lock on the current thread without hopping. And since actors are kept retained while there are any jobs, taking lock for deinit should be uncontended and always succeed. One would need to do something shady with unowned(unsafe) reference to trigger hopping. If you are aware of any other cases where actor lock can be contended in deinit - please let me know, that would be a juicy unit test.

Isolated deinit of default actors
When deinitializing an instance of default actor, swift_task_deinitOnExecutor() attempts to take actor's lock and execute deinit on the current thread. If previous executor was another default actor, it remains locked. So potentially multiple actors can be locked at the same time. This does not lead to deadlocks, because (1) lock is acquired conditionally, without waiting; and (2) object cannot be deinitializer twice, so graph of the deinit calls has no cycles.

For GAITs, I expect that majority of references live on the isolating actor anyway, so the last reference is likely to happen on the isolating actor and will not cause hopping. But even if it happens from the different thread/actor, I expect that entire tree of objects being deinitialized to be isolated on the same actor, so hopping will happen only once for the root of the tree.

These assumptions are based on subjective experience with my current codebase. Can we instead rely on data for making this kind of decisions? Is there a way to profile this on open-source Swift codebases, or on apps submitted to Apple?

stephencelis · 2022-08-25T02:25:39.438Z

Is there a toolchain available? Would love to take it for a spin.

ktoso · 2022-08-26T08:12:35.544Z

If it helps, a linux one has built successfully a few hours ago: [Pull Request] Swift Build Toolchain - Ubuntu 20.04 #153 [Jenkins] You could drop it into a docker container and play around.

You can check for these on the PR (check the CI status list, there are toolchain jobs there).

stephencelis · 2022-08-26T13:07:17.643Z

Unfortunately my use case for taking this for a spin is only on Apple platforms. Hopefully a successful macOS toolchain will build soon!

inamiy · 2022-08-28T07:10:17.297Z

+0.5 at the moment.

Previously I wrote in the pitch:

Isolated synchronous deinit Pitches

+1 for opt-in deinit async as I previously commented in: And in my opinion: "Non-isolated deinit when code is not written" will mislead developers when they just want to write a debug-print to observe when actor instance actually gets deallocated. // Assume this is implicit isolated deinit. deinit { // This printing may be called slower than non-isolated deinit. // (Also, thread printing will get changed which might also be tricky) print("deinit in \(Thread.current)") }

and added a concern about "nonisolated deinit" (when not writing deinit explicitly) which then suddenly turns into "isolated deinit" when writing.
I think this will cause a huge behavioral gap without any available opt-ins.

While "deinit async" came up as a solution for opt-in explicit isolated deinit, I now find that the entire discussion could be simplified just by introducing nonisolated deinit keyword instead to opt-in the same behavior as the compiler implicitly injects.

I believe having nonisolated deinit will eventually avoid thinking about complicated deinit async topic, and nonisolated deinit (or deinit async in the 2nd favor) is also a must-have feature to accompany with this proposal to add extra +0.5.

Side note

As a side note, having nonisolated deinit will unfortunately break:

init / init async / deinit / deinit async

symmetry, but I'm recently inclined to look for:

nonisolated init / init / nonisolated deinit / deinit

as more simpler and better correspondence, which actually oppose to the already-accepted SE-0327 On Actors and Initialization.

While explaining why is another long topic (so I will skip here for now), we can still discuss about nonisolated deinit separately from initializer (and their symmetry) topic.

OneSadCookie · 2022-08-29T20:33:07.324Z

+1 for this. My primary need for this proposal is that destructors on subclasses of UIView/UIViewController work again. The rest of it is icing

Douglas_Gregor · 2022-08-30T20:11:44.147Z

Hi all,

My apologies for not getting to review this sooner.

I'm thrilled to see this proposal. It's very carefully thought out and implemented, and extremely important for the usability for Swift's actor isolation model. Thank you!

My impression follows along pretty closely with @ktoso 's review. In particular, this would have been my summary:

ktoso:

Overall semantics look right, though I'll harp on a bit about the defaults a bit below

I agree with Konrad that copying task-locals to deinits is potentially a lot of expense without a lot of use cases supporting it. I think I'll take the argument further, that a deinit that depends on task locals is already semantically broken, because merely capturing the value in a different task would make it observe different task locals, and would be very likely be susceptible to higher-level race conditions where the deinit can happen in different tasks.

To this comment:

Nickolas_Pohilets:

My assumption is that even if copying task locals is expensive, it should happen very rarely.

My concern is that there's a performance cliff here: if you happen to trigger deallocation from a task-local-heavy task, deinitialization suddenly becomes expensive, and it would be hard to work around. And again, I feel like we're adding a cost throughout the system to serve a narrow use case that's more like an anti-pattern.

inamiy:

added a concern about "nonisolated deinit" (when not writing deinit explicitly) which then suddenly turns into "isolated deinit" when writing.

I share this concern: it has so far been the case that the implicit deinitializer can be written explicitly as deinit { }, and it seems unfortunate to introduce a subtle semantic difference here.

This is part of a more general concern that defaulting to an isolated deinit feels off: there's a binary size and runtime cost. I expect it to be small for default actors, and that actors with custom executors are fairly rare... but in the app space I expect there to be a lot of @MainActor class types, and if we're unnecessarily hopping over to the main thread for deinitialization that could become a problem.

I think I'd like to flip the default here, and say that deinit is nonisolated by default, just like init is nonisolated by default. Most deinits will stay nonisolated (which is good for code size/performance), and developers can opt in to making the deinitializer isolated in response to some need---say, accessing a non-Sendable property or using some isolated operation. We can make this discoverable through improved diagnostics.

I'm flip-flopping on this:

ktoso:

We should work a little bit more to make the deinit async possible and solve two problems with one proper solution, and not jump to the partial "just always hop" which is both the wrong tradeoff and does not mirror how initializers work in the language already.

Originally, I considered deinit async to be a separate feature, but I find this interesting. If, as I'm arguing above, we need to opt in to isolated deinitializers, then we have a choice between adding new syntax (isolated deinit) or extending the scope of this feature to mirror that of initializers (deinit async). The latter is really interesting: an isolated deinitializer is effectively an async function already, and marking it as such (1) highlights that there is a potential suspension point when performing deinitialization, and (2) would enable use of other async functions, which is a common feature request.

stephencelis:

Is there a toolchain available? Would love to take it for a spin.

I'm going to look into this further. Something is tripping up our toolchain builds. Sorry about that!

Doug

Fritzt · 2022-08-31T15:38:41.790Z

+1 for the same reason @OneSadCookie mentioned.

inamiy · 2022-08-31T17:41:01.847Z

Douglas_Gregor:

Originally, I considered deinit async to be a separate feature, but I find this interesting. If, as I'm arguing above, we need to opt in to isolated deinitializers, then we have a choice between adding new syntax (isolated deinit) or extending the scope of this feature to mirror that of initializers (deinit async). The latter is really interesting: an isolated deinitializer is effectively an async function already, and marking it as such (1) highlights that there is a potential suspension point when performing deinitialization, and (2) would enable use of other async functions, which is a common feature request.

I'm +1 for having an explicit isolated deinit as an alternative.

Yet I also agree that having deinit async will be more powerful and challenging,
so I would like to see its focused discussion in a new separate thread.

(If the new discussion takes more time to complete, gradual migration from isolated deinit to deinit async might to be an option.)

Jumhyn · 2022-09-02T15:40:33.933Z

The review has concluded and the workgroup has decided to return this proposal for revision.

John_McCall · 2022-09-02T17:52:50.865Z