Isolated synchronous deinit #2

[ktoso (Konrad 'ktoso' Malawski 🐟🏴‍☠️)]

Hi Nickolas! Super happy that you've picked this up again!

@Nickolas_Pohilets

class MyView1: UIView { // since UIView is @MainActor
    isolated deinit { ... }
}

I agree with this as the recommended spelling for a GAIT's deinit. Basically isolated and nonisolated operate as "isolate to self (or not)" (though we had some discussions about the nuances there with @kavon recently), so it makes sense for an isolated deinit {} to mean the same when defined in a concrete actor or any other type annotated with @GlobalActor

The latter I guess does not really have to be banned... if that would just work? No strong feels on that.

It is an error to use isolated if there is no global actor isolation to inherit. It is an error to combine isolated attribute with nonisolated or global-actor isolation attribute.

Agreed as well, sounds good.

@Nickolas_Pohilets

@Jumhyn

• Task locals should not be copied for an isolated deinit. Instead, to ensure the deinit receives a consistent set of task locals, the task locals should be cleared before calling into the deinit.

I'm willing to push back on this. I think it is a premature optimization. I see value in having access to the task locals in the deinit. If object is referenced by several child tasks, you cannot know which one will make last release, so you cannot reliably use values set in child tasks. But as long as you are confident that object does not escape scope of the parent task, you can still reliably use values set by the parent task.

Relying on a task local to be present in deinit has me very concerned. We should perform some benchmarks to see the impact here.

I really would like to be able to recycle tasks for the purpose of such deinit (and similar things). So whatever we come up with here really should not prevent such this optimization in the future. It is not the optimization right now that I'm concerned about.

@Nickolas_Pohilets

And also note that performance cost for copying task locals occurs only when hopping is needed. If last release occurs on the right actor, no copying happens. Which I expect to be pretty common.

I had to re-read this a few times to see what you mean... Specifically you mean that if we're going to cause the deinit while executing on the right actor already, then you'd run the deinit in-place. Yeah then by accident we don't have to copy, that's true...

But once we get to async deinits, we'll always have to, because the Task{} we kick off to perform a deinit is unstructured, i.e. we don't await for its completion inside the scope where deinit was "caused", so we always must copy locals if we say they are guarnteed to be seen in deinit. Visualized:

func x() {
  let x = X()
  // Task { await x.deinit() }
  // no await on the deinit means that we must copy the task-locals
}

Perhaps it's worth writing up a matrix of all the cases and expected behaviors so we can see if we can come to an agreement here?

The "reset task locals" function can potentially be useful in any case; that's a nice thing we might want to consider. Implementation wise we can do this very nicely by inserting a tombstone into the chain so lookups simply stop searching once they notice it