Optimize is_permutation for vector<bool> - #6148
Optimize is_permutation for vector<bool>#6148Alex Guteniev (AlexGuteniev) wants to merge 23 commits into
is_permutation for vector<bool>#6148Conversation
|
I believe this could be generalized further. For any ranges where the value types are |
|
Generalized, benchmark results are about the same |
There was a problem hiding this comment.
Pull request overview
This PR optimizes std::is_permutation when operating on bool ranges (notably vector<bool> iterators) by replacing the general-purpose O(N²) match-counting approach with a linear-time mismatch + count(true) strategy.
Changes:
- Added an internal helper (
_Is_permutation_of_bool) and fast-paths inis_permutationoverloads when the predicate is an equality predicate and both ranges arebool. - Added tests validating
is_permutationbehavior forvector<bool>and mixedvector<bool>/raw-bool[]iterator combinations. - Added a new microbenchmark to measure
is_permutationperformance onvector<bool>and on rawbool[].
Show a summary per file
| File | Description |
|---|---|
stl/inc/algorithm |
Introduces bool-specific is_permutation fast-paths using count(true) (and mismatch when neither iterator is vector<bool>). |
tests/std/tests/GH_000625_vector_bool_optimization/test.cpp |
Adds constexpr/runtime coverage for is_permutation with vector<bool> and bool[]. |
benchmarks/src/vector_bool_permute.cpp |
Adds benchmarks for is_permutation on vector<bool> and bool[]. |
benchmarks/CMakeLists.txt |
Registers the new vector_bool_permute benchmark target. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
| template <class _FwdIt1, class _FwdIt2> | ||
| _NODISCARD _CONSTEXPR20 bool _Is_permutation_of_bool(_FwdIt1 _First1, _FwdIt1 _Last1, _FwdIt2 _First2, _FwdIt2 _Last2) { |
There was a problem hiding this comment.
I think this one is premature. I don't attempt to handle std::ranges::is_permutation yet, so there's no distinct sentinel type. When we handle it, we'll see how it should be done.
| if constexpr (is_same_v<_Iter_value_t<decltype(_UFirst1)>, bool> | ||
| && is_same_v<_Iter_value_t<decltype(_UFirst2)>, bool> // | ||
| && _Is_ranges_random_iter_v<decltype(_UFirst1)> // | ||
| && _Is_ranges_random_iter_v<decltype(_UFirst2)> |
There was a problem hiding this comment.
The 4-arg version checks the wrapped iterator instead. The check it pre-existing.
Maybe I need to change it to check the unwrapped iterators too though?
There was a problem hiding this comment.
Checking the unwrapped iterators.
| if constexpr (!_Is_vb_iterator<_FwdIt1> && !_Is_vb_iterator<_FwdIt2>) { | ||
| auto _Pair = _STD mismatch(_First1, _Last1, _First2); |
There was a problem hiding this comment.
No.
This is intentional optimization decision. It is explained in the PR description.
mismatch is good for non-vb iterators (either vectorized, or better than count), but for just one vb iterator it flips as count is SWAR with popcount and mismatch is individual bit matching.
| } | ||
| } | ||
|
|
||
| return _STD count(_First1, _Last1, true) == _STD count(_First2, _Last2, true); |
There was a problem hiding this comment.
I believe we should guard this optimization with _Is_vb_iterator, as we've done with others, since reasoning about wacky iterators whose value type is bool but reference type is somebody else's proxy, is too difficult.
There was a problem hiding this comment.
(From Discord:) Ok, I suggest the following: optimize for iterators that are either _Is_vb_iterator, or actually have reference types that are bool after remove_cv_ref. That gets you any mix of real bool and vector<bool>, but never wacky bool-oids.
There was a problem hiding this comment.
- Added check for
_Iter_ref_t - Added a test that breaks without the
_Iter_ref_tchange - Ran benchmark to make sure the optimization is still in
There was a problem hiding this comment.
Copilot could you please verify the fix?
Stephan T. Lavavej (StephanTLavavej)
moved this from Initial Review
to Work In Progress
in STL Code Reviews
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/std/tests/GH_000625_vector_bool_optimization/test.cpp:1589
vbool_like_iteratoradvertisesrandom_access_iterator_tagbut omits required operations such as postfix++/--,n + iterator, andoperator[]. It therefore does not satisfy the LegacyForwardIterator/LegacyRandomAccessIterator requirements of the algorithms exercised below, so the test invokes them outside their contract and does not validate the proxy-reference guard with a conforming iterator. Please complete the random-access iterator interface (or use the repository's iterator test support).
struct vbool_like_iterator {
using iterator_category = random_access_iterator_tag;
using value_type = bool;
using difference_type = ptrdiff_t;
using pointer = bool*;
using reference = vbool_like_reference;
| // proxy-to-proxy comparison; the evil part, not present in vector<bool> proxies | ||
| bool operator==(const vbool_like_reference&) const { | ||
| return true; // all proxies are equal | ||
| } |
There was a problem hiding this comment.
Would it make more sense to just make this overload deleted, which should ensure that we don't use it even in non-executed branches? Ditto below.
| // proxy-to-proxy comparison; the evil part, not present in vector<bool> proxies | |
| bool operator==(const vbool_like_reference&) const { | |
| return true; // all proxies are equal | |
| } | |
| // proxy-to-proxy comparison; the evil part, not present in vector<bool> proxies | |
| bool operator==(const vbool_like_reference&) const = delete; |
There was a problem hiding this comment.
No. These are used.
The point of this problem is that is_permutation/mismatch/equal will use these operators instead of implicit value conversion and the results may differ. The test deliberately makes equal/is_permutation true for any inputs of equal lengths.
Stephan T. Lavavej (StephanTLavavej)
moved this from Work In Progress
to Initial Review
in STL Code Reviews
There was a problem hiding this comment.
🟡 Changes recommended
The invalid postfix iterator implementations must be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
tests/std/tests/GH_000625_vector_bool_optimization/test.cpp:1615
- This postfix decrement has the same invalid-return behavior: it returns an iterator with an indeterminate
ptr, not the position before decrementing. Copy*thisbefore updatingptr.
vbool_like_iterator operator--(int) {
vbool_like_iterator result;
--ptr;
return result;
tests/std/tests/GH_000625_vector_bool_optimization/test.cpp:1638
- A random-access iterator's
i[d]must be equivalent to*(i + d), but this implementation subtractsd. Positive indexes therefore access the wrong element and can move before the range.
vbool_like_reference operator[](const ptrdiff_t d) const {
return {ptr - d};
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The regression-test iterator has incorrect indexing semantics that must be fixed before approval.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/std/tests/GH_000625_vector_bool_optimization/test.cpp:1638
operator[]moves backward for a positive index, contradicting bothoperator+and the random-access iterator requirement thati[n]be equivalent to*(i + n). This makes the regression-test iterator malformed and can hide failures in code paths that use indexing.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
I was looking into bit tricks for
next_permutation/prev_permutation.Seems like that there will be no impressive results, as these operations perform already at ~15 ns order of magnitude.
I may further look into these though.
But let's go for the easiest optimization here that gains 100x and more!
The optimized algorithm does mismatch, and then counts
truevalues in each of the remaining ranges.Mismatch
The
mismatchpart serves to save the equality case optimization that is implied by the standard, asking for N**2 comparisons generally, but just N if the ranges are equal. If none of the ranges isvector<bool>then we have these cases:mismatchturns to vector mismatch, andcountto vector countmismatchis slightly fastermismatchcall that will only likely misalign the input for thecountcall due to advancing few elementsmismatchthe number of comparison is twice smaller for equal cases.counts is vectorized, somismatchis not that much fasterSo overall
mismatchshould be there.One
vector<bool>or both of them flip it:mismatchforvector<bool>is not optimized currentlycountis optimized equally well for aligned and misaligned casesBenchmark results
Interim version is without the initial
mismatch. Its timings are not used is speedup calculation.Array:
vector<bool>, Interim column is irrelevant, it does not show any data different from After.