[libc++] Optimize search_n #171389
Open
[libc++] Optimize search_n #171389
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
philnik777
force-pushed
the
optimize_search_n
branch
from
272aeda to
88a5755
Compare
llvmbot
added
the
libc++
Member
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesFull diff: https://github.com/llvm/llvm-project/pull/171389.diff 2 Files Affected:
diff --git a/libcxx/include/__algorithm/ranges_search_n.h b/libcxx/include/__algorithm/ranges_search_n.h
index 81b568c0965fd..746bfcc3d1a8f 100644
--- a/libcxx/include/__algorithm/ranges_search_n.h
+++ b/libcxx/include/__algorithm/ranges_search_n.h
@@ -54,8 +54,8 @@ struct __search_n {
}
if constexpr (random_access_iterator<_Iter1>) {
- auto __ret = std::__search_n_random_access_impl<_RangeAlgPolicy>(
- __first, __last, __count, __value, __pred, __proj, __size);
+ auto __ret =
+ std::__search_n_random_access_impl<_RangeAlgPolicy>(__first, __count, __value, __pred, __proj, __size);
return {std::move(__ret.first), std::move(__ret.second)};
}
}
diff --git a/libcxx/include/__algorithm/search_n.h b/libcxx/include/__algorithm/search_n.h
index 38474e1b2379d..8d961489a46c2 100644
--- a/libcxx/include/__algorithm/search_n.h
+++ b/libcxx/include/__algorithm/search_n.h
@@ -11,7 +11,9 @@
#define _LIBCPP___ALGORITHM_SEARCH_N_H
#include <__algorithm/comp.h>
+#include <__algorithm/find.h>
#include <__algorithm/iterator_operations.h>
+#include <__algorithm/unwrap_iter.h>
#include <__config>
#include <__functional/identity.h>
#include <__iterator/advance.h>
@@ -68,43 +70,35 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_Iter, _Iter> __search_
}
}
-template <class _AlgPolicy, class _Pred, class _Iter, class _Sent, class _SizeT, class _Type, class _Proj, class _DiffT>
+template <class _AlgPolicy, class _Pred, class _Iter, class _SizeT, class _Type, class _Proj, class _DiffT>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 std::pair<_Iter, _Iter> __search_n_random_access_impl(
- _Iter __first, _Sent __last, _SizeT __count, const _Type& __value, _Pred& __pred, _Proj& __proj, _DiffT __size1) {
- using difference_type = typename iterator_traits<_Iter>::difference_type;
+ _Iter __first, _SizeT __count_in, const _Type& __value, _Pred& __pred, _Proj& __proj, _DiffT __size) {
+ auto __last = __first + __size;
+ auto __count = static_cast<_DiffT>(__count_in);
+
if (__count == 0)
return std::make_pair(__first, __first);
- if (__size1 < static_cast<_DiffT>(__count)) {
- _IterOps<_AlgPolicy>::__advance_to(__first, __last);
- return std::make_pair(__first, __first);
- }
+ if (__size < __count)
+ return std::make_pair(__last, __last);
- const auto __s = __first + __size1 - difference_type(__count - 1); // Start of pattern match can't go beyond here
+ _Iter __try_match_until = __last - __count;
+ _Iter __match_start = __first;
+ _Iter __matched_until = __first;
while (true) {
- // Find first element in sequence that matchs __value, with a mininum of loop checks
- while (true) {
- if (__first >= __s) { // return __last if no element matches __value
- _IterOps<_AlgPolicy>::__advance_to(__first, __last);
- return std::make_pair(__first, __first);
- }
- if (std::__invoke(__pred, std::__invoke(__proj, *__first), __value))
- break;
- ++__first;
- }
- // *__first matches __value_, now match elements after here
- auto __m = __first;
- _SizeT __c(0);
+ if (__match_start > __try_match_until)
+ return std::make_pair(__last, __last);
+
+ _Iter __check = __match_start + __count;
+
while (true) {
- if (++__c == __count) // If pattern exhausted, __first is the answer (works for 1 element pattern)
- return std::make_pair(__first, __first + _DiffT(__count));
- ++__m; // no need to check range on __m because __s guarantees we have enough source
+ if (__check == __matched_until)
+ return std::make_pair(__match_start, __match_start + __count);
- // if there is a mismatch, restart with a new __first
- if (!std::__invoke(__pred, std::__invoke(__proj, *__m), __value)) {
- __first = __m;
- ++__first;
+ if (!std::__invoke(__pred, std::__invoke(__proj, *--__check), __value)) {
+ __matched_until = __match_start + __count;
+ __match_start = ++__check;
break;
- } // else there is a match, check next elements
+ }
}
}
}
@@ -119,7 +113,7 @@ template <class _Iter,
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 pair<_Iter, _Iter>
__search_n_impl(_Iter __first, _Sent __last, _DiffT __count, const _Type& __value, _Pred& __pred, _Proj& __proj) {
return std::__search_n_random_access_impl<_ClassicAlgPolicy>(
- __first, __last, __count, __value, __pred, __proj, __last - __first);
+ __first, __count, __value, __pred, __proj, __last - __first);
}
template <class _Iter1,
@@ -142,7 +136,16 @@ template <class _ForwardIterator, class _Size, class _Tp, class _BinaryPredicate
static_assert(
__is_callable<_BinaryPredicate&, decltype(*__first), const _Tp&>::value, "The comparator has to be callable");
auto __proj = __identity();
- return std::__search_n_impl(__first, __last, std::__convert_to_integral(__count), __value, __pred, __proj).first;
+ return std::__rewrap_iter(
+ __first,
+ std::__search_n_impl(
+ std::__unwrap_iter(__first),
+ std::__unwrap_iter(__last),
+ std::__convert_to_integral(__count),
+ __value,
+ __pred,
+ __proj)
+ .first);
}
template <class _ForwardIterator, class _Size, class _Tp>
|
ldionne
requested changes
Dec 10, 2025
Member
There was a problem hiding this comment.
Please add a description of the optimization to the commit message!
| if (!std::__invoke(__pred, std::__invoke(__proj, *__m), __value)) { | ||
| __first = __m; | ||
| ++__first; | ||
| if (!std::__invoke(__pred, std::__invoke(__proj, *--__check), __value)) { |
Member
There was a problem hiding this comment.
I would suggest introducing __find_last to clean up these two nested while (true) loops:
auto __not_value = [&](auto const& __x) { return !std::__invoke(__pred, __x, __value); };
auto __res = std::__find_last(__match_start, __match_start + __count, __not_value, __proj);
if (__res == __match_start + __count) {
// we have a match
} else {
// no match :-(
}We don't necessarily have to go all the way and refactor ranges::find_last to use it, but at least having a named algorithm for this is an improvement.
| __is_callable<_BinaryPredicate&, decltype(*__first), const _Tp&>::value, "The comparator has to be callable"); | ||
| auto __proj = __identity(); | ||
| return std::__search_n_impl(__first, __last, std::__convert_to_integral(__count), __value, __pred, __proj).first; | ||
| return std::__rewrap_iter( |
Member
There was a problem hiding this comment.
Seems like that is not necessary.
Member
There was a problem hiding this comment.
Let's add a benchmark that intersperses near matches throughout the sequence. Something like https://github.com/llvm/llvm-project/blob/main/libcxx/test/benchmarks/algorithms/nonmodifying/search.bench.cpp#L85.
| __first, __last, __count, __value, __pred, __proj, __last - __first); | ||
| __first, __count, __value, __pred, __proj, __last - __first); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Not attached to this line: Let's add test coverage for having multiple subranges that partially match within a sequence. You can take inspiration from ranges::search_n for the corner cases that are covered.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 #129327