[libcxx-commits] [PATCH] D141614: [libc++] nth_element change to partial sort when range is really small

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D141614#4053789 <https://reviews.llvm.org/D141614#4053789>, @Backl1ght wrote:

> It seems like the improvement is too small to make sense in benchmark. But from the output of code bellow we can tell that this reduce some calculations.
>
>   #include <algorithm>
>   #include <iostream>
>   #include <random>
>   #include <vector>
>   
>   int main(int argc, char** argv) {
>     const int REPETITIONS = 100'000;
>     const int N = 7;
>   
>     int seed = 998244353;
>     std::mt19937 rng(seed);
>     int num_comp = 0;
>     std::vector<int> a(N);
>     for (int i = 0; i < REPETITIONS; ++i) {
>       std::iota(a.begin(), a.end(), 0);
>       std::shuffle(a.begin(), a.end(), rng);
>   
>       int k = rng() % N;
>       std::nth_element(a.begin(), a.begin() + k, a.end(),
>                        [&](int x, int y) -> bool {
>                          ++num_comp;
>                          return x < y;
>                        });
>     }
>     std::cout << "num comp: " << num_comp << std::endl;
>     return 0;
>   }
>
> output before: `num comp: 2100000`
> output after: `num comp: 1597554`

I the improvement is too small to show up in a benchmark, I don't think it's actually an improvement. Maybe you could use a comparator that isn't trivial (e.g. use `std::string`)? This part at least shows that it isn't a pessimization for trivial comparators.



================
Comment at: libcxx/include/__algorithm/nth_element.h:42
+  for (; __first != __middle; ++__first) {
+    _BidirectionalIterator __i = std::__min_element(__first, __last, __comp);
+    if (__i != __first)
----------------
Why not `min_element`? Using the public API makes it always easier to refactor internal stuff, since that part is stable.


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

https://reviews.llvm.org/D141614