[libcxx-commits] [PATCH] D122780: Modify std::sort: add BlockQuickSort partitioning algorithm for arithmetic types

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added subscribers: marcogelmi, Morwenn.
ldionne added a comment.

I am in favour of making our `std::sort` implementation better, and at some point this did seem like the right path forward. Since then we also made some changes with branchless sorting (ping @marcogelmi), and we added a bunch of benchmarks so that we have a better understanding of the impact of this change. I wonder whether this is still the right approach given that information.

It seems like you rebased onto `main` with the branchless sort changes and also re-ran the benchmarks against that. Looking at the results, it looks like this basically improves the performance by a fairly significant margin for randomly distributed integers, but seems to also decrease the performance quite a bit for integers with some sort of structure (e.g. already sorted, descending or ascending). In particular, I assume the reduction of branch mispredictions is not a huge factor anymore since we introduced branchless sorting, or am I off here? The fact that it doesn't seem to provide improvements across the board definitely make it a trickier change to vet, since we'll have to decide whether we want to make this tradeoff.

As it stands, we basically have to decide whether to optimize for randomly sorted data or to also take into account common patterns IIUC. That's not a super easy call to make, since we're talking about 40-50% improvements and 40-50% regressions in each case. I remember @Morwenn had mentioned pdqsort previously, and that seemed to be an algorithm that would handle both random data and also patterns properly. Would that solve the problems of this algorithm?

Sorry if this isn't super helpful, but I literally don't know how to make this call. I'd very much welcome the perspective of other folks who have given a lot of thought to sorting algorithms.

FWIW, making a major change to `std::sort` isn't a problem, as long as we have sufficient tests to validate its correctness, and that we are confident about its performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122780