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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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);
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150831