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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/sift_down.h:78
+template <class _Compare, class _RandomAccessIterator>
+_LIBCPP_CONSTEXPR_AFTER_CXX11 _RandomAccessIterator
+__floyd_sift_down(_RandomAccessIterator __first, _Compare __comp,
----------------
philnik wrote:
> Is `_LIBCPP_HIDE_FROM_ABI` purposefully not here?
Nope, accidental... but maybe it //should// be on purpose? We don't put either `inline _LIBCPP_HIDE_FROM_ABI` or `inline _LIBCPP_INLINE_VISIBILITY` on ordinary `__sift_down` either; nor on `__make_heap`; nor on `__nth_element`... It seems like (at least in this old code) the visibility macro is intentionally affiliated with the `inline` keyword, and the `inline` keyword goes only on short functions. I don't know whether we should be following that rule, or fixing it. My default of course is to leave it as-is (i.e. not add `_LIBCPP_HIDE_FROM_ABI` here).


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


================
Comment at: libcxx/include/__algorithm/sift_down.h:82
+{
+    typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
+    _LIBCPP_ASSERT(__len >= 2, "shouldn't be called unless __len >= 2");
----------------
philnik wrote:
> 
FWIW this is not libc++'s historical style, but since Clang C++03 supports `using=` now, I'm cool with it if everyone else is. Will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118003