-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[stdlib] De-gyb internal sort functions #6638
Conversation
@swift-ci Please smoke test OS X platform |
@swift-ci Please test OS X platform |
@swift-ci Please benchmark |
Build comment file:Optimized (O) Regression (3)
Improvement (4)
No Changes (148)
Regression (5)
Improvement (3)
No Changes (147)
|
@swift-ci Please benchmark |
!!! Couldn't read commit file !!! |
@swift-ci Please benchmark |
!!! Couldn't read commit file !!! |
Benchmarks look promising – question is, do we have enough of them I wonder. |
The Phonebook test worries me a bit, since that's a test of sorting a custom |
@airspeedswift TBH, this may be a good opportunity to clean up our sorting benchmarks. To my mind, these are the axis that we care about:
My personal opinion is that for each "sortable" stdlib type, we should have a full suite of benchmarks like this. |
Hmmm. I simplified my benchmark so it would compile and I'm seeing a big slowdown (~11x) with just the de-gybbed code, even when passing a comparator closure, which shouldn't have changed at all. Test: https://gist.github.com/natecook1000/601463d7353114b047da54dd34d8c9dd
With de-gybbed version (8c4b543):
🤔 |
@gottesmm That's a really good idea 👍 |
@gottesmm other dimensions: size of input (small input will only call insertion sort, skip the introsort part completely), and how already-sorted the input is. Exhaustive combinations might be overkill given the number of possibilities (we don't want to start gybbing benchmarks 👹). Be good to start with a good range of some of the more common variations though. |
3645892
to
192a0dd
Compare
rdar://30641545 is tracking the performance problems with this. |
192a0dd
to
aeddff6
Compare
@swift-ci Please smoke test |
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
@swift-ci Please smoke test |
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
@swift-ci Please benchmark |
Build comment file:Optimized (O) Regression (5)
Improvement (6)
No Changes (171)
Regression (4)
Improvement (1)
No Changes (177)
|
So, based on the last round of benchmarks, we should rebase and merge this, right? Would help with binary size. |
cc @bob-wilson |
I rebase this as #9135 since things have changed quite a lot in the mean-time. |
Those benchmark results look OK to me. If anyone else has concerns, speak up! |
Thanks @airspeedswift! |
This de-gybs the internal sort functions
and converts them to extension methods on—conversion to extension methods can come in a separate change.MutableCollection