Optimising async task
I think you misunderstood me. There is no such assumption. I was talking about tasks being enqueued, but not running yet. Currently such tasks still have first slab of the stack allocated. This allocation is triggered by using task allocator to allocate internal data structures for priority escalation. If we can avoid this somehow, we could make memory footprint of unstated tasks much more lightweight, comparable to a single job.
Note that the current proposal draft contains diagnostics guiding users away from unnecessary usage of async deinit. So quite the contrary, we can assume that async deinit suspends at least once.
Priorities
Job for isolated deinit runs at the same priority as the code that performed last release. And if system exists in condition where deinit job may never run, this means that last release also may never run. Such system would be accumulating unfreed resources even with regular deinit.
Task locals
Default behavior
My preference was copying TL by default, plus an option for advanced users to do nothing.
“Copying” actually means to do nothing on the fast path. It has the best possible performance and behavior compatible with regular deinit. And actual copying on the slow path is intended to address different problem.
In the code snippet that @ktoso provided, there are two different tasks racing for the last release. In this case, copying cannot help, because each of the tasks has its own set of task-local values. In this example, deinit is not special from any other function. Any other function, if called from different tasks, will observe different task-local values. I don't think this is a problem at all. That's just how task-locals work, that's in their name.
@TaskLocal
static var value: String = "Hello"
actor MyActor {
var isClosed: Bool = false
func close() {
if !isClosed {
isClosed = true
print(Self.value)
}
}
}
func situationA_causesClose(actor: MyActor) async {
await $value.withValue("Some Value") {
await actor.close()
}
}
// ~~~ these two calls are "racing" ~~~
func situationD_causesRelease(actor: MyActor) async {
Task.detached {
await actor.close() // oh, no task locals
}
}
Copying on the slow path is intended to solve a different issue:
@TaskLocal
var value: String = "Hello"
@MainActor
final class SomeClass {
var store: SomeClass? = nil
isolated deinit {
print("context: \(value)")
}
func store(other: SomeClass) {
self.store = other // retain the class...
}
func release() { self.store = nil }
}
func example() async {
await $value.withValue("Some Value") {
let root = SomeClass()
root.store(other: SomeClass())
// Some logic
if Bool.random() {
await root.release() // child deinit takes fast path
}
// otherwise child deinit takes slow path
}
}
In this example, there are regions of code with different isolations, but all being part of the same task and all within the scope of the withValue(). Currently in Swift there are no lifetimes, so compiler cannot check that root or child objects do not escape the scope of withValue(). But developers still can do it manually. And if developers are confident enough that reference does not escape the scope, then they can reasonably assume that object will be deinitialised inside the scope, and expect that task locals are available inside deinitializer the way they are inside any other function.
Availability of task-locals in deinit is as reliable as escape analysis is. Manual escape analysis is not perfect, but can be good enough for some cases. While I would not recommend relying on task-locals in the deinit, I don't think it is bad enough to punish people for that, and pay performance costs for this punishment.
For regular deinit, in the example above task-locals would be visible on both fast path and slow path. If task-locals are not copied on the slow path, then adoption of the isolated deinit may cause breaking behavioural changes. Most users probably won't care about this, but for some this will come as a surprise. In this case, I think it is better to be consistent and also clear task locals on the fast path, to help users to discover this quirk sooner, and make it more likely to be caught in unit tests.
So for a default option, my preference is - "no-op(keep)/copy" > "reset/no-op(none)" > "no-op(keep)/no-op(none)".
Alternative behaviors
In addition to that, we can use attributes to provide alternative behaviours.
If default behaviour is "no-op(keep)/copy", then I see value in "no-op(keep)/no-op(none)" being available as a performance optimization for advanced users. I see it being useful in cases when code inside deinit does not use any task-locals, so paying runtime cost for copying them does not make sense. But if they are not used at all, then keeping them on the fast path should be an issue either. So I don't see a need to support "reset/no-op(none)".
Note that instead of optimising away copying of the task-local values, it can be more productive to minimise number of slow paths. E.g. destroying an array of objects with isolated deinit on a wrong actor will create a job for each object. But if array is wrapped into another object isolated to the same actor, then only one job will be created:
@MainActor
class Leaf {
isolated deinit {}
}
@AnotherActor
class Bad {
var leaves: [Leaf]
isolated deinit {
// Will create leaves.count jobs
}
}
@MainActor
class Container {
var leaves: [Leaf]
isolated deinit {
// Destroys leaves using fast path
}
}
@AnotherActor
class Good {
let container: Container
isolated deinit {
// Will create one job to destroy Container
}
}
If default behaviour is "reset/no-op(none)", then I see value in "no-op(keep)/no-op(none)" for performance reasons as well. But also I would provide "no-op(keep)/copy" as a way to quickly mitigate behavioural changes in the short term and be able to adopt best practices later, after successful adoption of the concurrency checking.
Opaque copy
And finally, if the best practice that we recommend is to copy TLs in the initializer and keep them as instance properties, then I think we may need to provide users with one more tool.
Task-locals can be used as an implementation detail, and not necessarily exposed publicly. In this case it is not possible to copy them in initializer. So, I think we need an API which allows opaque copying of all task-local values. Something like this:
// Library, closed source, cannot be changed
private class Context {
var foo: String
var bar: Bool
@TaskLocal
static var currentContext: Context?
}
public func withContext(foo: String, perform action: () async -> Void) async {
await Context.$currentContext.withValue(Context(foo: foo, bar: false)) {
await action()
}
}
public func useContext() {
if let context = Context.currentContext {
print(context.foo)
context.bar.toggle()
}
}
// App, uses library
class Processor {
var values: TaskLocalValues
init() {
// Copies everything including Context.currentContext
self.values = TaskLocalValues.current()
}
deinit {
self.values.perform {
useContext()
}
}
}
func example() async {
await withContext(foo: "Something") {
let processor = Processor()
...
}
}
