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

Fri Apr 1 17:09:33 PDT 2022

nilayvaish added a comment.

In D122780#3420119 <https://reviews.llvm.org/D122780#3420119>, @philnik wrote:

> Could you do the formatting changes in its own patch?

Moved formatting changes to https://reviews.llvm.org/D122858.

> This will probably merge-conflict with D118029 <https://reviews.llvm.org/D118029>. I think D118029 <https://reviews.llvm.org/D118029> will be landed in the near future, so it would be nice to have data relative to that patch.

I'll update the data once D118029 <https://reviews.llvm.org/D118029> has landed.  Since D118029 <https://reviews.llvm.org/D118029> and this diff are changing different parts of the code, I expect the performance numbers to hold mostly, may go down a little bit maybe.

> There also seems to be a lot of regression in many test cases, mainly for the types smaller than 8 bytes. The larger ones seem to be better across the board.

The test cases that have regressed are the ones where a specific pattern is being tested.  While we theoretically match the time complexity i.e. wherever current implementation is linear (ascending, descending, single element), the new implementation is also linear.  Where the current implementation is non-linear, the new implementation is also non linear.  We do more memory operations as we create temporary bitsets for recording comparison operations.  But this cost is overcome by avoiding branches in the new implementation.  When branches are easy to predict, the current implementation does better.

> You say in the description that you have optimized different things. Are these optimizations orthogonal? If that is the case I think it would be nice to have them in separate patches to be able to review them separately.

If we don't do those other things (unguarded insertion sort, ninther pivot selection, equal elements on the left), the new implementation would appear more bad on those patterns.  I would suggest that we review whole diff in one go.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122780