[stdlib][QoI] Replace recursion in sort _siftDown with Iteration #18629
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
Iterative approach removes recursion overhead. Therefore performance of sorting will be improved
|
@Overlazy Thanks for taking the time to contribute! I've tagged @airspeedswift as a reviewer. In the meantime, I can ask CI to take a benchmark. @swift-ci please smoke benchmark |
|
@harlanhaskins fyi, @airspeedswift is on vacation. You may want to tag someone else. |
|
Thanks @gottesmm! That’s ok I can still review this one. |
Build comment file:Optimized (O)Regression (15)
Improvement (10)
No Changes (420)
Hardware Overview |
airspeedswift
requested changes
Aug 12, 2018
stdlib/public/core/Sort.swift
Outdated
| let countToIndex = elements.distance(from: range.lowerBound, to: index) | ||
| let countFromIndex = elements.distance(from: index, to: range.upperBound) | ||
| // Check if left child is within bounds. If not, return, because there are | ||
| var index = index |
There was a problem hiding this comment.
I'd suggest using i instead of index. Really, all these free functions ought to be converted to be extensions on MutableCollection where Self: RandomAccessCollection, and if that refactoring happens then index with clash with all the same-named methods on self.
stdlib/public/core/Sort.swift
Outdated
| while countToIndex + 1 < countFromIndex { | ||
| let left = elements.index(index, offsetBy: countToIndex + 1) | ||
| var largest = index | ||
| if (try areInIncreasingOrder(elements[largest], elements[left])) { |
There was a problem hiding this comment.
tiny nit, and I realize you're imitating the style in the rest of the file (which I think predates 1.0): you don't have to put parentheses around conditions in an if.
stdlib/public/core/Sort.swift
Outdated
| elements.swapAt(index, largest) | ||
| index = largest | ||
| } else { | ||
| break |
There was a problem hiding this comment.
it would be nice if the variable updates could move up into the if so the break is the last line. It'd be even nicer if this list could be recast as a repeat/while so the break isn't necessary, but maybe that's not easily done.
|
Thanks for looking at this code @Overlazy! Looks like a lot of the code in this file is pretty out of date and badly needs some modernization by the look of it. Apart from some minor points, my main concern is that the benchmarks didn't show any difference because we aren't benchmarking input that triggers the heap sort. In the test suite, it looks like the only time this path gets exercised is when it's forced via this makeQSortKiller function. It'd be good to confirm if the benchmarks similarly force it, and if not, add some benchmarks (in another PR to merge first) to make sure we're testing the performance of this path before and after. I'd actually be pretty interested in whether this extra heap sort really brings a significant speed boost. Because we're paying a big price for it in terms of compilation and user code size – for |
|
@swift-ci please smoke benchmark |
|
@swift-ci please test |
Build comment file:Optimized (O)Regression (9)
Improvement (17)
No Changes (421)
Hardware Overview |
|
LLDB failure is unrelated. @swift-ci please test |
|
@airspeedswift , do we need a separate PR without heapSort to compare performance? |
airspeedswift
approved these changes
Aug 17, 2018
|
@Overlazy looks like it made a significant difference, good spot! Let's merge this, but yes, if you're interested, I definitely think it'd be worth seeing the impact of removing heap sort altogether. If you're interested in a bigger challenge – ideally we would switch to a stable sort instead. Most stable sorts involve allocating memory, but it would be an interesting project to see how much of a difference that makes. Especially for |
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.
stdlib sorting function uses recursion in it's supporting function called siftDown. We can replace recursion with iteration and reduce overhead.
@swift-ci please smoke benchmark