SE-0371: Isolated synchronous deinit

Evolution Proposal Reviews
3

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

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

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

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 :slight_smile:

3 Likes