TimSort #12
TimSort #12
Conversation
There was a problem hiding this comment.
The source looks ok.
What's the Jamfile change about?
We'll need tests, a benchmark, (can be simple), and a mini-review of the code before we can submit this. It'll also be necessary to add this to the documentation (in quickbooks) but that should be fairly simple with just one library call.
My thought is that once you have the tests + benchmark, we can run a mini-review, but should wait on merging it in until the parallel library has finished merging.
|
Ok. I will add tests, benchmarks, etc. in a few days. |
Added range support for spreadsort, integer_sort, float_sort, string_sort.
| which in testing tends to be roughly 50% to 2X faster than @c std::sort for large tests (>=100kB).\n | ||
| \par | ||
| Worst-case performance is <em> O(N * (lg(range)/s + s)) </em>, | ||
| so @c integer_sort is asymptotically faster |
There was a problem hiding this comment.
Thanks for catching the integer_sort references, but you missed one here.
There was a problem hiding this comment.
I get compile errors when I grab this. Does this use C++11, and if so, is it really necessary? Not everyone has a C++11 compiler yet (I can build C++11, but deliberately use the default b2 for testing).
gcc.compile.c++ ../../bin.v2/libs/sort/test/timsort.test/gcc-5.4.0/debug/timsort_test.o
In file included from test/timsort_test.cpp:9:0:
../../boost/sort/timsort.hpp:45:7: error: using template type parameter ‘Compare’ after ‘class’
class Compare
^
../../boost/sort/timsort.hpp:97:13: error: ‘Compare’ is not a template
typedef Compare<const value_t &, Compare> compare_t;
^
../../boost/sort/timsort.hpp: In static member function ‘static void boost::sort::TimSort<RandomAccessIterator, Compare>::binarySort(boo
st::sort::TimSort<RandomAccessIterator, Compare>::iter_t, boost::sort::TimSort<RandomAccessIterator, Compare>::iter_t, boost::sort::TimS
ort<RandomAccessIterator, Compare>::iter_t, boost::sort::TimSort<RandomAccessIterator, Compare>::compare_t)’:
../../boost/sort/timsort.hpp:170:39: error: ‘move’ is not a member of ‘std’
/const/ value_t pivot = std::move(*start);
^
../../boost/sort/timsort.hpp:175:22: error: ‘move’ is not a member of ‘std’
p = std::move((p - 1));
Replaced std::move and std::move_backward by boost::move and boost::move_backward.
|
Thank you for catching this. My bad, sorry for this stupid mistake. Currently we can compile timsort.hpp with -std=c++03 and -std=c++98. |
There was a problem hiding this comment.
Thanks for the fixes.
The mostly-sorted benchmark numbers were really impressive! It's 10+X faster than even pdqsort in that case. It's slower in the random input case, as is expected.
It'd be interesting to write a chunk-aware spreadsort based on what timsort takes advantage of, but I don't have the time.
Let me know when you're ready for a mini-review, and a 10-day time period when you can answer all questions about timsort promptly, and I'll try to get this on the review schedule. Note: I will expect you to maintain the timsort portion of the library and be responsive to email after the library is submitted.
|
Ok. Thank you for your help. Currently i have a lot of work, so i think will be great, if we will start review in 2-3 weeks (e.g. on 10 February). I will be ready to answer all questions about TimSort. What do you think about this idea? All can talk with me by GitHub (most oreferred way), e-mail ([email protected]), or IRC #boost (nickname: zamazan4ik, zamazan4ik_). My timezone: UTC +3 (Belarus, Minsk). |
|
I'm still in discussions with Francisco, who wrote the comparison-based half of the Boost.Sort library that he's in the process of integrating into the whole. He is the logical person to run the mini-review if he has the time. Once that's sorted out we'll figure out when is best for a mini review and whether he requires more changes first. |
|
Ok. Important information: on 1-12 February i will be at hospital, so i haven't good possibilities to participate in mini-review. After 12 February i will be ready. |
|
Hello. What is the status of the review? |
|
On Mon, Mar 13, 2017 at 8:47 AM Alexander ***@***.***> wrote:
What is the status of the review?
Francisco (my new co-author as of late last year) wanted to review Timsort,
and found that it didn't perform quite as well as he expected but still
provides benefit if the vast majority of the array is sorted. We're still
sorting out whether he will review it or I will review it; I am giving him
the option to do so if he wants.
… —
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AE2j3MmqL8cGL0PKRN_FmI9ENTz6hQQOks5rlWTzgaJpZM4K9zQW>
.
|
|
Thank you a lot :) |
|
I added your range wrapper code changes to develop (including the attribution you added and the documentation error corrections), as those aren't controversial. I still haven't heard back from Francisco, and if he doesn't get back I'll run the review myself. |
|
Hello @spreadsort . What the review status? |
|
The review is starting tomorrow. I've emailed you and the boost list about it. Please be responsive to questions. |
|
I collected some useful links for potential reviewers: |
|
Please answer the questions on the boost list directly. |
|
My email is [email protected]. Please email me directly. If I'm to accept your source, I need to be able to contact you to discuss it and have the assurance that you'll fix bugs in it. |
This is initial version of TimSort. Can you do review, please?