[libcxx-commits] [PATCH] D118003: [libc++] Floyd's improvement to pop_heap

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 6 inline comments as done.
Quuxplusone added inline comments.
Herald added a project: All.


================
Comment at: libcxx/include/__algorithm/sift_down.h:79
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _RandomAccessIterator
+__floyd_sift_down(_RandomAccessIterator __first, _Compare __comp,
+                  typename iterator_traits<_RandomAccessIterator>::difference_type __len)
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Mordante wrote:
> > > Since you use a know algorithm, please provide a link to the algorithm. Since the OP didn't know which paper I'm happy with just using https://en.wikipedia.org/wiki/Heapsort#Floyd's_heap_construction.
> > Can you add a benchmark for the new algorithm?
> https://en.wikipedia.org/wiki/Heapsort#Floyd's_heap_construction is describing the `make_heap` side of things (which we already implement): `make_heap` //can// be done with `push_heap` in a loop but that's slow. This PR is the `sort_heap` side of things: `sort_heap` is still done with `pop_heap` in a loop but `pop_heap` //itself// can be faster.
> 
> Ah, but this //is// the very next subsection: https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort I'll add a link to the commit message and also change the summary to mention "bottom-up heapsort / heapsort with bounce" since those are more googleable than "Floyd's improvement" :)
> 
> I'll investigate benchmarking. See the current commit message for my informal benchmark https://godbolt.org/z/M8do8sWhT ; it probably won't take much to turn it into a libcxx/benchmark/ test... although at the same time I don't know that anyone ever actually notices regressions in those benchmarks.
Turns out we already have libcxx/benchmarks/ code for this!
https://pastebin.com/iewiGLcN (before-bench.txt)
https://pastebin.com/MLP8f7y4 (after-bench.txt)

My hot takes are that:
- it's really noisy (he says defensively); note the changes (mostly badwards) in `MakeHeap`, whose actual code is unaffected by this patch
- this might be a //pessimization// for short arrays of trivial types, e.g. `BM_MakeThenSortHeap_uint64_Random_262144 ` goes from 101ns to 104ns
- this is, as expected, a major win for very long arrays of non-trivial types, e.g. `BM_MakeThenSortHeap_string_Random_262144` goes from 832ns to 669ns


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118003