Update documentation of select_nth_unstable and select_nth_unstable_by to state O(n^2) complexity

[schuelermine]

See #102451

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon.

Please see the contribution instructions for more information.

[scottmcm]

Are you sure the current implementation is actually O(n²) worse-case? It looks to me like it's doing a bunch of work in pivot selection to avoid being quadratic: EDIT: I'm wrong, and should have read the issue.

rust/library/core/src/slice/sort.rs

Lines 692 to 716 in 4817259

	if len >= SHORTEST_MEDIAN_OF_MEDIANS {
	// Finds the median of `v[a - 1], v[a], v[a + 1]` and stores the index into `a`.
	let mut sort_adjacent = |a: &mut usize| {
	let tmp = *a;
	sort3(&mut (tmp - 1), a, &mut (tmp + 1));
	};
	// Find medians in the neighborhoods of `a`, `b`, and `c`.
	sort_adjacent(&mut a);
	sort_adjacent(&mut b);
	sort_adjacent(&mut c);
	}
	// Find the median among `a`, `b`, and `c`.
	sort3(&mut a, &mut b, &mut c);
	}
	if swaps < MAX_SWAPS {
	(b, swaps == 0)
	} else {
	// The maximum number of swaps was performed. Chances are the slice is descending or mostly
	// descending, so reversing will probably help sort it faster.
	v.reverse();
	(len - 1 - b, true)
	}

At least I think that any documentation update here should emphasize that this is average-case O(n), since that's the point of the function existing.

[schuelermine]

I'm relying on the statement that this is O(n²) worst-case from here: #102451

[scottmcm]

Oh, just saw the discussion in the issue. And sadly it's not only O(n²) for malicious Ord either :(

Yeah, guess this needs to change to talk about the average and the worst-case separately.

[schuelermine]

I noted it. I’m not sure if select_nth_unstable_by_key is also affected.

[schuelermine]

Perhaps a better option would be to rewrite the algorithm based on one of the methods linked in the issue.

[scottmcm]

Maybe as a first step here do the simple change to update the documentation just to change "worst-case" to "average"? Since the issue shows it's currently incorrect.

Then a follow-up PR could do the same fallback to heapsort that sort_unstable does to avoid being worse-cast O(n²).

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[schuelermine]

oh f***. I ran x.py, but that introduced more changes…

[cuviper]

I’m not sure if select_nth_unstable_by_key is also affected.

I think it must be -- e.g. you could use T::clone to get the exact same complexity as select_nth_unstable (plus cloning overhead).

[schuelermine]

OK, so I guess this is resolved now.

[cuviper]

The documentation is still incorrect -- it's now average O(n) and worst-case O(n log n).

[workingjubilee]

Technically, this breaks an API promise, but one we never fulfilled and really could not fulfill as it is not truly possible to use a "comparison sort" as implied by the T: Ord bound and get better than O(n log n). Thus I tagged it for T-libs-api but the implications of this PR are more philosophical than ones requiring a decision.

[Sp00ph]

It is definitely possible to get better than O(n log n) for comparison based selection, using something like median of medians which runs in O(n) worst case. Although it's slow enough that it's not worth it to always use it, we could definitely realistically use it as a fallback algorithm for our introselect, so we get the fast average case of quickselect but still guaranteed O(n)

[cuviper]

r? libs-api

[workingjubilee]

It is definitely possible to get better than O(n log n) for comparison based selection, using something like median of medians which runs in O(n) worst case.

Oh, fair enough! I actually thought of a few different other counterexamples but most of them had sufficient additional space complexity that it didn't seem worth mentioning, and after a few tens of minutes surveying things I was about ready to write them off.

Although it's slow enough that it's not worth it to always use it, we could definitely realistically use it as a fallback algorithm for our introselect, so we get the fast average case of quickselect but still guaranteed O(n).

Interesting. This seems like it'd be worth actually comparing the real performance on the options in some practical expected cases at various sizes, because it's not worth having O(n) if the O(n log n) beats it every time, but maybe the median-of-medians fallback would actually win out here.

[Sp00ph]

I wrote a (very unoptimized) median-of-medians implementation a few days ago, and IIRC, using an input that caused quadratic behavior before #106997, it was slower to use as the introselect fallback than heapsort on a slice of length 1 << 19. It's kind of difficult to benchmark this because just creating an input that will even cause introselect to use its fallback algorithm takes a long time for large sizes (see the playground link in #102451). I unfortunately deleted my code, but I can try writing a more optimized median-of-medians later and doing some proper benchmarking against heapsort. Another alternative would be to use Fast Deterministic Selection which seems to have competitive performance with heuristic based quick select and also guarantees O(n) worst case runtime. This would mean a lot less code reuse between the selection and sorting algorithms though.

[Sp00ph]

So I wrote this pretty simple median of medians implementation:

// the indices must all be in bounds and must not overlap.
// swaps elements around so the median of the 5 elements ends up in `v[c]`
unsafe fn median_of_five<T, F: FnMut(&T, &T) -> bool>(
    v: &mut [T],
    is_less: &mut F,
    a: usize,
    b: usize,
    c: usize,
    d: usize,
    e: usize,
) {
    let [a, b, c, d, e] = unsafe { v.get_many_unchecked_mut([a, b, c, d, e]) };
    let sort = |a: &mut T, b: &mut T, is_less: &mut F| {
        if is_less(b, a) {
            mem::swap(a, b);
        }
    };

    sort(a, c, is_less);
    sort(b, d, is_less);

    if is_less(c, d) {
        mem::swap(c, d);
        mem::swap(a, b);
    }

    sort(b, e, is_less);

    if is_less(c, e) {
        mem::swap(c, e);
        sort(a, c, is_less);
    } else {
        sort(b, c, is_less);
    }
}

pub fn select_linear<T, F: FnMut(&T, &T) -> bool>(mut v: &mut [T], is_less: &mut F, mut k: usize) {
    fn select_pivot<T, F: FnMut(&T, &T) -> bool>(v: &mut [T], is_less: &mut F) -> usize {
        debug_assert!(v.len() >= 5);

        let mut j = 0;
        let mut i = 0;
        while i + 4 < v.len() {
            unsafe { median_of_five(v, is_less, i, i + 1, i + 2, i + 3, i + 4) };
            unsafe { v.swap_unchecked(i + 2, j) };
            i += 5;
            j += 1;
        }

        select_linear(unsafe { v.get_unchecked_mut(..j) }, is_less, j / 2);
        partition(v, j / 2, is_less).0
    }

    loop {
        if v.len() <= 10 {
            insertion_sort(v, is_less);
            return;
        }

        let p = select_pivot(v, is_less);

        if p == k {
            return;
        } else if p > k {
            v = unsafe { v.get_unchecked_mut(..p) };
        } else {
            k -= p + 1;
            v = unsafe { v.get_unchecked_mut(p + 1..) };
        }
    }
}

and on my machine it becomes faster at computing the median than the stdlib heapsort at roughly 1 << 9 elements (on random input, but neither heapsort nor this algorithm depend a lot on the shape of the input). On very short inputs it looks to be roughly 2-3x slower. Using an index that's not close to the middle of the input slice also causes the performance to degrade. Do note that there are a ways to improve this implementation, of which some are described in the Fast Deterministic Selection paper.

[Sp00ph]

I also now implemented the fast deterministic selection algorithm here. The implementation is entirely based on this paper (and this repo) except that I couldn't be bothered to implement expand_partition, and instead just used the existing stdlib partition. It completely blows heapsort out of the water. Even on very short slices it isn't much slower than heapsort, and on longer slices it becomes way faster. Random inputs of length 1 << 16 take ~4ms to sort using heapsort on my machine, whereas my fast deterministic selection implementation only takes ~70µs to select the median. Also, there is no noticable runtime degradation when choosing an index further from the middle of the slice as there was with median of medians. Note also that I paid absolutely no attention to optimization yet, so it may very well become even faster if we were to eliminate unnecessary bounds checks and such. The implementation also doesn't introduce that much new code (~200 lines, though with comments and optimizations that number will obviously grow).

In its current form it is a bit slower than select_nth_unstable on random input (though that might of course change with added optimizations), so I wouldn't (yet) consider always using it for selection, but for a fallback for introselect it looks very promising imo.

[Sp00ph]

Is this even the right thread for this comment? Should I be posting it in the original issue thread instead?

[schuelermine]

I just wanted to update the documentation. It’s probably better to discuss this on the original issue.

[Amanieu]

Some wording nits, but otherwise this doc change looks good to me.

[schuelermine]

Updated the wording.

[Amanieu]

@bors r+ rollup

[bors]

📌 Commit f1e649b has been approved by Amanieu

It is now in the queue for this repository.