[stdlib] Switch to a stable sort algorithm #19717
Conversation
|
@swift-ci Please smoke benchmark |
|
@swift-ci Please test |
| @@ -146,9 +146,6 @@ where Self: RandomAccessCollection, Element: Comparable { | |||
| /// `Comparable` protocol by calling this method. Elements are sorted in | |||
| /// ascending order. | |||
| /// | |||
| /// The sorting algorithm is not stable. A nonstable sort may change the | |||
There was a problem hiding this comment.
I'd suggest leaving this in for now. If this sort is better than the current one, we can just land it as an implementation improvement. But officially committing to stability needs an evolution proposal.
There was a problem hiding this comment.
Well, it might be misleading to say it's "not stable" when it is: I'd suggest maybe wording it something like "is not guaranteed to be stable" or "may or may not be stable," or maybe explicitly, "It is an implementation detail whether the sorting algorithm is stable."
There was a problem hiding this comment.
Yeah, "not guaranteed stable" is the right phrasing.
|
@swift-ci please test compiler performance |
Build comment file:Build failed before running benchmark. |
|
Looks like benchmarking blew up during SortStrings |
|
Build failed |
|
@swift-ci Please smoke benchmark |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR. Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein). Hardware Overview |
stdlib/public/core/Sort.swift
Outdated
| // Move y forward | ||
| self[i] = predecessor | ||
|
|
||
| // Swap the elements at `i` and `j`. |
There was a problem hiding this comment.
😅 I think I was just excited about not using the temporary any more
| // 1 swap: 132, 121 --- 2 swaps: 231, 221 | ||
| // swap(b, c): 132->123, 121->112, 231->213, 221->212 | ||
| swapAt(b, c) | ||
| return true |
There was a problem hiding this comment.
Doesn't look like there are any other returns here. So perhaps the whole -> Bool is not needed?
There was a problem hiding this comment.
Definitely not needed — it's a workaround for an optimizer issue. Those can all come out once that's resolved.
There was a problem hiding this comment.
optimizer issues also suggest the benchmarks could improve once resolved...
stdlib/public/core/Sort.swift
Outdated
| if c < 1 << bitsToUse { | ||
| return c | ||
| } | ||
| let offset = (MemoryLayout<Int>.size * 8 - bitsToUse) - c.leadingZeroBitCount |
There was a problem hiding this comment.
Isn't MemoryLayout<Int>.size * 8 equal to Int.bitWidth?
stdlib/public/core/Sort.swift
Outdated
| if next < elements.endIndex { | ||
| var current = next | ||
| elements.formIndex(after: &next) | ||
| if try next < elements.endIndex && |
There was a problem hiding this comment.
Can we extract this next < elements.endIndex into a guard, and then replace the else if few lines below with just an else?
There was a problem hiding this comment.
Pulling this out allowed a nice refactor, which handled the next comment (about the repeats) too.
stdlib/public/core/Sort.swift
Outdated
| } else if next < elements.endIndex { | ||
| // The elements in this run are in ascending order. Equal elements can | ||
| // be included in the run. | ||
| repeat { |
There was a problem hiding this comment.
This repeat ... while duplication is annoying. Would extracting it into a nested function affect performance?
stdlib/public/core/Sort.swift
Outdated
| break Loop | ||
| } else if runs[lastIndex - 1].count <= runs[lastIndex].count { | ||
| // Last two runs do not follow Y > Z. | ||
| // Merge Y and Z. |
There was a problem hiding this comment.
A comment Merge Y and Z. looks very scary in absence of any code following it. What would you say about adding // Intentionally left blank. kind of comment here as well?
stdlib/public/core/Sort.swift
Outdated
| } | ||
|
|
||
| if !result { fatalError() } |
There was a problem hiding this comment.
Isn't it the same as precondition(result)?
test/Prototypes/IntroSort.swift
Outdated
| // RUN: %target-run %t/a.out | ||
| // REQUIRES: executable_test | ||
|
|
||
| #if USE_STDLIBUNITTEST |
There was a problem hiding this comment.
Is this really needed? If so, the same guard should wrap around the actual test cases and a call to runAllTests.
| func expectSortedCollection<C: Collection>(_ a: C, | ||
| by areInIncreasingOrder: (C.Element, C.Element) -> Bool | ||
| ) { | ||
| expectFalse(zip(a.dropFirst(), a).contains(where: areInIncreasingOrder)) |
There was a problem hiding this comment.
Of course you meant a.lazy.dropFirst() :-)
There was a problem hiding this comment.
🤔 Would that make any difference here?
There was a problem hiding this comment.
I don't think so. It's just that I seem to have developed this habit of trying to apply .lazy everywhere and see if that'd work. Works in this case. Will perhaps make the test a millisecond faster.
|
This is great, @natecook1000! 👏 👏 👏 |
| @@ -529,6 +528,8 @@ extension UnsafeMutableBufferPointer { | |||
|
|
|||
| runs[i - 1] = low..<high | |||
| runs.remove(at: i) | |||
|
|
|||
| // FIXME: Remove this, it works around rdar://problem/45044610 | |||
|
@swift-ci Please smoke test |
natecook1000
force-pushed
the
nc-stable-sort
branch
from
9c326ce
to
9285780
Compare
|
@swift-ci Please smoke test |
natecook1000
force-pushed
the
nc-stable-sort
branch
from
9285780
to
b7120e8
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
This switches the standard library's sort algorithm from an in-place introsort to use a modified timsort, a stable, adaptive sort that merges runs using a temporary buffer. This implementation performs straight merges instead of adopting timsort's galloping strategy. In addition to maintaining the relative order of equal/non-comparable elements, this algorithm outperforms the introsort on data with any intrinsic structure, such as runs of ascending or descending elements or a significant number of equality collisions.
See rdar://45044610
This drops the new MutableCollection requirement in favor of using only _withUnsafeMutableBufferPointerIfSupported to dispatch the sort algorithm to a mutable buffer. This in turn requires that UnsafeMutableBufferPointer implements _withUnsafeMutableBufferPointerIfSupported, so that change is also included here (and makes more intuitive sense than a buffer not supporting buffer operations).
natecook1000
force-pushed
the
nc-stable-sort
branch
from
48d5b39
to
2b063df
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
|
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test OS X Platform |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux platform |
2 similar comments
|
@swift-ci Please smoke test Linux platform |
|
@swift-ci Please smoke test Linux platform |
This switches the standard library's sort algorithm from an in-place introsort to use a modified timsort, a stable, adaptive sort that merges runs using a temporary buffer. This implementation performs straight merges instead of adopting timsort's galloping strategy.
In addition to maintaining the relative order of equal/non-comparable elements, this algorithm outperforms the introsort on data with any intrinsic structure, such as runs of ascending or descending elements or a significant number of equality collisions. Benchmarking script and results can be seen in this gist: https://gist.github.com/natecook1000/5161e10aeba09408c130284ea6ec4e11