Add a sort method to &mut [T]

[huonw]

This uses quite a bit of unsafe code for speed and failure safety, and allocates 2*n temporary storage.

Performance:

n	new	priority_queue	quick3
5	200	155	106
100	6490	8750	5810
10000	1300000	1790000	1060000
100000	16700000	23600000	12700000
sorted	520000	1380000	53900000
trend	1310000	1690000	1100000

(The times are in nanoseconds, having subtracted the set-up time (i.e. the just_generate bench target).)

I imagine that there is still significant room for improvement, particularly because both priority_queue and quick3 are doing a static call via Ord or TotalOrd for the comparisons, while this is using a (boxed) closure.

Also, this code does not clone, unlike quick_sort3; and is stable, unlike both of the others.

[huonw]

This uses a lot of unsafe code, but I don't see a way around it to maintain efficiency.

The code there is essentially an optimised version of the following (which is several times slower):

fn merge_sort_safe<T>(v: &mut [T], less_eq: |&T, &T| -> bool) {
    if v.is_empty() { return }
    let len = v.len();
    let mut idx: ~[uint] = vec::with_capacity(len);

    for start in iter::range_step(0, len, INSERTION) {
        for i in range(start, cmp::min(start + INSERTION, len)) {
            let mut j = i as int;
            while j > start as int && !less_eq(&v[idx[j - 1]], &v[i]) {
                j -= 1;
            }
            idx.insert(j as uint, i);
        }
    }

    let mut width = INSERTION;
    let mut next_idx = vec::with_capacity(len);
    while width < len {
        for start in iter::range_step(0, len, 2 * width) {
            // the end of the first run/start of the second
            let left_end = cmp::min(start + width, len);
            // end of the second
            let right_end = cmp::min(start + 2 * width, len);
            let mut left = start;
            let mut right = left_end;
            for _ in range(start, right_end) {
                let choose_left = left < left_end && (right >= right_end ||
                                                      less_eq(&v[idx[left]], &v[idx[right]]));
                next_idx.push(if choose_left {
                    let l = idx[left];
                    left += 1;
                    l
                } else {
                    let r = idx[right];
                    right += 1;
                    r
                })
            }
        }
        swap(&mut idx, &mut next_idx);
        next_idx.truncate(0);
        width *= 2;
    }

    // trickery to put all the elements in the correct place.
    let mut inverse_idx = next_idx;
    inverse_idx.push_all(idx); // get the appropriate length; we overwrite it later.
    for (i, &j) in idx.iter().enumerate() {
        inverse_idx[j] = i
    }

    for i in range(0, len - 1) {
        let idx_i = idx[i];
        if idx_i == i { continue }
        v.swap(i, idx_i);
        idx.swap(i, inverse_idx[i]);
        inverse_idx.swap(i,idx_i);
    }
}

[alexcrichton]

A few high-level questions before I dive too deep into the code:

1. Have you considered other algorithms? The one that comes to mind for me is quicksort which I don't think needs the temporary storage. I also haven't implemented it in a serious setting, so I can't really speak to how reasonable it is to expect it.
2. At least in ruby the sort() method takes no arguments, but sort_by() takes a closure for comparing to elements. I think there's also a flavor that takes a closure which maps an element to a sortable value. I don't think that we want to get too fancy with this, but it may be worth considering having the sort() method be the convenience method and have an extra one for the sort-by-closure method.

Other than that, awesome work! At a quick glance this looks pretty awesome and I'm loving that diff stat.

[thestinger]

@alexcrichton: quicksort has an O(n^2) worst-case without slowing it down by making it use median-of-median pivot selection and isn't a stable sort

[huonw]

Have you considered other algorithms?

I was just going off @thestinger's suggestion of merge sort + insertion sort on #9819.

At least in ruby the sort() method takes no arguments, but sort_by() takes a closure for comparing to elements

I'm happy to do this, I'm also currently experimenting with:

pub trait LessThanEqualComparator<T> {
    fn less_eq(&self, a: &T, b: &T) -> bool;
}

pub struct SortForward;
impl<T: Ord> LessThanEqualComparator<T> for SortForward {
    fn less_eq(&self, a: &T, b: &T) -> bool { *a <= *b }
}

pub struct SortReverse;
impl<T: Ord> LessThanEqualComparator<T> for SortReverse {
    fn less_eq(&self, a: &T, b: &T) -> bool { *b <= *a }
}

impl<'a, T> LessThanEqualComparator<T> for 'a |&T, &T| -> bool {
    fn less_eq(&self, a: &T, b: &T) -> bool {
        (*self)(a, b)
    }
}

[...]

    fn sort<Sort: LessThanEqualComparator>(self, less_eq: Sort);

which would avoid the boxed closure problem for a plain a <= b sort, but still be flexible, and avoid writing the error prone |a,b| a <= b repeatedly. I have a feeling that this will require type annotations on the closure, which may be too ugly (and it would require importing std::vec::SortForward etc, so the sugary .sort() method might still be nice).

[huonw]

I've added a commit that does the above suggestion, it does require putting type signatures on the closures, which is fairly annoying. :( (I'm happy to drop it if necessary.)

However, this adjusts the times to

n	original	trait
5	200	155
100	6490	5900
10000	1300000	1280000
100000	16700000	16400000
sorted	520000	310000
trend	1310000	1250000

If it were split into .sort() and .sort_by(<comparator>) this might not be so ugly in practice.

[huonw]

And added one for .sort() and .sort_by(..). Unfortunately that requires adding a whole new trait and adding it to the prelude. :(

[alexcrichton]

@brson and I were talking about this this morning, and here's some thoughts that we had (plus a few of my own with these updates).

1. There are 3 flavors of sorting that I know of. sort() which requires T: Ord, sort(|a| { ... }) which maps elements to a orderable-type, and sort(|a, b| { ... }) which compares two elements directly. I think that sort() should definitely be available (as is today), and sort(|a, b| { ... }) is also quite useful (as you have right now), and I'm not entirely sure if we want sort(|a| { ... }) for now. I think we can get by without it.
2. The standard sort should be super fast. It appears that other programming languages don't necessarily put a huge focus on having the default sort being a stable sort. I personally have never cared about my sort being stable, but there are very real use cases which require this. Perhaps a good solution for this would be to have extra::sort with a fast stable sort (and maybe some other flavors), but the std::sort variant would be a super-fast explicitly not guaranteed to be stable sort. @brson mentions that the "hot sorting algorithm" today is timsort, which we apparently have an implementation of in libextra (but may need to be optimized).
3. I think that the api should be a little different. I always forget what order I need to do things in the closure. Should I return less than, less than or equal, or the inverse of these? You solve this well with a less_eq method (so it's clear), but this seems like yet another trait which isn't that necessary. It seems to be that a closure should be perfectly sufficient, but the return value of the closure should be Ordering. I still personally don't understand the difference between Ord and TotalOrd, but the basic idea that I would like to see is just returning the relative comparison between the two elements, and then the array is always sorted smallest to largest. We could have convenient ordering.invert() methods for inverting how the array is sorted.

How does that sound?

[huonw]

@brson mentions that the "hot sorting algorithm" today is timsort, which we apparently have an implementation of in libextra (but may need to be optimized).

It definitely does; this algorithm here is much faster in general that the timsort in extra (although timsort is amazing on pre-sorted lists):

n	original	trait	timsort
5	200	155	236
100	6490	5900	24000
10000	1300000	1280000	2810000
100000	16700000	16400000	37000000
sorted	520000	310000	15000
trend	1310000	1250000	2770000

this seems like yet another trait which isn't that necessary. It seems to be that a closure should be perfectly sufficient

The point of the trait is to get static dispatch for a simple a <= b comparison since this is faster (see the table above); (I agree that it would be unnecessary if we had unboxed stack closures).

the return value of the closure should be Ordering

Sounds good.

Re stability and the exact choice of algorithm, you'll have to talk to @thestinger, since I'm just following his suggestion on #9819.

[huonw]

I'm reading over the tim_sort code in extra now... there is a lot of it (560 LOC for the implementation, not including any tests or anything) and it looks fairly complicated; I imagine that doing it with unsafe code would be hard to verify as correct, and will be hard to get to be as efficient as this (especially when avoiding .clone).

[brson]

This looks very nice, @huonw. Thanks.

[brson]

Well commented.

[huonw]

Pushed the less_eq -> Ordering change. It makes some things ugly since it then requires TotalOrd which makes handling (for example) floats less nice (cc #5585 and #10320).

[alexcrichton]

@brson/@pcwalton and I were talking about this, and we're a little wary of the Sort: SortComparator<T> bound. It should always be the case that arr.sort_by(|a, b| a.foo.cmp(&b.foo)) works, but in the future this will likely be done with an unboxed closure rather than a trait bound. For forward compatibility, we think that for now the argument should just be a closure. If we have a trait bound, code will eventually break because it relied on using the trait rather than a function-like thing which we're a little worried about.

I realize that it's a little slower, but I would be more worried about forwards compatibility. What do you think?

[pnkfelix]

@alexcrichton Your comment, "There are 3 flavors of sorting that I know of ..." (which then listed three flavors, at least two of which were comparison-based sorts, not sure about the third) reminded me that I have been meaning to go back over this paper on Generic Discrimination based Sorting/Partitioning [1] [2], which I saw presented some years ago but never got a chance to really play with.

(The idea, if I recall correctly, is a generalization of the ideas that drive a radix-sort to more general data structures, yielding "linear time" sorting, where I've quoted linear time because I'm sure there are certain assumptions that must be met.)

[1] http://www.diku.dk/hjemmesider/ansatte/henglein/papers/henglein2007b.pdf

[2] http://www.icfpconference.org/icfp2008/accepted/41.html

[huonw]

Rebased out the trait addition (I agree with the backward-compatibility concerns).

(I'm "away" at the moment, so iteration of this PR will be slow.)