[libcxx-commits] [PATCH] D150831: [libc++] Implement ranges::ends_with

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.

Thank you for working on this and addressing all the feedback! LGTM with a green CI (let me know if you need any help with CI failures).

I don't know if you have commit permissions -- if not, I can help merge the patch (please provide the name and the email address to use for the `author` field in that case).



================
Comment at: libcxx/benchmarks/algorithms/ranges_ends_with.bench.cpp:14
+
+#include <concepts>
+#include <vector>
----------------
Nit: I don't think this header is used.


================
Comment at: libcxx/benchmarks/algorithms/ranges_ends_with.bench.cpp:42
+    benchmark::DoNotOptimize(a);
+    benchmark::DoNotOptimize(p);
+    auto begin1 = random_access_iterator(a.begin());
----------------
Nit: please add a blank line after this line.


================
Comment at: libcxx/benchmarks/algorithms/ranges_ends_with.bench.cpp:67
+    benchmark::DoNotOptimize(std::ranges::ends_with(begin1, end1, begin2, end2));
+
+  }
----------------
Nit: extraneous blank line (here and in some other functions in this file).


================
Comment at: libcxx/benchmarks/algorithms/ranges_ends_with.bench.cpp:94
+  std::vector<int> p(state.range(), 1);
+  p.push_back(2);
+  for (auto _ : state) {
----------------
Nit: please add a blank line after this line for consistency with other functions in this file.


================
Comment at: libcxx/benchmarks/algorithms/ranges_ends_with.bench.cpp:14
+
+static void bm_ends_with_random_iter(benchmark::State& state) {
+  std::list<int> a(state.range(), 1);
----------------
philnik wrote:
> var-const wrote:
> > I tried out the benchmarks locally. After applying the fixes outlined above, I noticed a couple of interesting things:
> > 
> > 1. Random access iterators end up being a little slower than bidirectional iterators, at least in my environment. This is because `std::__equal_impl` ends up being a little slower than `std::ranges::__mismatch::__fn::__go` for the same arguments. It looks like the combination of calling `advance` on `__first1` (even though in this case it's a no-op) with the fact that `equal` checks `__first1 == __last1` in the end inhibits some optimization. I'm reluctant to try to fix this, though, at least in this patch -- the difference is small and isn't really specific to `ends_with`; it might also be dependent on the compiler.
> > 
> > 2. `equal` contains a powerful `memcmp` optimization for contiguous iterators but it doesn't get triggered currently. That's because `ends_with` wraps the predicate and the projections in a `reference_wrapper`, and the optimization only kicks in if (simplified):
> > ```
> > __is_trivial_equality_predicate<_Pred, _Tp, _Up>::value && __is_identity<_Proj1>::value && __is_identity<_Proj2>::value
> > ```
> > `__is_trivial_equality_predicate` recognizes `ranges::equal_to` as trivial, but not `reference_wrapper<ranges::equal_to>`, similarly for `__is_identity`.
> > 
> > There are a few potential solutions:
> > 1. Modify these traits to recognize `reference_wrapper`.
> > 2. Call an internal version of `ranges::equal` that takes functors by reference (needs to be written) or call `std::__equal` directly. It would also be necessary to duplicate the `__unwrap_iter` and `__unwrap_range` logic that's currently in `ranges::equal`.
> > 3. Stop providing the guarantee that we never copy functors in algorithms.
> > 
> > I'm currently leaning towards option 1. @philnik Nikolas, what do you think?
> I think both (1) and (3) make sense. (1) Seems like a nice generalizations, but I doubt it will make much of a difference 99% of the time, and (3) seems like it simplifies some things quite a bit. I also have the suspicion that all the references in the algorithms might inhibit optimizations, but I didn't test that yet. I also don't expect most predicates and projections to be non-trivial, making this an optimization that only applies very rarely, and potentially a pessimization in some cases (although that's just conjecture on my part).
@philnik Thanks!

@ZijunZhao As discussed offline, let's implement (1) in a follow-up.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150831/new/

https://reviews.llvm.org/D150831