I like the direction this is going, but I think it needs more iteration. In particular, it looks like the writing got updated a few times along the way as other evolution proposals came in and is now inconsistent in several places (more below). This makes it difficult to understand what is being proposed.
This is a key part of the Swift concurrency story, and many aspects of this area great: I like a clear designation of task local values as being separate from thread local and other preceding technologies, tying into structured concurrency and presenting the associated structure as a dynamic scoping mechanism is good, and many of the details and framing issues are very nice. I also like how this is clearly learning from prior art in other systems.
That said, I'd like us to continue exploring a few things:
-
The biggest and most important to me is that proposal makes great points about the analogy between TLS and builtin task properties like priority and 'is canceled', but then breaks the model for TLS by making TLS only accessible from
asyncfunctions. I think it is very important for non-async functions to have access to TLS values, because "needing to suspend" and "needing access to TLS" are orthogonal features. Allowing sync methods to access TLS allows builder patterns and other things that are very common, and are otherwise implemented with thread-local systems in other languages, and in Swift 5. My understanding is that the runtime has evolved since this was first written, and this is not an implementation problem anymore.==> It might be that the proposal has already switched, the writing is confusing and inconsistent on this point. This is what I'm reacting to in particular: "Task local values may only be accessed from contexts that are running in a task: asynchronous functions. As such all operations, except declaring the task-local value's handle are asynchronous operations."
-
Why does this feature add complexity in order to allow default values? Swift has very strong support for optional types. I would think that you'd build on top of that (having the lookup return an optional T), instead of building an alternative implementation strategy with
defaultValue. If this was removed, then we could go with other approaches that require less verbosity/boilerplate to declare keys. -
The implementation approach is quite sensible (I'm pleasantly surprised to see this level of detail in the proposal, thank you!). FYI, this algorithm is known as "path compression", at least in union-find like algorithms.
-
As a minor point, I'm curious whether the implementation will use inline optimization to optimize for a few task local values, or whether everything will use out of line indirect access. Indirect TLS affects memory use of tasks and the indirections affect locality. It seems worthwhile to provide some scratch space on the task object to make the "small TLS" case fast in practice.
-
How does the "forcing inheritance for detatched tasks" stuff work with path compression? Does this force keeping task objects alive in some cases?
-
The
Task.local(..)andTask.withLocal(..)names don't connote getting and setting TLS values to me. There is a verb that is missing here, and I don't think the word "local" is specific or communicative enough. -
" The handle is bound to a specific declaration that is accessible only to its lexical scope." nit, this is a dynamic scope, not a lexical scope. Lexical relates to syntax, these are related to the dynamic construction of tasks. This is closer to how name lookup works in elisp for example.
-
The
Sendableproposal got accepted, so all references toConcurrentValueshould be removed and be included. Detailed design has several references that are out of date.
Yes, yes. We need something to solve these sorts of problems, and this is going in the right direction.
Yes, I've used a lot of TLS and queue local sorts of storage mechanisms. This seems like natural progression aligning with the Swift Concurrency model in general.
Participated a bit in the review thread, read the proposal in depth.
-Chris