Skip to content

Commit 83c530f

Browse files
VoultapherVoultapher
committed
Apply review comments
1 parent 86878b9 commit 83c530f

File tree

2 files changed

+98
-90
lines changed

2 files changed

+98
-90
lines changed

‎core/src/slice/sort/shared/smallsort.rs

+23-15
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,9 @@ impl<T: FreezeMarker> UnstableSmallSortTypeImpl for T {
9494
#[inline(always)]
9595
fn small_sort_threshold() -> usize {
9696
match const { choose_unstable_small_sort::<T>() } {
97-
UnstalbeSmallSort::Fallback => SMALL_SORT_FALLBACK_THRESHOLD,
98-
UnstalbeSmallSort::General => SMALL_SORT_GENERAL_THRESHOLD,
99-
UnstalbeSmallSort::Network => SMALL_SORT_NETWORK_THRESHOLD,
97+
UnstableSmallSort::Fallback => SMALL_SORT_FALLBACK_THRESHOLD,
98+
UnstableSmallSort::General => SMALL_SORT_GENERAL_THRESHOLD,
99+
UnstableSmallSort::Network => SMALL_SORT_NETWORK_THRESHOLD,
100100
}
101101
}
102102

@@ -137,34 +137,34 @@ const SMALL_SORT_NETWORK_SCRATCH_LEN: usize = SMALL_SORT_NETWORK_THRESHOLD;
137137
/// within this limit.
138138
const MAX_STACK_ARRAY_SIZE: usize = 4096;
139139

140-
enum UnstalbeSmallSort {
140+
enum UnstableSmallSort {
141141
Fallback,
142142
General,
143143
Network,
144144
}
145145

146-
const fn choose_unstable_small_sort<T: FreezeMarker>() -> UnstalbeSmallSort {
146+
const fn choose_unstable_small_sort<T: FreezeMarker>() -> UnstableSmallSort {
147147
if T::is_copy()
148148
&& has_efficient_in_place_swap::<T>()
149149
&& (mem::size_of::<T>() * SMALL_SORT_NETWORK_SCRATCH_LEN) <= MAX_STACK_ARRAY_SIZE
150150
{
151151
// Heuristic for int like types.
152-
return UnstalbeSmallSort::Network;
152+
return UnstableSmallSort::Network;
153153
}
154154

155155
if (mem::size_of::<T>() * SMALL_SORT_GENERAL_SCRATCH_LEN) <= MAX_STACK_ARRAY_SIZE {
156-
return UnstalbeSmallSort::General;
156+
return UnstableSmallSort::General;
157157
}
158158

159-
UnstalbeSmallSort::Fallback
159+
UnstableSmallSort::Fallback
160160
}
161161

162162
const fn inst_unstable_small_sort<T: FreezeMarker, F: FnMut(&T, &T) -> bool>()
163163
-> fn(&mut [T], &mut F) {
164164
match const { choose_unstable_small_sort::<T>() } {
165-
UnstalbeSmallSort::Fallback => small_sort_fallback::<T, F>,
166-
UnstalbeSmallSort::General => small_sort_general::<T, F>,
167-
UnstalbeSmallSort::Network => small_sort_network::<T, F>,
165+
UnstableSmallSort::Fallback => small_sort_fallback::<T, F>,
166+
UnstableSmallSort::General => small_sort_general::<T, F>,
167+
UnstableSmallSort::Network => small_sort_network::<T, F>,
168168
}
169169
}
170170

@@ -384,8 +384,12 @@ where
384384
}
385385
}
386386

387-
// Never inline this function to avoid code bloat. It still optimizes nicely and has practically no
388-
// performance impact.
387+
/// Sorts the first 9 elements of `v` with a fast fixed function.
388+
///
389+
/// Should `is_less` generate substantial amounts of code the compiler can choose to not inline
390+
/// `swap_if_less`. If the code of a sort impl changes so as to call this function in multiple
391+
/// places, `#[inline(never)]` is recommended to keep binary-size in check. The current design of
392+
/// `small_sort_network` makes sure to only call this once.
389393
fn sort9_optimal<T, F>(v: &mut [T], is_less: &mut F)
390394
where
391395
F: FnMut(&T, &T) -> bool,
@@ -429,8 +433,12 @@ where
429433
}
430434
}
431435

432-
// Never inline this function to avoid code bloat. It still optimizes nicely and has practically no
433-
// performance impact.
436+
/// Sorts the first 13 elements of `v` with a fast fixed function.
437+
///
438+
/// Should `is_less` generate substantial amounts of code the compiler can choose to not inline
439+
/// `swap_if_less`. If the code of a sort impl changes so as to call this function in multiple
440+
/// places, `#[inline(never)]` is recommended to keep binary-size in check. The current design of
441+
/// `small_sort_network` makes sure to only call this once.
434442
fn sort13_optimal<T, F>(v: &mut [T], is_less: &mut F)
435443
where
436444
F: FnMut(&T, &T) -> bool,

‎core/src/slice/sort/stable/merge.rs

+75-75
Original file line numberDiff line numberDiff line change
@@ -61,91 +61,91 @@ pub fn merge<T, F: FnMut(&T, &T) -> bool>(
6161
// Finally, `merge_state` gets dropped. If the shorter run was not fully
6262
// consumed, whatever remains of it will now be copied into the hole in `v`.
6363
}
64+
}
6465

65-
// When dropped, copies the range `start..end` into `dst..`.
66-
struct MergeState<T> {
67-
start: *mut T,
68-
end: *mut T,
69-
dst: *mut T,
70-
}
66+
// When dropped, copies the range `start..end` into `dst..`.
67+
struct MergeState<T> {
68+
start: *mut T,
69+
end: *mut T,
70+
dst: *mut T,
71+
}
7172

72-
impl<T> MergeState<T> {
73-
/// # Safety
74-
/// The caller MUST guarantee that `self` is initialized in a way where `start -> end` is
75-
/// the longer sub-slice and so that `dst` can be written to at least the shorter sub-slice
76-
/// length times. In addition `start -> end` and `right -> right_end` MUST be valid to be
77-
/// read. This function MUST only be called once.
78-
unsafe fn merge_up<F: FnMut(&T, &T) -> bool>(
79-
&mut self,
80-
mut right: *const T,
81-
right_end: *const T,
82-
is_less: &mut F,
83-
) {
84-
// SAFETY: See function safety comment.
85-
unsafe {
86-
let left = &mut self.start;
87-
let out = &mut self.dst;
88-
89-
while *left != self.end && right as *const T != right_end {
90-
let consume_left = !is_less(&*right, &**left);
91-
92-
let src = if consume_left { *left } else { right };
93-
ptr::copy_nonoverlapping(src, *out, 1);
94-
95-
*left = left.add(consume_left as usize);
96-
right = right.add(!consume_left as usize);
97-
98-
*out = out.add(1);
99-
}
73+
impl<T> MergeState<T> {
74+
/// # Safety
75+
/// The caller MUST guarantee that `self` is initialized in a way where `start -> end` is
76+
/// the longer sub-slice and so that `dst` can be written to at least the shorter sub-slice
77+
/// length times. In addition `start -> end` and `right -> right_end` MUST be valid to be
78+
/// read. This function MUST only be called once.
79+
unsafe fn merge_up<F: FnMut(&T, &T) -> bool>(
80+
&mut self,
81+
mut right: *const T,
82+
right_end: *const T,
83+
is_less: &mut F,
84+
) {
85+
// SAFETY: See function safety comment.
86+
unsafe {
87+
let left = &mut self.start;
88+
let out = &mut self.dst;
89+
90+
while *left != self.end && right as *const T != right_end {
91+
let consume_left = !is_less(&*right, &**left);
92+
93+
let src = if consume_left { *left } else { right };
94+
ptr::copy_nonoverlapping(src, *out, 1);
95+
96+
*left = left.add(consume_left as usize);
97+
right = right.add(!consume_left as usize);
98+
99+
*out = out.add(1);
100100
}
101101
}
102+
}
102103

103-
/// # Safety
104-
/// The caller MUST guarantee that `self` is initialized in a way where `left_end <- dst` is
105-
/// the shorter sub-slice and so that `out` can be written to at least the shorter sub-slice
106-
/// length times. In addition `left_end <- dst` and `right_end <- end` MUST be valid to be
107-
/// read. This function MUST only be called once.
108-
unsafe fn merge_down<F: FnMut(&T, &T) -> bool>(
109-
&mut self,
110-
left_end: *const T,
111-
right_end: *const T,
112-
mut out: *mut T,
113-
is_less: &mut F,
114-
) {
115-
// SAFETY: See function safety comment.
116-
unsafe {
117-
loop {
118-
let left = self.dst.sub(1);
119-
let right = self.end.sub(1);
120-
out = out.sub(1);
121-
122-
let consume_left = is_less(&*right, &*left);
123-
124-
let src = if consume_left { left } else { right };
125-
ptr::copy_nonoverlapping(src, out, 1);
126-
127-
self.dst = left.add(!consume_left as usize);
128-
self.end = right.add(consume_left as usize);
129-
130-
if self.dst as *const T == left_end || self.end as *const T == right_end {
131-
break;
132-
}
104+
/// # Safety
105+
/// The caller MUST guarantee that `self` is initialized in a way where `left_end <- dst` is
106+
/// the shorter sub-slice and so that `out` can be written to at least the shorter sub-slice
107+
/// length times. In addition `left_end <- dst` and `right_end <- end` MUST be valid to be
108+
/// read. This function MUST only be called once.
109+
unsafe fn merge_down<F: FnMut(&T, &T) -> bool>(
110+
&mut self,
111+
left_end: *const T,
112+
right_end: *const T,
113+
mut out: *mut T,
114+
is_less: &mut F,
115+
) {
116+
// SAFETY: See function safety comment.
117+
unsafe {
118+
loop {
119+
let left = self.dst.sub(1);
120+
let right = self.end.sub(1);
121+
out = out.sub(1);
122+
123+
let consume_left = is_less(&*right, &*left);
124+
125+
let src = if consume_left { left } else { right };
126+
ptr::copy_nonoverlapping(src, out, 1);
127+
128+
self.dst = left.add(!consume_left as usize);
129+
self.end = right.add(consume_left as usize);
130+
131+
if self.dst as *const T == left_end || self.end as *const T == right_end {
132+
break;
133133
}
134134
}
135135
}
136136
}
137+
}
137138

138-
impl<T> Drop for MergeState<T> {
139-
fn drop(&mut self) {
140-
// SAFETY: The user of MergeState MUST ensure, that at any point this drop
141-
// impl MAY run, for example when the user provided `is_less` panics, that
142-
// copying the contiguous region between `start` and `end` to `dst` will
143-
// leave the input slice `v` with each original element and all possible
144-
// modifications observed.
145-
unsafe {
146-
let len = self.end.sub_ptr(self.start);
147-
ptr::copy_nonoverlapping(self.start, self.dst, len);
148-
}
139+
impl<T> Drop for MergeState<T> {
140+
fn drop(&mut self) {
141+
// SAFETY: The user of MergeState MUST ensure, that at any point this drop
142+
// impl MAY run, for example when the user provided `is_less` panics, that
143+
// copying the contiguous region between `start` and `end` to `dst` will
144+
// leave the input slice `v` with each original element and all possible
145+
// modifications observed.
146+
unsafe {
147+
let len = self.end.sub_ptr(self.start);
148+
ptr::copy_nonoverlapping(self.start, self.dst, len);
149149
}
150150
}
151151
}

0 commit comments

Comments
 (0)