Skip to content

[libc++] [sort] Improve performance of std::sort #76616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[libc++] [sort] Improve performance of std::sort #76616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions libcxx/include/__algorithm/sort.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
#include <__iterator/iterator_traits.h>
#include <__type_traits/conditional.h>
#include <__type_traits/disjunction.h>
#include <__type_traits/is_arithmetic.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_trivially_copyable.h>
#include <__utility/move.h>
#include <__utility/pair.h>
#include <climits>
Expand Down Expand Up @@ -144,7 +144,7 @@ template <class _Compare, class _Iter, class _Tp = typename iterator_traits<_Ite
using __use_branchless_sort =
integral_constant<bool,
__libcpp_is_contiguous_iterator<_Iter>::value && sizeof(_Tp) <= sizeof(void*) &&
is_arithmetic<_Tp>::value && __is_simple_comparator<_Compare>::value>;
is_trivially_copyable<_Tp>::value && __is_simple_comparator<_Compare>::value>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there already tests for sorting trivially_copyable types?


namespace __detail {

Expand All @@ -158,6 +158,8 @@ template <class _Compare, class _RandomAccessIterator>
inline _LIBCPP_HIDE_FROM_ABI void __cond_swap(_RandomAccessIterator __x, _RandomAccessIterator __y, _Compare __c) {
// Note: this function behaves correctly even with proxy iterators (because it relies on `value_type`).
using value_type = typename iterator_traits<_RandomAccessIterator>::value_type;
static_assert(is_trivially_copyable<value_type>::value,
"Copying must have no side effects distinguishable from swapping/moving");
bool __r = __c(*__x, *__y);
value_type __tmp = __r ? *__x : *__y;
*__y = __r ? *__y : *__x;
Expand All @@ -171,6 +173,8 @@ inline _LIBCPP_HIDE_FROM_ABI void
__partially_sorted_swap(_RandomAccessIterator __x, _RandomAccessIterator __y, _RandomAccessIterator __z, _Compare __c) {
// Note: this function behaves correctly even with proxy iterators (because it relies on `value_type`).
using value_type = typename iterator_traits<_RandomAccessIterator>::value_type;
static_assert(is_trivially_copyable<value_type>::value,
"Copying must have no side effects distinguishable from swapping/moving");
bool __r = __c(*__z, *__x);
value_type __tmp = __r ? *__z : *__x;
*__z = __r ? *__x : *__z;
Expand Down
3 changes: 1 addition & 2 deletions libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ void test_same() {
auto snapshot_custom_v = v;
std::sort(v.begin(), v.end());
std::sort(snapshot_v.begin(), snapshot_v.end());
std::sort(snapshot_custom_v.begin(), snapshot_custom_v.end(),
[](const EqualType&, const EqualType&) { return false; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this test instead of just adding the new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old test case passes [](const EqualType&, const EqualType&) { return false; } as a comparator, which is not a "simple comparator", and __use_branchless_sort is written in a way that is true only when the __is_simple_comparator is true, therefor libc++ chooses to use branchless sort for std::sort(snapshot_v.begin(), snapshot_v.end()); but not for std::sort(snapshot_custom_v.begin(), snapshot_custom_v.end(), [](const EqualType&, const EqualType&) { return false; }); but after the change std::sort(snapshot_custom_v.begin(), snapshot_custom_v.end(), std::less<EqualType>()) uses branchless sort. After this change, both statements will use branchless sort while the main behavior is preserved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that means we start to strongly write our tests based on our current implementation. That's not a good idea in general. Having some duplicated test coverage can help during refactoring where different paths are taken. So I really like adding a test, I'm just less thrilled with the removal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that this is in test/libcxx, not test/std, and so there's more lenience for tests that are coupled with the implementation. I do think that we should avoid this practice though.

It's also worth noting that this lambda doesn't meet the requirements of Compare because it doesn't impose a strict weak order on its elements, so replacing it with something that does is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return false; imposes a perfectly correct strict weak ordering comparator

std::sort(snapshot_custom_v.begin(), snapshot_custom_v.end(), std::less<EqualType>());
bool all_equal = true;
for (int i = 0; i < kSize; ++i) {
if (v[i].value != snapshot_v[i].value || v[i].value != snapshot_custom_v[i].value) {
Expand Down