[libc++] Improve the performance of std::make_heap a bit #154092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- libcxx/include/__algorithm/make_heap.h libcxx/include/__algorithm/partial_sort.h libcxx/include/__algorithm/partial_sort_copy.h libcxx/include/__algorithm/sift_down.h libcxx/test/libcxx/algorithms/bad_iterator_traits.verify.cpp
View the diff from clang-format here.diff --git a/libcxx/include/__algorithm/make_heap.h b/libcxx/include/__algorithm/make_heap.h
index 46fab2039..8cfeda2b5 100644
--- a/libcxx/include/__algorithm/make_heap.h
+++ b/libcxx/include/__algorithm/make_heap.h
@@ -33,8 +33,8 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
__make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare&& __comp) {
__comp_ref_type<_Compare> __comp_ref = __comp;
- using __diff_t = __iter_diff_t<_RandomAccessIterator>;
- const __diff_t __n = __last - __first;
+ using __diff_t = __iter_diff_t<_RandomAccessIterator>;
+ const __diff_t __n = __last - __first;
static const bool __assume_both_children = is_arithmetic<__iter_value_type<_RandomAccessIterator> >::value;
|
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesFixes #120752 Full diff: https://github.com/llvm/llvm-project/pull/154092.diff 4 Files Affected:
diff --git a/libcxx/include/__algorithm/make_heap.h b/libcxx/include/__algorithm/make_heap.h
index e8f0cdb27333a..7eb5d6b89492d 100644
--- a/libcxx/include/__algorithm/make_heap.h
+++ b/libcxx/include/__algorithm/make_heap.h
@@ -12,6 +12,7 @@
#include <__algorithm/comp.h>
#include <__algorithm/comp_ref_type.h>
#include <__algorithm/iterator_operations.h>
+#include <__algorithm/push_heap.h>
#include <__algorithm/sift_down.h>
#include <__config>
#include <__iterator/iterator_traits.h>
@@ -31,13 +32,18 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
__make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare&& __comp) {
__comp_ref_type<_Compare> __comp_ref = __comp;
- using difference_type = typename iterator_traits<_RandomAccessIterator>::difference_type;
- difference_type __n = __last - __first;
+ using __diff_t = __iter_diff_t<_RandomAccessIterator>;
+ const __diff_t __n = __last - __first;
+
+ const __diff_t __odd_n = (__n & 1) ? __n : __n - 1;
+
if (__n > 1) {
// start from the first parent, there is no need to consider children
- for (difference_type __start = (__n - 2) / 2; __start >= 0; --__start) {
- std::__sift_down<_AlgPolicy>(__first, __comp_ref, __n, __first + __start);
+
+ for (__diff_t __start = (__odd_n - 2) / 2; __start >= 0; --__start) {
+ std::__sift_down<_AlgPolicy, true>(__first, __comp_ref, __odd_n, __start);
}
+ std::__sift_up<_AlgPolicy>(__first, __last, __comp, __n);
}
}
diff --git a/libcxx/include/__algorithm/partial_sort.h b/libcxx/include/__algorithm/partial_sort.h
index 7f8d0c49147e3..4b39ae0cf2dfc 100644
--- a/libcxx/include/__algorithm/partial_sort.h
+++ b/libcxx/include/__algorithm/partial_sort.h
@@ -45,7 +45,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _RandomAccessIterator __part
for (; __i != __last; ++__i) {
if (__comp(*__i, *__first)) {
_IterOps<_AlgPolicy>::iter_swap(__i, __first);
- std::__sift_down<_AlgPolicy>(__first, __comp, __len, __first);
+ std::__sift_down<_AlgPolicy, false>(__first, __comp, __len, 0);
}
}
std::__sort_heap<_AlgPolicy>(std::move(__first), std::move(__middle), __comp);
diff --git a/libcxx/include/__algorithm/partial_sort_copy.h b/libcxx/include/__algorithm/partial_sort_copy.h
index 172f53b290d54..2230dfc9cc4ad 100644
--- a/libcxx/include/__algorithm/partial_sort_copy.h
+++ b/libcxx/include/__algorithm/partial_sort_copy.h
@@ -60,7 +60,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair<_InputIterator, _Random
for (; __first != __last; ++__first)
if (std::__invoke(__comp, std::__invoke(__proj1, *__first), std::__invoke(__proj2, *__result_first))) {
*__result_first = *__first;
- std::__sift_down<_AlgPolicy>(__result_first, __projected_comp, __len, __result_first);
+ std::__sift_down<_AlgPolicy, false>(__result_first, __projected_comp, __len, 0);
}
std::__sort_heap<_AlgPolicy>(__result_first, __r, __projected_comp);
}
diff --git a/libcxx/include/__algorithm/sift_down.h b/libcxx/include/__algorithm/sift_down.h
index 42803e30631fb..e01c9b2b00f86 100644
--- a/libcxx/include/__algorithm/sift_down.h
+++ b/libcxx/include/__algorithm/sift_down.h
@@ -24,59 +24,60 @@ _LIBCPP_PUSH_MACROS
_LIBCPP_BEGIN_NAMESPACE_STD
-template <class _AlgPolicy, class _Compare, class _RandomAccessIterator>
+template <class _AlgPolicy, bool __assume_both_children, class _Compare, class _RandomAccessIterator>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
__sift_down(_RandomAccessIterator __first,
_Compare&& __comp,
- typename iterator_traits<_RandomAccessIterator>::difference_type __len,
- _RandomAccessIterator __start) {
+ __iter_diff_t<_RandomAccessIterator> __len,
+ __iter_diff_t<_RandomAccessIterator> __start) {
using _Ops = _IterOps<_AlgPolicy>;
typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
typedef typename iterator_traits<_RandomAccessIterator>::value_type value_type;
// left-child of __start is at 2 * __start + 1
// right-child of __start is at 2 * __start + 2
- difference_type __child = __start - __first;
+ difference_type __child = __start;
if (__len < 2 || (__len - 2) / 2 < __child)
return;
- __child = 2 * __child + 1;
- _RandomAccessIterator __child_i = __first + __child;
+ __child = 2 * __child + 1;
- if ((__child + 1) < __len && __comp(*__child_i, *(__child_i + difference_type(1)))) {
+ if _LIBCPP_CONSTEXPR (__assume_both_children) {
+ // right-child exists and is greater than left-child
+ __child += __comp(__first[__child], __first[__child + 1]);
+ } else if ((__child + 1) < __len && __comp(__first[__child], __first[__child + 1])) {
// right-child exists and is greater than left-child
- ++__child_i;
++__child;
}
// check if we are in heap-order
- if (__comp(*__child_i, *__start))
+ if (__comp(__first[__child], __first[__start]))
// we are, __start is larger than its largest child
return;
- value_type __top(_Ops::__iter_move(__start));
+ value_type __top(_Ops::__iter_move(__first + __start));
do {
// we are not in heap-order, swap the parent with its largest child
- *__start = _Ops::__iter_move(__child_i);
- __start = __child_i;
+ __first[__start] = _Ops::__iter_move(__first + __child);
+ __start = __child;
if ((__len - 2) / 2 < __child)
break;
// recompute the child based off of the updated parent
- __child = 2 * __child + 1;
- __child_i = __first + __child;
+ __child = 2 * __child + 1;
- if ((__child + 1) < __len && __comp(*__child_i, *(__child_i + difference_type(1)))) {
+ if _LIBCPP_CONSTEXPR (__assume_both_children) {
+ __child += __comp(__first[__child], __first[__child + 1]);
+ } else if ((__child + 1) < __len && __comp(__first[__child], __first[__child + 1])) {
// right-child exists and is greater than left-child
- ++__child_i;
++__child;
}
// check if we are in heap-order
- } while (!__comp(*__child_i, __top));
- *__start = std::move(__top);
+ } while (!__comp(__first[__child], __top));
+ __first[__start] = std::move(__top);
}
template <class _AlgPolicy, class _Compare, class _RandomAccessIterator>
|
ldionne
reviewed
Aug 25, 2025
There was a problem hiding this comment.
Given the results that show a benefit only for simple types (e.g. arithmetic types), it would probably make sense to only enable this optimization in these cases. That would at least allow us to keep the optimization in the code base and potentially widen its applicability in the future if we know how to do that.
philnik777
force-pushed
the
optimize_make_heap
branch
2 times, most recently
from
18f79a5 to
db5b085
Compare
ldionne
approved these changes
Sep 4, 2025
philnik777
force-pushed
the
optimize_make_heap
branch
2 times, most recently
from
593b553 to
cc46759
Compare
philnik777
force-pushed
the
optimize_make_heap
branch
from
cc46759 to
32307b6
Compare
|
This breaks compilation of QtDeclarative: (plus many more errors similar to these) |
|
@mstorsjo That looks to me like an issue with the iterator. If its |
|
Hmm. I'm quite out of my depth here... FWIW, I tried to see if this had been fixed in newer versions of Qt; the issue doesn't appear with Qt 6.9, but only with 6.8 (and probably earlier versions). The issue went away in qt/qtdeclarative@6537b2b#diff-e2bfe467554d88bbaf4f85e0cd871919b55134a80767ed2812b6262bf0e7b520, where
Sorry, I don't really see where the user is invoking
I'm also hitting similar issues with the sorting with a bidirectional iterator (in the call |
|
@mstorsjo |
|
FYI, I reported this issue to Qt at https://bugreports.qt.io/browse/QTBUG-140181, where it got the initial response:
|
|
Using subscript operator in |
Prabhuk
reviewed
Sep 22, 2025
| using __diff_t = __iter_diff_t<_RandomAccessIterator>; | ||
| const __diff_t __n = __last - __first; | ||
|
|
||
| static const bool __assume_both_children = is_arithmetic<__iter_value_type<_RandomAccessIterator> >::value; |
There was a problem hiding this comment.
Use of static within constexpr seems to be failing with gcc for C++ versions before 23. Does not seem to fail with clang. I am working on getting a simple reproducer but wanted to leave a message for the code owners to be aware of.
error: '__assume_both_children' defined 'static' in 'constexpr' function only available with '-std=c++23' or '-std=gnu++23' [-Wtemplate-body]
39 | static const bool __assume_both_children = is_arithmetic<__iter_value_type<_RandomAccessIterator> >::value;
| ^~~~~~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
The warning is triggered in Clang when -Wsystem-headers is set. Godbolt link: https://godbolt.org/z/Wh7zsxsYY
There was a problem hiding this comment.
In Clang, this seems to be a warning whereas in GCC its a hard error. Regardless, this code should be fixed given this violates the language spec. @philnik777
There was a problem hiding this comment.
Which part of the spec is violated when using a conforming extension in system headers?
There was a problem hiding this comment.
Apologies for lack of clarity in my previous comments where I was just thinking out loud. From what I understand static variables were not allowed within constexpr functions until C++ 23 (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2647r0.html). Your change creates a static variable static const bool __assume_both_children inside __make_heap function which is annotated with _LIBCPP_CONSTEXPR_SINCE_CXX14. This code to my understanding breaks the language spec w.r.t. creating static variables inside constexpr functions. I created a PR here to fix this and added you as a reviewer: #160180
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #120752