[libc] Bound the worst-case stack usage in qsort(). #110849
Conversation
statham-arm
requested review from
petrhosek,
lntue,
smithp35 and
michaelrj-google
|
@llvm/pr-subscribers-libc Author: Simon Tatham (statham-arm) ChangesPreviously, the Quicksort implementation was written in the obvious way: after each partitioning step, it explicitly recursed twice to sort the two sublists. Now it compares the two sublists' sizes, and recurses only to sort the smaller one. To handle the larger list it loops back round to the top of the function, so as to handle it within the existing stack frame. This means that every recursive call is handling a list at most half that of its caller. So the maximum recursive call depth is O(lg N). Otherwise, in Quicksort's bad cases where each partition step peels off a small constant number of array elements, the stack usage could grow linearly with the array being sorted, i.e. it might be Θ(N). I tested this code by manually constructing a List Of Doom that causes this particular quicksort implementation to hit its worst case, and confirming that it recursed very deeply in the old code and doesn't in the new code. But I haven't added that list to the test suite, because the List Of Doom has to be constructed in a way based on every detail of the quicksort algorithm (pivot choice and partitioning strategy), so it would silently stop being a useful regression test as soon as any detail changed. Full diff: https://github.com/llvm/llvm-project/pull/110849.diff 2 Files Affected:
diff --git a/libc/src/stdlib/qsort_data.h b/libc/src/stdlib/qsort_data.h
index db045332708ae6..9d44d41f46580c 100644
--- a/libc/src/stdlib/qsort_data.h
+++ b/libc/src/stdlib/qsort_data.h
@@ -92,6 +92,12 @@ class Array {
Array make_array(size_t i, size_t s) const {
return Array(get(i), s, elem_size, compare);
}
+
+ // Reset this Array to point at a different interval of the same items.
+ void reset_bounds(uint8_t *a, size_t s) {
+ array = a;
+ array_size = s;
+ }
};
using SortingRoutine = void(const Array &);
diff --git a/libc/src/stdlib/quick_sort.h b/libc/src/stdlib/quick_sort.h
index 89ec107161e3e5..a3fe1681591d26 100644
--- a/libc/src/stdlib/quick_sort.h
+++ b/libc/src/stdlib/quick_sort.h
@@ -59,17 +59,33 @@ static size_t partition(const Array &array) {
}
}
-LIBC_INLINE void quick_sort(const Array &array) {
- const size_t array_size = array.size();
- if (array_size <= 1)
- return;
- size_t split_index = partition(array);
- if (array_size <= 2) {
- // The partition operation sorts the two element array.
- return;
+LIBC_INLINE void quick_sort(Array array) {
+ while (true) {
+ const size_t array_size = array.size();
+ if (array_size <= 1)
+ return;
+ size_t split_index = partition(array);
+ if (array_size <= 2) {
+ // The partition operation sorts the two element array.
+ return;
+ }
+
+ // Make Arrays describing the two sublists that still need sorting.
+ Array left = array.make_array(0, split_index);
+ Array right = array.make_array(split_index, array.size() - split_index);
+
+ // Recurse to sort the smaller of the two, and then loop round within this
+ // function to sort the larger. This way, recursive call depth is bounded
+ // by log2 of the total array size, because every recursive call is sorting
+ // a list at most half the length of the one in its caller.
+ if (left.size() < right.size()) {
+ quick_sort(left, depth+1);
+ array.reset_bounds(right.get(0), right.size());
+ } else {
+ quick_sort(right, depth+1);
+ array.reset_bounds(left.get(0), left.size());
+ }
}
- quick_sort(array.make_array(0, split_index));
- quick_sort(array.make_array(split_index, array.size() - split_index));
}
} // namespace internal
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Previously, the Quicksort implementation was written in the obvious way: after each partitioning step, it explicitly recursed twice to sort the two sublists. Now it compares the two sublists' sizes, and recurses only to sort the smaller one. To handle the larger list it loops back round to the top of the function, so as to handle it within the existing stack frame. This means that every recursive call is handling a list at most half that of its caller. So the maximum recursive call depth is O(lg N). Otherwise, in Quicksort's bad cases where each partition step peels off a small constant number of array elements, the stack usage could grow linearly with the array being sorted, i.e. it might be Θ(N). I tested this code by manually constructing a List Of Doom that causes this particular quicksort implementation to hit its worst case, and confirming that it recursed very deeply in the old code and doesn't in the new code. But I haven't added that list to the test suite, because the List Of Doom has to be constructed in a way based on every detail of the quicksort algorithm (pivot choice and partitioning strategy), so it would silently stop being a useful regression test as soon as any detail changed.
statham-arm
force-pushed
the
qsort-log-stack
branch
from
2a763fd to
b298f32
Compare
libc/src/stdlib/quick_sort.h
Outdated
| if (array_size <= 2) { | ||
| // The partition operation sorts the two element array. | ||
| return; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
That construction was unchanged from the previous version of the code – all I've done is to move it right by two spaces! But fine, I can fix it while I'm here.
libc/src/stdlib/quick_sort.h
Outdated
| if (array_size <= 1) | ||
| return; | ||
| size_t split_index = partition(array); | ||
| if (array_size <= 2) |
There was a problem hiding this comment.
| if (array_size <= 2) | |
| if (array_size == 2) |
libc/src/stdlib/qsort_data.h
Outdated
| @@ -92,6 +92,12 @@ class Array { | |||
| Array make_array(size_t i, size_t s) const { | |||
| return Array(get(i), s, elem_size, compare); | |||
| } | |||
|
|
|||
| // Reset this Array to point at a different interval of the same items. | |||
| void reset_bounds(uint8_t *a, size_t s) { | |||
There was a problem hiding this comment.
This method, and the others should probably have the LIBC_INLINE macro added.
https://libc.llvm.org/dev/code_style.html#inline-functions-and-variables-defined-in-header-files
Done: #111495. |
Previously, the Quicksort implementation was written in the obvious way: after each partitioning step, it explicitly recursed twice to sort the two sublists. Now it compares the two sublists' sizes, and recurses only to sort the smaller one. To handle the larger list it loops back round to the top of the function, so as to handle it within the existing stack frame.
This means that every recursive call is handling a list at most half that of its caller. So the maximum recursive call depth is O(lg N). Otherwise, in Quicksort's bad cases where each partition step peels off a small constant number of array elements, the stack usage could grow linearly with the array being sorted, i.e. it might be Θ(N).
I tested this code by manually constructing a List Of Doom that causes this particular quicksort implementation to hit its worst case, and confirming that it recursed very deeply in the old code and doesn't in the new code. But I haven't added that list to the test suite, because the List Of Doom has to be constructed in a way based on every detail of the quicksort algorithm (pivot choice and partitioning strategy), so it would silently stop being a useful regression test as soon as any detail changed.