Skip to content

[libc++] Optimize search_n #171389

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

Open
philnik777 wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −33
Open

[libc++] Optimize search_n #171389

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
4 changes: 2 additions & 2 deletions libcxx/include/__algorithm/ranges_search_n.h
View file
Open in desktop
Copy link
Member

@ldionne ldionne Dec 10, 2025

Choose a reason for hiding this comment

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

Please add a description of the optimization to the commit message!

Original file line number Diff line number Diff line change
Expand Up @@ -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)};
}
}
Expand Down
65 changes: 34 additions & 31 deletions libcxx/include/__algorithm/search_n.h
View file
Open in desktop
Copy link
Member

@ldionne ldionne Dec 10, 2025

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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)) {
Copy link
Member

@ldionne ldionne Dec 10, 2025

Choose a reason for hiding this comment

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

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.

__matched_until = __match_start + __count;
__match_start = ++__check;
break;
} // else there is a match, check next elements
}
}
}
}
Expand All @@ -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);
}

Copy link
Member

@ldionne ldionne Dec 10, 2025

Choose a reason for hiding this comment

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

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.

template <class _Iter1,
Expand All @@ -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(
Copy link
Member

@ldionne ldionne Dec 10, 2025

Choose a reason for hiding this comment

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

Seems like that is not necessary.

__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>
Expand Down
Loading