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 (via email or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
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?
+1. The proposal itself is extremely well written, enough so that someone with no experience with atomics, like me, could learn a lot and understand what's being added to the language.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes. Atomics often come up as significant limitation for Swift in systems programming or other low level work.
Does this proposal fit well with the feel and direction of Swift?
Yes.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Never used atomics, though the APIs look much nicer than their C++ equivalents.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Watched the pitch thread from afar and did a quick read of the proposal.
My only question is, why no UnsafeAtomic<Bool>? It seems useful enough on its own and could even expose a safe toggle().
Overall, it is thorough, pragmatic, constrained, and reasonable. Great work!
Is the problem being addressed significant enough to warrant a change to Swift?
In short, yes.
Does this proposal fit well with the feel and direction of Swift?
Yup.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I've worked with x86/PowerPC/ARM assembly and the C++ model, and I find them all to be maddening for different reasons. I think the fundamental problem with atomic instructions (and abstractions like the C++ model) is that the user-experience is such a cliff. Yes, implementing a counter, spin-lock, or LIFO is straightforward and well documented, but good luck if you if you try to implement something like an atomic collection type or a reader-writer lock that doesn't spin-wait. You basically need to become an expert at multiple CPU architectures and have nontrivial testing resources to have any confidence that things are working correctly, and then at that point the C++ abstraction model feels quaint at best.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
+1. I’m very happy with how this proposal turned out. It was refined a lot during the pitch phase. Thank you everyone who contributed!
Is the problem being addressed significant enough to warrant a change to Swift?
Yes. Swift aspires to be suitable for lower level systems programming and atomic are an important feature in that domain. More generally, it’s always nice to reduce the number of reasons we might need to leave Swift and do something in C(++).
Does this proposal fit well with the feel and direction of Swift?
Very much so.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I have never used atomic in C++ but am superficially familiar with them. The alignment with C++ seems like a good decision to me.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I followed the discussion in the pitch thread and proposal revisions closely.
Same uses as a normal toggle() I would think. I’m mainly wondering why UnsafeAtomic<Bool> was left out? Was it strictly utility? The cost doesn’t seem that high in that case, but maybe I’m missing something.
Well I mean the thing is that atomics are used for concurrent mutations - is there a case where you’d want to concurrently toggle a Boolean, without caring about its initial or final state? All it would actually store is whether the Boolean was flipped an odd or even number of times. I’m not sure that’s really useful, and it’s simple to build on top of an atomic integer.
I don’t agree with exposing methods just because we can, and this low-level interface shouldn’t add too many conveniences - it’s not supposed to be used widely, so that stuff just becomes bloat. But maybe there is some particular data structure that uses it and a really makes it a compelling operation to add?
UnsafeAtomic<Bool> by itself is fine, I guess. I wonder if we should maybe also include a version of atomic_flag for architectures which can’t support full lock-free atomics, although none of the architectures we currently support have that limitation AFAIK.
Yes. Performant lock-free programming is currently not accessible to Swift programmers, necessitating C modules that can do this work. These C modules tend to feel awkward in Swift, and broaden the scope of the unsafe code from simply the operations on the atomics and related memory locations out to unrelated other operations on incidental data.
Swift is currently entirely incapable of operating in this space: this proposal addresses that limitation.
I believe so. It clearly addresses the current status of Swift's memory and ownership models, as well as the pre-existing rules on concurrent memory access. It defines a limited extension to those rules with an eye towards sensible interoperability with future directions. It relies heavily on pre-existing terms of art in Swift (including Unsafe), and provides clean, idiomatic Swift spellings for atomic operations.
This proposal draws heavily from the pre-existing C/C++ memory model. While there are many criticisms that can be levelled at that memory model, inventing an entirely new model out of whole cloth would have been inappropriate and excessive. Additionally, as noted in the proposal, Swift already implicitly assumes this memory model applies (albeit with additional restrictions), so adopting this memory model is reasonable.
There are sharp edges and dragons here, of course. Some of these are inherent to lock-free programming, and some are not. It seems fair to say that this proposal is not the best-possible API for atomics, but providing that in Swift requires a number of language features that do not exist today. I'm in agreement with the author that the perfect should not be the enemy of the good, and waiting for a fully-fledged ownership model in order to bring a feature into the language seems unwise to me.
I've read multiple versions of the proposal, worked with NIO's pre-existing atomic support (such as it is), and collaborated with @Karoy_Lorentey on ensuring that this proposal would suit our existing and future use-cases.
+1. I feel good about this change because it means we're finally getting to the point where Swift is making up for its lack of 'must-haves' as a full-featured language.
Without a shadow of a doubt, yes.
As in all forms of engineering, there are tradeoffs; it's a very complex solution, but I like that it lives within its own namespace so, 99% that don't need to know that these exists, won't ever need to know.
I remember using atomic properties in Objective-C once or twice, but I wouldn't consider myself an expert user. The highest level of synchronization work I do uses DispatchQueues either directly or through a Swift Property-Wrapper, but I'm familiar with the topic.
In the proposal UnsafeAtomic* types is written. Normally MyType* in C++ means a pointer to the type MyType. * is not added as an operator for pointers in Swift and never talked about in that proposal. If UnsafeAtomic itself represents a pointer, what are UnsafeAtomic* types then.
Is it just written wrong or what does it mean in that context?
I found it cumbersome to see all the additions which would be added, since they are spread out between the explanations. The proposed solution is longer than the detailed design.
I looks like it is too much in one proposal. In other big proposals the answer of the core team is, that it should be split up into smaller ones to (exclude the ones that come from Apple, maybe this is one of them).
The Motivation just says it is important, other proposals at least give an academic example. I do not see a big benefit, but I am no system programmer. The proposals seems to be written from a C++ perspective to a C++ programmer. I used UnsafePointer in Swift with C libraries and concurrency and did not had a problem.
I am very happy with the way this proposal has evolved, and I support it fully.
The only thing I'm not enthusiastic about is including the "load after" variants of operations on integers. I'm dubious about their utility, and as far as I can tell they're not even implemented in llvm. If the near-duplicates were not included, we could have simpler names. In the occasional case where user can't represent their algorithm in terms of value_before, they have all the same tools as the stdlib to derive value_after, and the optimizer will be just as helpful. This would reduce the integer-specific read-modify-write methods from 12 to 5 (increment/decrement can get the discardableResult attribute.)
Is the problem being addressed significant enough to warrant a change to Swift?
Yes.
Does this proposal fit well with the feel and direction of Swift?
Accepting that this is not the be-all end-all for this kind of functionality, yes.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I've used atomics quite a bit in C and C++, and have tinkered with atomics bridged from clang to swift quite a bit as well. I understand the criticisms of the C++ memory model, but given how Swift is built, the alternatives seem either worse or unproven.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
In-depth. I've been following this implementation PR and its predecessor, gave feedback on the pitch and the PRs, and implemented a version of it just because.
I love it, I can't wait for Swift to have this functionality. That said, this is a very large proposal and I think it is very important to continue refining some details of the proposal.
In particular, given the design points, I think it would make sense to carefully consider whether it makes sense to make a significant design shift and replace the UnsafeAtomic struct with an AtomicReference final class. This would be safe (just like the eventual Atomic<T> type in the future) and provide the same reference behavior. AtomicReference would still provide access to the low-level unsafe mechanics as static methods for library builders. I expect this to be controversial, so the other feedback below doesn't assume this change.
More detailed thoughts and comments:
Unifying this under a single generic type is great, but the name UnsafeAtomic is very problematic to me. This type is unsafe, but the name UnsafeAtomic does not imply the reference semantic behavior that this type provides, and I don't believe that "Unsafe" connotes references that at all. I would much rather see a name like UnsafePointerToAtomic or UnsafeAtomicReference.
Relatedly, I do not believe that the type name or method names need to be have particularly terse names. This is a bundle of sharp knives intended for experts, and being explicit is more important than being terse. The method names LGTM btw, this is just a general opinion.
Comment on the writing: in the "We want to limit this proposal to constructs that satisfy the following requirements:" section, points 2/3 could be turned into a single point about how the proposal values "predictability over generality". I think this is the principle that underlies much of your approach - and I agree that is super important btw.
Curious: why is it important to split this off to another module? With the new design, it seems to be a single UnsafeAtomic type, a few protocol and some other stuff that starts with the word Atomic - is there harm in putting that into the standard library? As far as I know, this would be the first thing split off of the stdlib, so this would set new precedent - is there a principle for how we should think about this in the future?
I would recommend renaming AtomicProtocol -> AtomicValueProtocol (or something else) for clarity. Multiple things in this design relate to atomics, but this protocol is specifically about values that can be atomically accessed.
I still think that UnsafeAtomicLazyReference is an outlier when compared to the rest of this proposal. The rest of the proposal consists of the base primitives that require access to compiler intrinsics to implement. This is one specific helper type used for higher level applications. I would recommend subsetting this out of the initial proposal, and add it back when the community has more experience with the basic functionality. At that time, we may find that there is a collection of helpers that would be added as a group and designed together as a family (along with double wide atomics and many other things that were intentionally subset out).
As I mentioned in the pitch phase, I think it is really important to provide access to the low-level operations as static methods on UnsafeAtomic. This allows implementors to build things that are more efficient. For example, compare the AtomicCounter example in the proposal to the version shown in the linked post. I don't see any downside to this either, this is just a matter of exposing the members that already exist on AtomicProtocol but which aren't "public API".
I would recommend using an initializer instead of a create method, because Swift consistently uses initializers for these things. Instead of: let atomic = UnsafeAtomic<Value>.create(initialValue: 0), it would be better to have: let atomic = UnsafeAtomic<Value>(createInitialValue: 0) or whatever.
More significantly, I think this whole create/destroy pattern is a potentially significant design smell for this type in general. Did you consider making UnsafeAtomic be a final class instead of a struct? This would define away the destroy method, make the whole system far safer. You're already forcing a separate allocation anyway, the only downside is that you'd get an additional reference count, but that is probably swallowed up by a typical malloc quantum anyway. I think this is a pretty big design hinge, and I think it should be discussed in depth in the Alternatives Considered section.
Yes; Yes. I have use the c++ std::atomic and related functionally extensively. This seems like a great step for Swift.
I participated in several rounds of iteration in the pitch phase and did a detailed review of this draft of the proposal, but did not read all the other comments above.
Thank you so much to the proposal authors for driving this - this is a really important step in Swift's evolution, and they have put in a tremendous amount of work shaping and iterating this in the pitch phase, I am super excited to see this coming together!
ImplicitlyUnwrappedOptional doesn't exist anymore as a separate type. T! is purely a declaration modifier for arguments, returns, and property types. See SE-54.
Yes, atomics are extremely important if Swift is to fulfill it's vision of being a full-stack programming language.
Yes, I really like how things have been refined over the course of the pitch phase.
I concur with @Chris_Lattner3; I'm concerned with the lack of static methods to access the atomic operations. This will be really helpful in my applications of atomics in order to avoid the extra overhead. Without that capability, I will need to either keep my use of C, or eat some extra overheads (and will likely choose the former, meaning I won't get any benefit from the proposal for my current application of atomics).
Further, I'd like to see support for tear-able reads added, but that can be in a subsequent evolution.
Additionally, as pointed out on the pitch thread, this proposal definitely runs up against limitations of today's Swift. While I don't think we should slow this evolution proposal down, I strongly encourage the core team to prioritize these fundamental issues sooner rather than later, if at all possible.
In depth study, and have used atomics (via C) in Swift a number of times.
Having a class-based interface strikes me as a nice thing to build on top of the functionality this proposal provides. We'd want these atomics to serve as underlying functionality for putting atomic variables into inline storage within larger classes, or eventually for safe move-only atomic types; if they come pre-wrapped in their own class instances, that would get in the way of those other eventual use cases.
Could you give an example of what overhead static methods would avoid?