[Stdlib] Improves Collection.sort to accept throwing closure #1527
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are two tests which are failing.
I need help to fix these test cases. |
|
@codestergit Thank you! Two requests from me.
|
|
Please Fix conflicts |
|
There are lot of conflicts because of swift 3 changes. I am redoing my changes.I will merge the changes soon by addressing comments :) |
|
I'm Happy to hear that!! 2016-03-22 11:27 GMT+01:00 codester [email protected]:
|
codestergit
changed the title
|
First of all I am really sorry for the delay. I got some urgent work and need to go out of station so this got delay. @gribozavr @dabrahams any suggestions how to handle this or is this required behaviour?? |
test/1_stdlib/ErrorHandling.swift
Outdated
| print("collection before error \(collection)") | ||
| print("a \(a) \(b)") | ||
|
|
||
| if b == 2 { |
There was a problem hiding this comment.
With this case sort mutates the collection to [2, 3, 3]. insertionSort not able to complete the loop and swap elements so we are loosing the elements from original collection.
There was a problem hiding this comment.
This behavior is OK. See https://en.wikipedia.org/wiki/Exception_safety
There was a problem hiding this comment.
I find losing elements surprising though. Wouldn't we want to ensure that we don't lose elements as a matter of QoI for this API specifically?
There was a problem hiding this comment.
@gribozavr It throws here and swap operation not able to complete. We also do partition of elements if elements are above 20. It may duplicate elements and loose.
There was a problem hiding this comment.
I see. Can we catch the error, store the elements back in arbitrary order, and re-throw?
There was a problem hiding this comment.
Yes, and in 99% of cases a partly sorted collection is useless to you and you have no alternative way to do the same sort, so in 99% of cases you’ll be sorting something you’re willing to lose, like a copy of the original collection.
By the way, if you did have a fallback for a failing comparison, you’d just make your comparison fall back to it rather than aborting the sort and starting over again.
There was a problem hiding this comment.
That's what I was thinking too, and so I couldn't think of why you'd want to have a throwing comparator at all. But I can imagine wanting to have the fallback be consistent: either everything sorts using the "good" comparison, or everything sorts using the fallback.
There was a problem hiding this comment.
Yes, this is all theoretically possible but highly unlikely and not worth designing for. It’s reasonable to do something as QOI but the precedent set by documenting the behavior would significantly complicate what people need to do to reason about (and document their own) error handling.
There was a problem hiding this comment.
If it's not worth designing for, I'm not sure why it's worth implementing. If it's there, someone will start depending on it.
There was a problem hiding this comment.
on Fri Apr 22 2016, Jordan Rose <notifications-AT-i.8713187.xyz> wrote:
In test/1_stdlib/ErrorHandling.swift:
@@ -340,4 +340,42 @@ ErrorHandlingTests.test("ErrorHandling/Collection map") {
}
}+/* when b == 2 we are losing elements. insertion sort throws error
+in between while sorting so loop is not able to complete and swap the elements and array mutates
+to [2,3,3]
+ErrorHandlingTests.test("ErrorHandling/sort") {
- var collection = [3, 2, 1]
- do {
- try collection.sort { (a, b) throws -> Bool in
print("collection before error (collection)")print("a (a) (b)")
if b == 2 {If it's not worth designing for, I'm not sure why it's worth implementing. If
it's there, someone will start depending on it.
It will prevent one or two naive users from having a subtle bug, it will
prevent us from fielding/rejecting a long stream of bug reports from
people who think they should be able to rely on it, and I don't mind
people relying on it. What I don't want, at least not without
significantly more consideration, is to formalize this as part of how we
describe error handling behavior.
Dave
|
on Thu Apr 21 2016, codester <notifications-AT-i.8713187.xyz> wrote:
There's nothing wrong with that. The only requirement of an operation Dave |
|
@dabrahams @gribozavr Updated the pull request. Please review it. I have caught the error here and thrown it again. I have tried to test this thoroughly and it passed the tests. We do partition and sort with greater than 20 elements.. So checking all permutations with more than 20 elements was not feasible so I changed the algorithm locally and tried to do partition and sort with all permutations of 5 element sequence and it passed all the tests. |
|
@codestergit I'm sorry that this fell by the wayside. If you can rebase and fix the merge conflicts could you please do so. Else I can take over this PR. |
|
@CodaFi Thanks, I will rebase and update the pull request soon. |
codestergit
force-pushed
the
master
branch
3 times, most recently
from
b2ef055 to
e86fb6e
Compare
|
@CodaFi @gribozavr |
codestergit
changed the title
dabrahams
reviewed
Jan 29, 2017
| @@ -269,6 +269,9 @@ extension ${Self} { | |||
| /// Returns the elements of the ${sequenceKind}, sorted using the given | |||
| /// predicate as the comparison between elements. | |||
| /// | |||
| /// This method can take throwing closure. If closure throws error while | |||
| /// sorting, it will return empty collection. | |||
There was a problem hiding this comment.
This seems like the wrong behavior. Why isn't the error simply propagated outward (in which case nothing is actually returned)?
There was a problem hiding this comment.
Sorry I made mistake(wrongly documented) here. It will throw error. I will correct it.
Thanks
There was a problem hiding this comment.
@dabrahams Removed comment. Thanks.
Please let me know if any other changes required.
dabrahams
reviewed
Jan 29, 2017
| break | ||
| } | ||
| } catch { | ||
| elements[i] = x |
There was a problem hiding this comment.
Why bother doing the above? Isn't elements about to be discarded?
There was a problem hiding this comment.
No, If we are mutating a collection with sort(by:) than collection has been changed and at this line we are trying to swap the elements.
For eg: if I have array
var a = [ 5, 4, 3 ,2, 1]
if we throw at b == 4 than sort(by:) mutate the collection to
[4, 5, 5, 2, 1] //3 is lost here
as the last step
if i != sortedEnd {
// Plop x into position
elements[i] = x
} not able to swap the element and we thrown the error in between. So that is why this step is required and we are catching the error and throwing it again.
It just take care of no elements will lost and add. Elements of a may reorder .
|
on Sun Jan 29 2017, codester <notifications-AT-i.8713187.xyz> wrote:
So that is why we are catching the error and throwing it again. It
just take care of no elements will lost and add.
OK, that is a nice QOI feature for algorithms that only permute the
elements (provided it doesn't hurt performance), but keep in mind that
in general it's not something we can guarantee for mutating algorithms
that rethrow.
|
@dabrahams Yes, I agree with you. Previously @gribozavr suggested that we should try to handle this. If you would like I can remove this documentation .
|
airspeedswift
changed the title
|
This PR conflicts with @natecook1000’s one (#6638) to de-gyb sorts. Does it make sense to combine them or should one go before the other? |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@airspeedswift Thanks for running tests. Why error by build bot. Is something wrong ?
@natecook1000 @airspeedswift @dabrahams I think we should merge them separately. We can work one above another. It got conflict three times previously 😢. I hope it will merge this time 😁 😊 😄. |
|
If we do end up getting to the point where we need to debug generated code for performance, this makes it more worthwhile to combine this + the de-gybbed PR, since it also had unexpected performance implications. |
Build comment file:Optimized (O) Regression (10)
Improvement (2)
No Changes (157)
Regression (5)
Improvement (4)
No Changes (160)
|
|
Hi @codestergit – the hope is that PR #7672 should resolve some of these performance issues. Do you think you could rebase to pull it in, then we can rebenchmark? |
|
@airspeedswift Sure I will do it by tomorrow. |
|
@airspeedswift @dabrahams Rebased the pull request. Please run the benchmark. I have commented the do/catch block for now. I will uncomment it after benchmarks to see if it is impacting performance. Some of the validation test case will fail because of this. |
|
@swift-ci Please benchmark |
Build comment file:Optimized (O) Regression (1)
Improvement (1)
No Changes (170)
Regression (6)
Improvement (2)
No Changes (164)
|
|
Much better! @codestergit want to try putting that do/catch back in? |
|
@airspeedswift @dabrahams Great!! |
|
@swift-ci Please benchmark |
Build comment file:Optimized (O) Regression (3)
Improvement (3)
No Changes (166)
Regression (2)
Improvement (3)
No Changes (167)
|
|
@swift-ci Please benchmark |
|
@rintaro Thanks for running benchmarks. @airspeedswift @dabrahams |
|
Let's run full test. |
|
Build failed |
|
Build failed |
|
Linux test failed with unrelated test case (Syntax) |
Build comment file:Optimized (O) Regression (3)
Improvement (3)
No Changes (166)
Regression (2)
Improvement (2)
No Changes (168)
|
|
@airspeedswift |
|
@dabrahams Updated the pull request to resolve conflicts. |
|
@swift-ci Please smoke test |
|
@codestergit thanks for all your work on this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in this pull request?
This pull request improves
MutableCollectionType.sort,MutableCollectionType.sortInPlaceand other sort related functions to take throwing clousre.Resolved bug number: (SR-715)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Note: Only members of the Apple organization can trigger swift-ci.
This commit resolves https://bugs.swift.org/browse/SR-715