Implement partial_sort_unstable for slice #149318

tisonkun · 2025-11-25T15:36:46Z

This refers to #149046.

rustbot · 2025-11-25T15:36:51Z

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

library/core/src/slice/mod.rs

tisonkun · 2025-11-25T17:58:23Z

cc @orlp

orlp

Left some remarks, some on style, but also some with substance.

Besides comments on the code that's written, I do note a lack of tests?

View changes since this review

library/core/src/slice/mod.rs

tisonkun · 2025-11-26T02:14:12Z

I do note a lack of tests?

Doc tests cover most branches. I don't find a dedicated file to cover its cousin sort_unstable. If you can point me to one, I'm glad to add cases there.

orlp · 2025-11-26T07:29:05Z

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

tisonkun · 2025-11-26T07:37:28Z

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

Thanks and yes. Do you know where the unit tests of sort/sort_unstable locate?

orlp · 2025-11-26T07:41:01Z

I believe the bulk is found in https://github.com/rust-lang/rust/blob/main/library/alloctests/tests/sort/tests.rs.

orlp · 2025-11-26T09:39:57Z

What I suggested in the ACP was a sketch implementation, I did some more thinking and I think the following handles all corner cases nicely:

pub fn partial_sort<T, F, R>(mut v: &mut [T], range: R, is_less: &mut F)
where
    F: FnMut(&T, &T) -> bool,
    R: RangeBounds<usize>,
{
    let len = v.len();
    let Range { start, end } = slice::range(range, ..len);
    
    if end - start <= 1 {
        // Can be resolved in at most a single partition_at_index call, without
        // further sorting. Do nothing if it is an empty range at start or end.
        if start != len && end != 0 {
            sort::select::partition_at_index(v, start, is_less);
        }
        return;
    }
    
    // Don't bother reducing the slice to sort if it eliminates fewer than 8 elements.
    if end + 8 <= len {
        v = sort::select::partition_at_index(v, end - 1, is_less).0;
    }
    if start >= 8 {
        v = sort::select::partition_at_index(v, start, is_less).2;
    }
    sort::unstable::sort(v, is_less);
}

And to formalize the post-conditions, I think the following should hold after a call to v.partial_sort_unstable(b..e):

for i in 0..b {
    for j in b..n {
        assert!(v[i] <= v[j]);
    }
}
for i in 0..e {
    for j in e..n {
        assert!(v[i] <= v[j]);
    }
}
for i in b..e {
    for j in i..e {
        assert!(v[i] <= v[j]);
    }
}

quaternic · 2025-11-28T05:31:13Z

And to formalize the post-conditions, I think the following should hold after a call to v.partial_sort_unstable(b..e):

A lot of those individual comparisons are implied by transitivity of the ordering, so it can be reduced to choosing the maximum of the prefix (if any), the minimum of the suffix (if any), and then asserting that the concatenation is sorted.

Informally, max(v[..b]) <= v[b] <= v[b + 1] <= ... <= v[e-1] <= min(v[e..]), or in code:

let max_before = v[..b].iter().max().into_iter();
let sorted_range = v[b..e].iter();
let min_after = v[e..].iter().min().into_iter();
let seq = max_before.chain(sorted_range).chain(min_after);
assert!(seq.is_sorted());

That's pretty much what you said in rust-lang/libs-team#685 (comment) , just using transitivity of the comparison. Without assuming that, the implementation couldn't guarantee the universally quantified property anyway.

rustbot · 2025-12-01T04:09:16Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

tisonkun · 2025-12-01T04:10:27Z

Pushed a new implementation.

I'm writing tests but perhaps we'd have a new mod under library/alloctests/tests/partial_sort rather than patch the existing library/alloctests/tests/sort/tests.rs. Since the sort tests are heavily depending on macros while partial sorts assertions are quite different.

tisonkun · 2025-12-01T04:11:26Z

Pushed a new implementation.

I'm writing tests but perhaps we'd have a new mod under library/alloctests/tests/partial_sort rather than patch the existing library/alloctests/tests/sort/tests.rs. Since the sort tests are heavily depending on macros while partial sorts assertions are quite different.

cc @Amanieu for early review for the direction and advice on where to organize the tests.

library/core/src/slice/sort/unstable/mod.rs

Signed-off-by: tison <wander4096@gmail.com> Co-Authored-By: Orson Peters <orsonpeters@gmail.com>

library/core/src/slice/sort/unstable/mod.rs

Amanieu · 2025-12-02T03:24:49Z

Regarding the tests I'm happy with either a separate file or as part of the existing tests as you see fit.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2025-12-04T06:43:50Z

Pushed some basic test cases at 01bfcc0.

The existing sort tests have many assumptions, like checked_sort always sorts the whole range (for sure), and this helper is tightly coupled in cases. I can hardly insert the range concept in the current sort tests set up.

I added the basic test cases and will try to leverage the pattern functions in the sort module to generate more cases. But this patch itself should be mergeable as long as the implementation looks good, since now we have the test structure, which can be improved continuously.

scottmcm · 2025-12-06T08:11:03Z

r? libs

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2025-12-22T12:51:35Z

Added some pattern tests - can be extended later.

tisonkun · 2025-12-22T12:52:00Z

cc @tgross35 can you take a look here?

tgross35

I have a few requests, mostly stylistic. I think the tests could use some improvements but I'm not sure what would be reasonable - @orlp would you mind providing some suggestions here? (In general, I'm happy to defer this review to you)

View changes since this review

tgross35 · 2025-12-28T08:44:51Z

library/alloctests/tests/lib.rs

 mod linked_list;
 mod misc_tests;
 mod num;
+mod partial_sort;


Optional nit: since sort is a module with children, maybe organize this as sort::partial instead.

tgross35 · 2025-12-28T08:49:44Z

library/core/src/slice/sort/unstable/mod.rs

 use crate::slice::sort::shared::find_existing_run;
 #[cfg(not(any(feature = "optimize_for_size", target_pointer_width = "16")))]
 use crate::slice::sort::shared::smallsort::insertion_sort_shift_left;
+use crate::slice::{self};


Nit, unusual import style

Suggested change

use crate::slice::{self};

use crate::slice;

tgross35 · 2025-12-28T09:00:02Z

library/core/src/slice/sort/unstable/mod.rs

+        if start != len && end != 0 {
+            partition_at_index(v, start, &mut is_less);
+        } else {
+            // Do nothing if it is an empty range at start or end: all guarantees
+            // are already upheld.
+        }
+
+        return;


Nit: the positive condition reads easier than negative, and this avoids the empty else just for a comment

if end == 0 || start == len { // Do nothing if it is an empty range at start or end: all guarantees // are already upheld. return; } partition_at_index(v, start, &mut is_less); return;

tgross35 · 2025-12-28T09:02:52Z

library/core/src/slice/sort/unstable/mod.rs

+    // Avoid partitioning the slice when it eliminates only a few elements to sort.
+    // The threshold of 8 elements was determined empirically.
+    let mut v = v;
+    if end + 8 <= len {
+        v = partition_at_index(v, end - 1, &mut is_less).0;
+    }
+    if start >= 8 {
+        v = partition_at_index(v, start, &mut is_less).2;
+    }


Put the "8" in a constant, similar to MAX_LEN_ALWAYS_INSERTION_SORT above. It would also be nice to include a link to further context on how the heuristic was determined (even if it's just the GH comment here) so the next person to update this doesn't need to dig too much.

tgross35 · 2025-12-28T09:10:37Z

library/core/src/slice/sort/unstable/mod.rs

+#[inline(always)]
+pub fn partial_sort<T, F, R>(v: &mut [T], range: R, mut is_less: F)


#[inline(always)] seems a bit strong here, is it actually needed? I think #[inline] would probably be fine so small code size heuristics can pick an outline point here if advantageous.

This follows how sort_unstable was implemented now -

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

Lines 19 to 20 in 7eadf83

#[inline(always)]

pub fn sort<T, F: FnMut(&T, &T) -> bool>(v: &mut [T], is_less: &mut F) {

I tend to keep this flavor and review them together later, rather than make a difference here.

After a close look, the cyclomatic complexity of sort is about 4, while partial_sort is about 7, with the final sort to be inlined always, so perhaps 10.

This is typically small enough, but it's reasonable if you insist on making it #[inline] now.

The rule of thumb nowadays is that using #[inline(always)] rather than #[inline] should be backed up by benchmarks and come with a comment explaining why, because it tends to hurt the size-optimized case. So yeah, I'd prefer #[inline] unless there is something to back up always making a difference.

Note also that inlining is bottom-up, so inline(always) applies after other things might have been inlined into the body. Thus the cyclomatic complexity of this function is actually unknown.

In general, any argument of the form "it's typically small enough" is only enough to say #[inline], because if it really is small enough that's already sufficient to get it inlined -- and means that since "typically" isn't "always", leaving the flexibility to say "well actually no this is the atypical case" in the inliner's heuristics, which is also a good thing. (Making a case for always is easier in leaves, where for example you can argue that things like pointer::add shouldn't have function-call overhead even in opt-level=0, but something like sort is very much not that.)

It's possible that what you're looking for here would be inline(trampoline) if we got rust-lang/rfcs#3778 , but today you just want a normal #[inline].

I actually intended to ping you here to double check, but somehow you have a way of appearing whenever inlining is discussed anyway :)

@scottmcm Thanks for your information!

One more question: as this partial_sort method is only used within the crate, even without #[inline], the compiler may still heuristically inline the function when desired?

So long as it has the information needed available (the MIR for the MIR inliner, the LLVM-IR for the LLVM inliner) then everything is an inlining candidate (though of course inline(never) is rarely inlined). In practice, that means that in -C codegen-units=1 with -C opt-level=3 there's not much of a difference between generic things (since those always need MIR available) and inline things.

And if you're using LTO, everything is an inlining candidate -- that's a big part of why LTO is useful (and a big part of why rustc doesn't have all that many inline annotations).

tgross35 · 2025-12-28T09:35:35Z

library/core/src/slice/mod.rs

+    /// Partially sorts the slice in ascending order **without** preserving the initial order of equal elements.
+    ///
+    /// Upon completion, for the specified range `start..end`, it's guaranteed that:
+    ///
+    /// 1. Every element in `self[..start]` is smaller than or equal to
+    /// 2. Every element in `self[start..end]`, which is sorted, and smaller than or equal to
+    /// 3. Every element in `self[end..]`.
+    ///
+    /// This partial sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not
+    /// allocate), and *O*(*n* + *k* \* log(*k*)) worst-case, where *n* is the length of the slice and
+    /// *k* is the length of the specified range.
+    ///
+    /// See the documentation of [`sort_unstable`] for implementation notes.


I think it's worth a comment noting the (lack of) guarantees about ..start and end.., given this isn't exactly covered by stability, and that the current implementation may just sort the whole thing. IOW, users shouldn't be surprised if partial_sort_unstable([0, 1, 3, 2, 4, 5], 2..4) returns [1, 0, 2, 3, 5, 4].

rustbot · 2025-12-28T09:59:56Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

orlp

I think the overall structure of tests look fine, just some stuff missing.

View changes since this review

orlp · 2025-12-28T10:07:58Z

library/alloctests/tests/partial_sort.rs

+
+    check_is_partial_sorted::<T, _>(&mut v.to_vec(), ..);
+    check_is_partial_sorted::<T, _>(&mut v.to_vec(), 0..0);
+    check_is_partial_sorted::<T, _>(&mut v.to_vec(), len - 1..len - 1);


Missing test for len..len which is a valid empty range at the end.

orlp · 2025-12-28T10:08:57Z

library/alloctests/tests/partial_sort.rs

+            check_is_partial_sorted::<T, _>(&mut v.to_vec(), mid..mid);
+            check_is_partial_sorted::<T, _>(&mut v.to_vec(), mid - 1..mid + 1);
+            check_is_partial_sorted::<T, _>(&mut v.to_vec(), mid - 1..mid);
+            check_is_partial_sorted::<T, _>(&mut v.to_vec(), mid..mid + 1);


Missing tests for substantial slices somewhere in the middle.

tisonkun · 2025-12-28T15:46:43Z

Thanks for your comments @orlp @tgross35! One comment inline #149318 (comment)

Rest SGTM. I'll integrate them in a few days and re-request a review :D

rustbot assigned scottmcm Nov 25, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 25, 2025

tisonkun commented Nov 25, 2025

View reviewed changes

library/core/src/slice/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tisonkun force-pushed the slice_partial_sort_unstable branch from 7af04ad to 115ac5c Compare November 25, 2025 16:13

tisonkun mentioned this pull request Nov 25, 2025

Tracking Issue for adding partial_sort_unstable to [T] #149046

Open

4 tasks

This comment has been minimized.

Sign in to view

tisonkun force-pushed the slice_partial_sort_unstable branch from 115ac5c to 0e87d5d Compare November 25, 2025 16:42

orlp suggested changes Nov 25, 2025

View reviewed changes

tisonkun force-pushed the slice_partial_sort_unstable branch from f9a09e0 to 372589e Compare December 1, 2025 04:09

tisonkun commented Dec 1, 2025

View reviewed changes

library/core/src/slice/sort/unstable/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tisonkun force-pushed the slice_partial_sort_unstable branch 2 times, most recently from 6ef6ab4 to 10d053f Compare December 1, 2025 10:57

This comment has been minimized.

Sign in to view

tisonkun commented Dec 1, 2025

View reviewed changes

library/core/src/slice/sort/unstable/mod.rs Show resolved Hide resolved

tisonkun commented Dec 1, 2025

View reviewed changes

library/core/src/slice/sort/unstable/mod.rs Outdated Show resolved Hide resolved

tisonkun force-pushed the slice_partial_sort_unstable branch from 10d053f to 43fc006 Compare December 1, 2025 11:13

Implement partial_sort_unstable for slice

bbca3c0

Signed-off-by: tison <wander4096@gmail.com> Co-Authored-By: Orson Peters <orsonpeters@gmail.com>

tisonkun force-pushed the slice_partial_sort_unstable branch from 43fc006 to bbca3c0 Compare December 1, 2025 11:43

Amanieu reviewed Dec 1, 2025

View reviewed changes

library/core/src/slice/sort/unstable/mod.rs Show resolved Hide resolved

Add test cases

01bfcc0

Signed-off-by: tison <wander4096@gmail.com>

rustbot assigned tgross35 and unassigned scottmcm Dec 6, 2025

Add more cases

ca6e4f8

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested review from Amanieu and orlp December 22, 2025 12:52

tgross35 requested changes Dec 28, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2025

orlp suggested changes Dec 28, 2025

View reviewed changes

		#[inline(always)]
		pub fn partial_sort<T, F, R>(v: &mut [T], range: R, mut is_less: F)

	#[inline(always)]
	pub fn sort<T, F: FnMut(&T, &T) -> bool>(v: &mut [T], is_less: &mut F) {

Uh oh!

Implement partial_sort_unstable for slice #149318

Are you sure you want to change the base?

Implement partial_sort_unstable for slice #149318

Conversation

tisonkun commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 25, 2025

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

tisonkun commented Nov 25, 2025

Uh oh!

orlp left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tisonkun commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025

Uh oh!

tisonkun commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quaternic commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 1, 2025

Uh oh!

tisonkun commented Dec 1, 2025

Uh oh!

tisonkun commented Dec 1, 2025

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Amanieu commented Dec 2, 2025

Uh oh!

tisonkun commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm commented Dec 6, 2025

Uh oh!

tisonkun commented Dec 22, 2025

Uh oh!

tisonkun commented Dec 22, 2025

Uh oh!

tgross35 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tisonkun commented Nov 25, 2025 •

edited

Loading

orlp left a comment •

edited by rustbot

Loading

orlp commented Nov 26, 2025 •

edited

Loading

quaternic commented Nov 28, 2025 •

edited

Loading

tisonkun commented Dec 4, 2025 •

edited

Loading

tgross35 left a comment •

edited by rustbot

Loading

tisonkun Dec 28, 2025 •

edited

Loading

orlp left a comment •

edited by rustbot

Loading