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 me as the review manager via email or direct message on the forums. If you send me email, please put "SE-0223" somewhere in the subject line.
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. This seems like a definite performance trap in Swift.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes. The code change is small and the benefits are worthwhile.
Does this proposal fit well with the feel and direction of Swift?
Yes. One of Swift's nicest features is its value type system. It yields code that is easier to reason about, with a tradeoff â sometimes values are copied around more than expected. This change is the best kind, because it gives us back that performance, without sacrificing the grokkable model.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I don't have experience with other languages at this level of depth.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I read the pitch thread, the proposal multiple times, and the implementation PR.
+1. I've wanted something like this for a very long time (e.g. to implement a concurrent map, where values are initialised in parallel). It's also very helpful for C interop. The API seems as clear as it can be.
Is the problem being addressed significant enough to warrant a change to Swift?
Absolutely. Initialising an Array out-of-order is an important feature, but until now it's required intermediate results and copies.
Does this proposal fit well with the feel and direction of Swift?
I think the API is as clear as it could be.
As for the direction of Swift - I hope a similar API comes to other types with a well-defined binary format like String one day. I think this kind of pattern could probably fit with String.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
C++'s std::vector has a (mutable, contiguous) data pointer, but AFAIK it cannot be used to reinitialise contents out-of-order or in a way which changes the number of elements.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Participated in discussion, read draft and final proposals, have wanted something like this for years (it's not top of the list, but it's definitely on there)..
+1. Though I would probably find few places to use this in the user-facing iOS apps that I write on a daily basis, those places where I do need this type of performance-sensitive code would be helped by this new method.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes. Performance gotchas on hidden collection copies can be crippling.
Does this proposal fit well with the feel and direction of Swift?
Yes. I pulled down the example code into a playground and it felt completely natural.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
The closest example I can think of is manually creating array buffers in C. The Swift version is definitely more verbose, but comes close to the feel of directly allocating the memory in C.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I read the proposal and downloaded the sample code into a playground, where I played with it a bit.
The proposed functionality is useful and much safer than the current workarounds, which require extensive use of unsafe APIs to obtain similar performance. I participated in the pitch thread and I'm very happy with how the proposal turned out. The main remaining sharp edge is the inout initializedCount, which is easy to misuse by forgetting to set it on some return path, but there doesn't seem to be an alternative that preserves the other useful aspects like rethrows (see the pitch thread for details). These methods are clearly labelled as unsafe, so I think having this sharp edge is acceptable.
I think it's a good design for a good idea. (I also think String might benefit from something like this, but that's somewhat more complicated and definitely severable.)
The one thing I would criticize: I'm not sure it's a good idea to make these members rethrowing. The "Guarantees after throwing" section reads like a hundred subtle memory safety bugs waiting to happen. Not making these members rethrow would force users to stop and think about their error handling, rather than blindly writing try and failing to catch the bug.
Is the problem being addressed significant enough to warrant a change to Swift?
Probably. There are places where I might have used this API if it had been available already.
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?
The most similar thing I can think of is the wonky -[NSData initWithBytesNoCopy:length:deallocator:] family of APIs. This approach seems vastly superior, since it doesn't complicate the management of the array's memory.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
A quick read. I didn't participate in earlier rounds of discussion.
To everyone who contributed to this thread, thank you for your feedback! I will update this thread with the core team's decision when it happens. Meanwhile, discussion and further feedback is still welcome until final core team review happens.
I will definitely benefit from the manual optimization opportunities enabled by this proposal, But:
I don't feel providing such manual optimization enablers is the right approach in general. I think we need deeper redesign of the compiler and standard library to reduce such bottlenecks and make them more reliably optimizable.
So, I welcome this addition, but I hope patching performance traps like this does not become the dominant approach in the language evolution.
I also second @beccadax's concern about rethrowing behavior.
The problem here, pointed out in the earlier discussion threads, is that you will want to use these methods to implement stablyPartitioned(by:) and similar rethrows methods, and the only way to work around this (i.e. prove to the compiler that you will only throw if the closure is throwing) is to exploit a compiler bug. Though, now that I look more closely at the implementation of stablyPartitioned(by:) in the proposal, it seems like it might violate its own rules around throwing since I don't see where it deinitialises the elements if belongsInFirstPartition throws. Am I missing something, @nnnnnnnn?
You're going to need a fairly heroic compiler to somehow realise that the straightforward (but slow) implementation of stablyPartitioned(by:), using only memory-safe APIs (e.g. filling two arrays then concatenating them), should be optimised to instead fill an array from both ends and meet at the right point in the middle. Nobody is proposing to add these methods instead of improving the optimiser, but there will always be cases that no reasonable optimisation will be able to handle.
Put simply: We need some kind of withActuallyRethrowing(_:do:) function anyway. We shouldnât design this API in a dangerous way because that API doesnât exist yet.
I should have put standard library first in my response. I think we can do better in the design and implementation of key currency types and protocols to improve optimizability without too frequently patching things up by enabling manual optimizations that become necessary because of the shortcomings of the current design (both at the API level and internal data structure level). I think memory-safe API can be achieved at lower cost with better design, but to get there we need to look at the whole jungle instead of focusing on individual trees.
Okay, sure, but I'm not sure I see how it would be much safer. If belongsInFirstPartition throws inside the initialiser you're still going to need to catch the error, put memory into some reasonable state, juggle the error out of the closure and return the initialised count, then withActuallyRethrowing the error. So the only differences are that you've made it harder to forget to set an accurate initialised count (by presumably making it a return vale of the closure) while adding a significant complexity around handling the error, including the use of a new unsafe API with a different possible failure mode (withActuallyRethrowing).
Because you can no longer use the obvious, instinctive, and wrong solution of âjust add try to get rid of the errorâ. If we could instead mark the closure so that it would need to contain a catch block rather than just propagating the error without additional logic, that might work too. But that would require a much larger and trickier language change.
This is, in my opinion, very well thought out and well written proposal that safely addresses an important performance deficit. The naming section deserves extra special kudos. Exemplary work @nnnnnnnn!
I don't have much to contribute for a formal review (which is good since it's past the formal review period), but I am also very much in favor of adding this. The times I've needed it have all been out-parameters of imported C functions, where I was forced to initialize the array elements to some dummy value before use. Which isn't terrible (the value is usually zero), but still feels wrong.
My one bit of discomfort is Brent's point about rethrows. I was previously on the "it's important for composability" side but the hypothetical future withActuallyRethrowing (or whatever) is a better answer to that problem. The trouble is we don't know how far future that is, and therefore it may still be worth including this API with the additional sharp edges.
I haven't followed this proposal in detail, so if this doesn't make sense just ignore me: The rethrowing should only be a problem with the method, not the initializer, right? Would a potential consuming-self-attribute (plus returning self alongside the result after the access is done) help here, so that the array is not accessible if the function didn't return normally?
If that's the case it may be worth holding back that part of the proposal until we have a consuming attribute, and include just the initializer in the meantime?